[PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[Leon Romanovsky]

From: Maor Gottlieb <maorg@mellanox.com>

We don't need to set pkey as valid in case that user set only one
of pkey index or port number, otherwise it will be resulted in NULL
pointer dereference while accessing to uninitialized pkey list.

The following crash from syzkaller revealed it.

kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN PTI
CPU: 1 PID: 14753 Comm: syz-executor.2 Not tainted 5.5.0-rc5 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
RIP: 0010:get_pkey_idx_qp_list+0x161/0x2d0
Code: 01 00 00 49 8b 5e 20 4c 39 e3 0f 84 b9 00 00 00 e8 e4 42 6e fe 48
8d 7b 10 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04
02 84 c0 74 08 3c 01 0f 8e d0 00 00 00 48 8d 7d 04 48 b8
RSP: 0018:ffffc9000bc6f950 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff82c8bdec
RDX: 0000000000000002 RSI: ffffc900030a8000 RDI: 0000000000000010
RBP: ffff888112c8ce80 R08: 0000000000000004 R09: fffff5200178df1f
R10: 0000000000000001 R11: fffff5200178df1f R12: ffff888115dc4430
R13: ffff888115da8498 R14: ffff888115dc4410 R15: ffff888115da8000
FS:  00007f20777de700(0000) GS:ffff88811b100000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b2f721000 CR3: 00000001173ca002 CR4: 0000000000360ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 port_pkey_list_insert+0xd7/0x7c0
 ib_security_modify_qp+0x6fa/0xfc0
 _ib_modify_qp+0x8c4/0xbf0
 modify_qp+0x10da/0x16d0
 ib_uverbs_modify_qp+0x9a/0x100
 ib_uverbs_write+0xaa5/0xdf0
 __vfs_write+0x7c/0x100
 vfs_write+0x168/0x4a0
 ksys_write+0xc8/0x200
 do_syscall_64+0x9c/0x390
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: d291f1a652329 ("IB/core: Enforce PKey security on QPs")
Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/security.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/security.c b/drivers/infiniband/core/security.c
index 6eb6d2717ca5..a34e11757bc5 100644
--- a/drivers/infiniband/core/security.c
+++ b/drivers/infiniband/core/security.c
@@ -353,7 +353,9 @@ static struct ib_ports_pkeys *get_new_pps(const struct ib_qp *qp,
 					 qp_attr->pkey_index :
 					 qp_pps->main.pkey_index;
 		}
-		new_pps->main.state = IB_PORT_PKEY_VALID;
+		if ((qp_attr_mask & IB_QP_PKEY_INDEX) &&
+		    (qp_attr_mask & IB_QP_PORT))
+			new_pps->main.state = IB_PORT_PKEY_VALID;
 	} else if (qp_pps) {
 		new_pps->main.port_num = qp_pps->main.port_num;
 		new_pps->main.pkey_index = qp_pps->main.pkey_index;
-- 
2.24.1

Re: [PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[Gal Pressman]

On 26/01/2020 19:15, Leon Romanovsky wrote:
> From: Maor Gottlieb <maorg@mellanox.com>
> 
> We don't need to set pkey as valid in case that user set only one
> of pkey index or port number, otherwise it will be resulted in NULL
> pointer dereference while accessing to uninitialized pkey list.

Why would the pkey list be uninitialized? Isn't it initialized as an empty list
on device registration?

Re: [PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[Maor Gottlieb]

On 1/29/2020 2:06 PM, Gal Pressman wrote:
> On 26/01/2020 19:15, Leon Romanovsky wrote:
>> From: Maor Gottlieb <maorg@mellanox.com>
>>
>> We don't need to set pkey as valid in case that user set only one
>> of pkey index or port number, otherwise it will be resulted in NULL
>> pointer dereference while accessing to uninitialized pkey list.
> Why would the pkey list be uninitialized? Isn't it initialized as an empty list
> on device registration?

It will try to access to list of invalid port / pkey, e.g. to list of 
port 0. port_data is indexed by port number.
dev->port_data[pp->port_num].pkey_list

Re: [PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[Gal Pressman]

On 29/01/2020 14:14, Maor Gottlieb wrote:
> 
> On 1/29/2020 2:06 PM, Gal Pressman wrote:
>> On 26/01/2020 19:15, Leon Romanovsky wrote:
>>> From: Maor Gottlieb <maorg@mellanox.com>
>>>
>>> We don't need to set pkey as valid in case that user set only one
>>> of pkey index or port number, otherwise it will be resulted in NULL
>>> pointer dereference while accessing to uninitialized pkey list.
>> Why would the pkey list be uninitialized? Isn't it initialized as an empty list
>> on device registration?
> 
> It will try to access to list of invalid port / pkey, e.g. to list of 
> port 0. port_data is indexed by port number.
> dev->port_data[pp->port_num].pkey_list

Makes sense.
Shouldn't there be a check in the (!qp_pps) section as well? We shouldn't assign
the field unless the mask is given.

Does this work correctly if the user issues two calls to modify_qp where the
first one modifies the pkey index and the second the port number (if that's even
possible)?
Is it expected that the state would stay invalid?

Re: [PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[Jason Gunthorpe]

On Wed, Jan 29, 2020 at 02:43:51PM +0200, Gal Pressman wrote:
> On 29/01/2020 14:14, Maor Gottlieb wrote:
> > 
> > On 1/29/2020 2:06 PM, Gal Pressman wrote:
> >> On 26/01/2020 19:15, Leon Romanovsky wrote:
> >>> From: Maor Gottlieb <maorg@mellanox.com>
> >>>
> >>> We don't need to set pkey as valid in case that user set only one
> >>> of pkey index or port number, otherwise it will be resulted in NULL
> >>> pointer dereference while accessing to uninitialized pkey list.
> >> Why would the pkey list be uninitialized? Isn't it initialized as an empty list
> >> on device registration?
> > 
> > It will try to access to list of invalid port / pkey, e.g. to list of 
> > port 0. port_data is indexed by port number.
> > dev->port_data[pp->port_num].pkey_list
> 
> Makes sense.
> Shouldn't there be a check in the (!qp_pps) section as well? We shouldn't assign
> the field unless the mask is given.

Indeed, reading a qp_attr field without the corresponding bt in
qp_attr_mask set should be wrong.
 
> Does this work correctly if the user issues two calls to modify_qp where the
> first one modifies the pkey index and the second the port number (if that's even
> possible)?
> Is it expected that the state would stay invalid?

Also sounds wrong

.. and then there is the confusing testing of state !=
IB_PORT_PKEY_NOT_VALID but nothing ever assigns
IB_PORT_PKEY_NOT_VALID.. Humm.

Jason

Re: [PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[Maor Gottlieb]

On 1/29/2020 10:11 PM, Jason Gunthorpe wrote:
> On Wed, Jan 29, 2020 at 02:43:51PM +0200, Gal Pressman wrote:
>> On 29/01/2020 14:14, Maor Gottlieb wrote:
>>> On 1/29/2020 2:06 PM, Gal Pressman wrote:
>>>> On 26/01/2020 19:15, Leon Romanovsky wrote:
>>>>> From: Maor Gottlieb <maorg@mellanox.com>
>>>>>
>>>>> We don't need to set pkey as valid in case that user set only one
>>>>> of pkey index or port number, otherwise it will be resulted in NULL
>>>>> pointer dereference while accessing to uninitialized pkey list.
>>>> Why would the pkey list be uninitialized? Isn't it initialized as an empty list
>>>> on device registration?
>>> It will try to access to list of invalid port / pkey, e.g. to list of
>>> port 0. port_data is indexed by port number.
>>> dev->port_data[pp->port_num].pkey_list
>> Makes sense.
>> Shouldn't there be a check in the (!qp_pps) section as well? We shouldn't assign
>> the field unless the mask is given.
> Indeed, reading a qp_attr field without the corresponding bt in
> qp_attr_mask set should be wrong.

Agree, but it doesn't affect cause we don't use this value if the pkey 
is not valid. However I can add it.
>   
>> Does this work correctly if the user issues two calls to modify_qp where the
>> first one modifies the pkey index and the second the port number (if that's even
>> possible)?
>> Is it expected that the state would stay invalid?
> Also sounds wrong

When QP is modified from reset to init, both pkey and port should be 
set, but this check happens later when the vendor call to 
ib_modify_qp_is_ok.
>
> .. and then there is the confusing testing of state !=
> IB_PORT_PKEY_NOT_VALID but nothing ever assigns
> IB_PORT_PKEY_NOT_VALID.. Humm.

IB_PORT_PKEY_NOT_VALID is the default value, set when we do kzalloc.

>
> Jason

Re: [PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[Leon Romanovsky]

On Sun, Jan 26, 2020 at 07:15:53PM +0200, Leon Romanovsky wrote:
> From: Maor Gottlieb <maorg@mellanox.com>
>
> We don't need to set pkey as valid in case that user set only one
> of pkey index or port number, otherwise it will be resulted in NULL
> pointer dereference while accessing to uninitialized pkey list.
>
> The following crash from syzkaller revealed it.
>
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN PTI
> CPU: 1 PID: 14753 Comm: syz-executor.2 Not tainted 5.5.0-rc5 #2
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> RIP: 0010:get_pkey_idx_qp_list+0x161/0x2d0
> Code: 01 00 00 49 8b 5e 20 4c 39 e3 0f 84 b9 00 00 00 e8 e4 42 6e fe 48
> 8d 7b 10 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04
> 02 84 c0 74 08 3c 01 0f 8e d0 00 00 00 48 8d 7d 04 48 b8
> RSP: 0018:ffffc9000bc6f950 EFLAGS: 00010202
> RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff82c8bdec
> RDX: 0000000000000002 RSI: ffffc900030a8000 RDI: 0000000000000010
> RBP: ffff888112c8ce80 R08: 0000000000000004 R09: fffff5200178df1f
> R10: 0000000000000001 R11: fffff5200178df1f R12: ffff888115dc4430
> R13: ffff888115da8498 R14: ffff888115dc4410 R15: ffff888115da8000
> FS:  00007f20777de700(0000) GS:ffff88811b100000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b2f721000 CR3: 00000001173ca002 CR4: 0000000000360ee0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  port_pkey_list_insert+0xd7/0x7c0
>  ib_security_modify_qp+0x6fa/0xfc0
>  _ib_modify_qp+0x8c4/0xbf0
>  modify_qp+0x10da/0x16d0
>  ib_uverbs_modify_qp+0x9a/0x100
>  ib_uverbs_write+0xaa5/0xdf0
>  __vfs_write+0x7c/0x100
>  vfs_write+0x168/0x4a0
>  ksys_write+0xc8/0x200
>  do_syscall_64+0x9c/0x390
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Fixes: d291f1a652329 ("IB/core: Enforce PKey security on QPs")
> Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/core/security.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

As Maor explained below, the code is correct, so what is the resolution here?

Thanks

Re: [PATCH rdma-next] RDMA/core: Fix protection fault in get_pkey_idx_qp_list

[Jason Gunthorpe]

On Sun, Feb 02, 2020 at 11:33:08AM +0200, Leon Romanovsky wrote:
> On Sun, Jan 26, 2020 at 07:15:53PM +0200, Leon Romanovsky wrote:
> > From: Maor Gottlieb <maorg@mellanox.com>
> >
> > We don't need to set pkey as valid in case that user set only one
> > of pkey index or port number, otherwise it will be resulted in NULL
> > pointer dereference while accessing to uninitialized pkey list.
> >
> > The following crash from syzkaller revealed it.
> >
> > kasan: CONFIG_KASAN_INLINE enabled
> > kasan: GPF could be caused by NULL-ptr deref or user memory access
> > general protection fault: 0000 [#1] SMP KASAN PTI
> > CPU: 1 PID: 14753 Comm: syz-executor.2 Not tainted 5.5.0-rc5 #2
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
> > RIP: 0010:get_pkey_idx_qp_list+0x161/0x2d0
> > Code: 01 00 00 49 8b 5e 20 4c 39 e3 0f 84 b9 00 00 00 e8 e4 42 6e fe 48
> > 8d 7b 10 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04
> > 02 84 c0 74 08 3c 01 0f 8e d0 00 00 00 48 8d 7d 04 48 b8
> > RSP: 0018:ffffc9000bc6f950 EFLAGS: 00010202
> > RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff82c8bdec
> > RDX: 0000000000000002 RSI: ffffc900030a8000 RDI: 0000000000000010
> > RBP: ffff888112c8ce80 R08: 0000000000000004 R09: fffff5200178df1f
> > R10: 0000000000000001 R11: fffff5200178df1f R12: ffff888115dc4430
> > R13: ffff888115da8498 R14: ffff888115dc4410 R15: ffff888115da8000
> > FS:  00007f20777de700(0000) GS:ffff88811b100000(0000)
> > knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000001b2f721000 CR3: 00000001173ca002 CR4: 0000000000360ee0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> >  port_pkey_list_insert+0xd7/0x7c0
> >  ib_security_modify_qp+0x6fa/0xfc0
> >  _ib_modify_qp+0x8c4/0xbf0
> >  modify_qp+0x10da/0x16d0
> >  ib_uverbs_modify_qp+0x9a/0x100
> >  ib_uverbs_write+0xaa5/0xdf0
> >  __vfs_write+0x7c/0x100
> >  vfs_write+0x168/0x4a0
> >  ksys_write+0xc8/0x200
> >  do_syscall_64+0x9c/0x390
> >  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >
> > Fixes: d291f1a652329 ("IB/core: Enforce PKey security on QPs")
> > Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/security.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> 
> As Maor explained below, the code is correct, so what is the resolution here?

Attributes should still not be accessed without their masks set, I
don't see how that is correct just because in some tricky way the
2nd copy isn't read

Jason