Re: [PATCH security IB/uverbs: Perform validity check for supplied port number in create_ah
From: Doug Ledford <hidden>
Date: 2017-06-29 16:42:01
On Tue, 2017-06-27 at 15:09 +0300, Leon Romanovsky wrote:
Hi Doug and Security Team, How should we proceed with the following patch? The malicious user (non-root) can send ib_create_ah() comamnd to exposed /sys/class/infiniband_verbs/uverbs* file.
Your explanation is not making a lot of sense. The /sys/class/infiniband_verbs/uverbs* files are directories, not files. Are you saying we have an open method by which you can open the directory in question and then send ib verbs commands over the directory file? And even if you really mean some other file in that directory, why would we be fixing the create_ah handler instead of fixing the sysfs write handler for that file to not accept ib verbs commands? Why would we be talking ib verbs commands on *any* sysfs file?
All that is needed is to provide port number which is out-of-range and it will kill the system.
Why is this not an issue on the normal /dev/infiniband/uverbs* files (which are world writable)? Is that merely because rdma- core/libibverbs checks the port number before submitting the command? If so, then a maliciously changed rdma-core/libibverbs would have the same problem.
There is need to be root to open uverbs* file, but after that those persmissions can be dropped.
I don't get the issue with the sysfs file, but the bug appears to be exploitable by any user who is willing to recompile rdma-core to bypass any checks it might have on input validity. From what I can tell, the offending code that should be the source of the problem is rdma_ah_find_type() which uses the user supplied port_num for an array lookup without checking it for validity, thereby being tricked into going outside the array bounds by user input. We call rdma_ah_find_type() in two locations: ib_verbs.c:modify_qp() and ib_verbs.c:ib_uverbs_create_ah(). Why is this a bug in one and not the other? And in modify_qp() it looks like we actually use cmd-
base.port_num, cmd->base.alt_port_num, and cmd->base.dest.port_num,
all as user provided values without checking.
quoted hunk ↗ jump to hunk
Thanks --- drivers/infiniband/core/uverbs_cmd.c | 3 +++ 1 file changed, 3 insertions(+)diff --git a/drivers/infiniband/core/uverbs_cmd.cb/drivers/infiniband/core/uverbs_cmd.c index 70b7fb156414..6065395b6465 100644--- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c@@ -2541,6 +2541,9 @@ ssize_t ib_uverbs_create_ah(structib_uverbs_file *file, if (copy_from_user(&cmd, buf, sizeof cmd)) return -EFAULT; + if (!rdma_is_port_valid(ib_dev, cmd.attr.port_num)) + return -EINVAL; + INIT_UDATA(&udata, buf + sizeof(cmd), (unsigned long)cmd.response + sizeof(resp), in_len - sizeof(cmd), out_len - sizeof(resp)); -- 2.13.1
--
Doug Ledford [off-list ref]
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD