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

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

[Leon Romanovsky]

From: Boris Pismenny <borisp@mellanox.com>

The ib_uverbs_create_ah() call receives the port number as part of its
attributes and assumes it is valid. Down on the stack, that parameter
is used to access kernel data structures.

BUG: KASAN: use-after-free in ib_uverbs_create_ah+0x6d5/0x7b0
Read of size 4 at addr ffff880018d67ab8 by task syz-executor/313

CPU: 0 PID: 313 Comm: syz-executor Not tainted 4.12.0-rc3+ #4
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
Call Trace:
 dump_stack+0x95/0xeb
 print_address_description+0x78/0x290
 kasan_report+0x25f/0x370
 ? ib_uverbs_create_ah+0x6d5/0x7b0
 __asan_report_load4_noabort+0x19/0x20
 ib_uverbs_create_ah+0x6d5/0x7b0
 ? ib_uverbs_post_srq_recv+0x4f0/0x4f0
 ? sched_clock_cpu+0x1b/0x190
 ? sched_clock_cpu+0x1b/0x190
 ? sched_clock_cpu+0x1b/0x190
 ? __lock_acquire+0x9ed/0x14e0
 ib_uverbs_write+0x5a5/0xb20
 ? ib_uverbs_write+0x5a5/0xb20
 ? ib_uverbs_post_srq_recv+0x4f0/0x4f0
 ? ib_uverbs_post_srq_recv+0x4f0/0x4f0
 ? ib_uverbs_open+0x740/0x740
 ? lock_acquire+0x370/0x370
 ? trace_hardirqs_on+0xd/0x10
 ? ldsem_up_read+0x3f/0x70
 ? sched_clock_cpu+0x1b/0x190
 ? sched_clock_cpu+0x1b/0x190
 __vfs_write+0x118/0x580
 ? sched_clock_cpu+0x1b/0x190
 ? ib_uverbs_open+0x740/0x740
 ? __vfs_read+0x560/0x560
 ? lock_acquire+0x370/0x370
 ? __fget+0x4c/0x380
 ? __fget+0x234/0x380
 ? rw_verify_area+0xca/0x290
 vfs_write+0x192/0x490
 SyS_write+0xde/0x1c0
 ? SyS_read+0x1c0/0x1c0
 ? trace_hardirqs_on_thunk+0x1a/0x1c
 entry_SYSCALL_64_fastpath+0x18/0xad
RIP: 0033:0x4471f9
RSP: 002b:00007f44d0379c18 EFLAGS: 00000216 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 0000000000708000 RCX: 00000000004471f9
RDX: 0000000000000018 RSI: 0000000020025000 RDI: 0000000000000003
RBP: 0000000000000046 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000216 R12: ffff880014b7ff98
R13: 0000000020025000 R14: 0000000000000003 R15: 00000000ffffffff

Fixes: 67cdb40ca444 ("[IB] uverbs: Implement more commands")
Cc: <stable@vger.kernel.org> # v2.6.14+
Cc: <security@kernel.org>
Cc: Yevgeny Kliteynik <kliteyn@mellanox.com>
Cc: Tziporet Koren <tziporet@mellanox.com>
Cc: Alex Polak <alexpo@mellanox.com>
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
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. All that is
needed is to provide port number which is out-of-range and it will
kill the system.

There is need to be root to open uverbs* file, but after that those
persmissions can be dropped.

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

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

[Doug Ledford]

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

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3181 bytes --]

On Thu, Jun 29, 2017 at 12:41:58PM -0400, Doug Ledford wrote:
> 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?

You are right,
It was my mistake and the real issue with /dev/infiniband/uverbs0, which
is open for everypne for write:

root@mtr-leonro:/sys/class/infiniband_verbs# ls -al /dev/infiniband/uverbs0
crw-rw-rw- 1 root root 231, 192 Jun 29 14:24 /dev/infiniband/uverbs0


>
> >  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.

It is exactly the problem. We started to run fuzzy testing on the
ibverbs interface with direct calls to uverbs files without any
libibverbs/rdma-core involvement. For years, we checked our stack as
a bundle (user + kernel), this is why libibvers/rdma-core limited us
to find it before.

>
> > 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.

Please take my apology for sysfs. It was a mistake.

You don't need to create special library for that. Boris has
reproduction program, which does open/mmap/write in similar way to
rdma-core.


> 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?

We don't have fuzzy templates for all commands yet. IMHO, Boris has only
3-4 commands and didn't do modify_qp yet.

> 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.

In general, I have 2 more similar bugs pending submissions and would
like to know the procedure.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Linus Torvalds]

On Thu, Jun 29, 2017 at 11:16 AM, Leon Romanovsky <leon@kernel.org> wrote:
>
> In general, I have 2 more similar bugs pending submissions and would
> like to know the procedure.

I think the procedure is to just submit them and mark them for stable.

Do *not* include a reproducer (or necessarily enough hints to
trivially write one). Although with rdma, this is presumably limited
to only hardware that actually has rdma support (and exposes it, so
presumably not any VM in server farms where the hardware does do
rdma), so it's presumably not a huge issue.

I mean, if you actually have shell access on some server (and not in a
VM), you already pretty much are the sysadmin.

                   Linus

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

On Thu, Jun 29, 2017 at 11:30:48AM -0700, Linus Torvalds wrote:
> On Thu, Jun 29, 2017 at 11:16 AM, Leon Romanovsky <leon@kernel.org> wrote:
> >
> > In general, I have 2 more similar bugs pending submissions and would
> > like to know the procedure.
>
> I think the procedure is to just submit them and mark them for stable.
>
> Do *not* include a reproducer (or necessarily enough hints to
> trivially write one). Although with rdma, this is presumably limited
> to only hardware that actually has rdma support (and exposes it, so
> presumably not any VM in server farms where the hardware does do
> rdma), so it's presumably not a huge issue.
>
> I mean, if you actually have shell access on some server (and not in a
> VM), you already pretty much are the sysadmin.

Thanks, we will do.

>
>                    Linus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]