Re: [PATCH security IB/uverbs: Perform validity check for supplied port number in create_ah

[Doug Ledford <dledford@redhat.com>]

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.

> Thanks
> ---
>  drivers/infiniband/core/uverbs_cmd.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/infiniband/core/uverbs_cmd.c
> b/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(struct
> ib_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 <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD