[PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

The ib->rec.qkey field is accessed without being initialized.
Clear the ib_sa_multicast struct to fix the following syzkaller error/.

=====================================================
BUG: KMSAN: uninit-value in cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
BUG: KMSAN: uninit-value in cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
 cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
 cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
 cma_iboe_join_multicast drivers/infiniband/core/cma.c:4782 [inline]
 rdma_join_multicast+0x2b83/0x30a0 drivers/infiniband/core/cma.c:4814
 ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
 ucma_join_multicast+0x1e3/0x250 drivers/infiniband/core/ucma.c:1546
 ucma_write+0x639/0x6d0 drivers/infiniband/core/ucma.c:1732
 vfs_write+0x8ce/0x2030 fs/read_write.c:588
 ksys_write+0x28c/0x520 fs/read_write.c:643
 __do_sys_write fs/read_write.c:655 [inline]
 __se_sys_write fs/read_write.c:652 [inline]
 __ia32_sys_write+0xdb/0x120 fs/read_write.c:652
 do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
 __do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
 do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
 do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
 entry_SYSENTER_compat_after_hwframe+0x4d/0x5c

Local variable ib.i created at:
 cma_iboe_join_multicast drivers/infiniband/core/cma.c:4737 [inline]
 rdma_join_multicast+0x586/0x30a0 drivers/infiniband/core/cma.c:4814
 ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479

CPU: 0 PID: 29874 Comm: syz-executor.3 Not tainted 5.16.0-rc3-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
=====================================================

Fixes: b5de0c60cc30 ("RDMA/cma: Fix use after free race in roce multicast join")
Reported-by: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/cma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 69c9a12dd14e..9c53e1e7de50 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
 	int err = 0;
 	struct sockaddr *addr = (struct sockaddr *)&mc->addr;
 	struct net_device *ndev = NULL;
-	struct ib_sa_multicast ib;
+	struct ib_sa_multicast ib = {};
 	enum ib_gid_type gid_type;
 	bool send_only;
 
-- 
2.33.1

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Jason Gunthorpe]

On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> The ib->rec.qkey field is accessed without being initialized.
> Clear the ib_sa_multicast struct to fix the following syzkaller error/.
> 
> =====================================================
> BUG: KMSAN: uninit-value in cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> BUG: KMSAN: uninit-value in cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
>  cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
>  cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
>  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4782 [inline]
>  rdma_join_multicast+0x2b83/0x30a0 drivers/infiniband/core/cma.c:4814
>  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
>  ucma_join_multicast+0x1e3/0x250 drivers/infiniband/core/ucma.c:1546
>  ucma_write+0x639/0x6d0 drivers/infiniband/core/ucma.c:1732
>  vfs_write+0x8ce/0x2030 fs/read_write.c:588
>  ksys_write+0x28c/0x520 fs/read_write.c:643
>  __do_sys_write fs/read_write.c:655 [inline]
>  __se_sys_write fs/read_write.c:652 [inline]
>  __ia32_sys_write+0xdb/0x120 fs/read_write.c:652
>  do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
>  __do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
>  do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
>  do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
>  entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
> 
> Local variable ib.i created at:
>  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4737 [inline]
>  rdma_join_multicast+0x586/0x30a0 drivers/infiniband/core/cma.c:4814
>  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> 
> CPU: 0 PID: 29874 Comm: syz-executor.3 Not tainted 5.16.0-rc3-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> =====================================================
> 
> Fixes: b5de0c60cc30 ("RDMA/cma: Fix use after free race in roce multicast join")
> Reported-by: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>  drivers/infiniband/core/cma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 69c9a12dd14e..9c53e1e7de50 100644
> +++ b/drivers/infiniband/core/cma.c
> @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
>  	int err = 0;
>  	struct sockaddr *addr = (struct sockaddr *)&mc->addr;
>  	struct net_device *ndev = NULL;
> -	struct ib_sa_multicast ib;
> +	struct ib_sa_multicast ib = {};
>  	enum ib_gid_type gid_type;
>  	bool send_only;

We shouldn't be able to join anything except a RDMA_PS_UDP to a
multicast in the first place:

	if (id_priv->id.ps == RDMA_PS_UDP)
		ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);

Multicast RC/etc is meaningless. So I guess it should be like this:

--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -4744,7 +4744,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
 
        send_only = mc->join_state == BIT(SENDONLY_FULLMEMBER_JOIN);
 
-       if (cma_zero_addr(addr))
+       if (cma_zero_addr(addr) || id_priv->id.ps != RDMA_PS_UDP)
                return -EINVAL;
 
        gid_type = id_priv->cma_dev->default_gid_type[id_priv->id.port_num -
@@ -4752,8 +4752,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
        cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
 
        ib.rec.pkey = cpu_to_be16(0xffff);
-       if (id_priv->id.ps == RDMA_PS_UDP)
-               ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
+       ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
 
        if (dev_addr->bound_dev_if)
                ndev = dev_get_by_index(dev_addr->net, dev_addr->bound_dev_if);

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Leon Romanovsky]

On Thu, Jan 06, 2022 at 01:39:41PM -0400, Jason Gunthorpe wrote:
> On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > The ib->rec.qkey field is accessed without being initialized.
> > Clear the ib_sa_multicast struct to fix the following syzkaller error/.
> > 
> > =====================================================
> > BUG: KMSAN: uninit-value in cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> > BUG: KMSAN: uninit-value in cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
> >  cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> >  cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
> >  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4782 [inline]
> >  rdma_join_multicast+0x2b83/0x30a0 drivers/infiniband/core/cma.c:4814
> >  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> >  ucma_join_multicast+0x1e3/0x250 drivers/infiniband/core/ucma.c:1546
> >  ucma_write+0x639/0x6d0 drivers/infiniband/core/ucma.c:1732
> >  vfs_write+0x8ce/0x2030 fs/read_write.c:588
> >  ksys_write+0x28c/0x520 fs/read_write.c:643
> >  __do_sys_write fs/read_write.c:655 [inline]
> >  __se_sys_write fs/read_write.c:652 [inline]
> >  __ia32_sys_write+0xdb/0x120 fs/read_write.c:652
> >  do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
> >  __do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
> >  do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
> >  do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
> >  entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
> > 
> > Local variable ib.i created at:
> >  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4737 [inline]
> >  rdma_join_multicast+0x586/0x30a0 drivers/infiniband/core/cma.c:4814
> >  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> > 
> > CPU: 0 PID: 29874 Comm: syz-executor.3 Not tainted 5.16.0-rc3-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > =====================================================
> > 
> > Fixes: b5de0c60cc30 ("RDMA/cma: Fix use after free race in roce multicast join")
> > Reported-by: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> >  drivers/infiniband/core/cma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 69c9a12dd14e..9c53e1e7de50 100644
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> >  	int err = 0;
> >  	struct sockaddr *addr = (struct sockaddr *)&mc->addr;
> >  	struct net_device *ndev = NULL;
> > -	struct ib_sa_multicast ib;
> > +	struct ib_sa_multicast ib = {};
> >  	enum ib_gid_type gid_type;
> >  	bool send_only;
> 
> We shouldn't be able to join anything except a RDMA_PS_UDP to a
> multicast in the first place:
> 
> 	if (id_priv->id.ps == RDMA_PS_UDP)
> 		ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> 
> Multicast RC/etc is meaningless. So I guess it should be like this:

Strange that we don't have repro for such deterministic flow.

> 
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -4744,7 +4744,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
>  
>         send_only = mc->join_state == BIT(SENDONLY_FULLMEMBER_JOIN);
>  
> -       if (cma_zero_addr(addr))
> +       if (cma_zero_addr(addr) || id_priv->id.ps != RDMA_PS_UDP)
>                 return -EINVAL;
>  
>         gid_type = id_priv->cma_dev->default_gid_type[id_priv->id.port_num -
> @@ -4752,8 +4752,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
>         cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
>  
>         ib.rec.pkey = cpu_to_be16(0xffff);
> -       if (id_priv->id.ps == RDMA_PS_UDP)
> -               ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> +       ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
>  
>         if (dev_addr->bound_dev_if)
>                 ndev = dev_get_by_index(dev_addr->net, dev_addr->bound_dev_if);

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Leon Romanovsky]

On Thu, Jan 06, 2022 at 01:39:41PM -0400, Jason Gunthorpe wrote:
> On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > The ib->rec.qkey field is accessed without being initialized.
> > Clear the ib_sa_multicast struct to fix the following syzkaller error/.
> > 
> > =====================================================
> > BUG: KMSAN: uninit-value in cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> > BUG: KMSAN: uninit-value in cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
> >  cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> >  cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
> >  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4782 [inline]
> >  rdma_join_multicast+0x2b83/0x30a0 drivers/infiniband/core/cma.c:4814
> >  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> >  ucma_join_multicast+0x1e3/0x250 drivers/infiniband/core/ucma.c:1546
> >  ucma_write+0x639/0x6d0 drivers/infiniband/core/ucma.c:1732
> >  vfs_write+0x8ce/0x2030 fs/read_write.c:588
> >  ksys_write+0x28c/0x520 fs/read_write.c:643
> >  __do_sys_write fs/read_write.c:655 [inline]
> >  __se_sys_write fs/read_write.c:652 [inline]
> >  __ia32_sys_write+0xdb/0x120 fs/read_write.c:652
> >  do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
> >  __do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
> >  do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
> >  do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
> >  entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
> > 
> > Local variable ib.i created at:
> >  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4737 [inline]
> >  rdma_join_multicast+0x586/0x30a0 drivers/infiniband/core/cma.c:4814
> >  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> > 
> > CPU: 0 PID: 29874 Comm: syz-executor.3 Not tainted 5.16.0-rc3-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > =====================================================
> > 
> > Fixes: b5de0c60cc30 ("RDMA/cma: Fix use after free race in roce multicast join")
> > Reported-by: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> >  drivers/infiniband/core/cma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 69c9a12dd14e..9c53e1e7de50 100644
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> >  	int err = 0;
> >  	struct sockaddr *addr = (struct sockaddr *)&mc->addr;
> >  	struct net_device *ndev = NULL;
> > -	struct ib_sa_multicast ib;
> > +	struct ib_sa_multicast ib = {};
> >  	enum ib_gid_type gid_type;
> >  	bool send_only;
> 
> We shouldn't be able to join anything except a RDMA_PS_UDP to a
> multicast in the first place:
> 
> 	if (id_priv->id.ps == RDMA_PS_UDP)
> 		ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> 
> Multicast RC/etc is meaningless. So I guess it should be like this:

I don't know, I used 0 exactly like we have for cma_join_ib_multicast().

Where can I read about this PS limitation? I didn't find anything
relevant in the IBTA spec.

Thanks

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Jason Gunthorpe]

On Sun, Jan 09, 2022 at 02:22:04PM +0200, Leon Romanovsky wrote:
> On Thu, Jan 06, 2022 at 01:39:41PM -0400, Jason Gunthorpe wrote:
> > On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > The ib->rec.qkey field is accessed without being initialized.
> > > Clear the ib_sa_multicast struct to fix the following syzkaller error/.
> > > 
> > > =====================================================
> > > BUG: KMSAN: uninit-value in cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> > > BUG: KMSAN: uninit-value in cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
> > >  cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> > >  cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
> > >  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4782 [inline]
> > >  rdma_join_multicast+0x2b83/0x30a0 drivers/infiniband/core/cma.c:4814
> > >  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> > >  ucma_join_multicast+0x1e3/0x250 drivers/infiniband/core/ucma.c:1546
> > >  ucma_write+0x639/0x6d0 drivers/infiniband/core/ucma.c:1732
> > >  vfs_write+0x8ce/0x2030 fs/read_write.c:588
> > >  ksys_write+0x28c/0x520 fs/read_write.c:643
> > >  __do_sys_write fs/read_write.c:655 [inline]
> > >  __se_sys_write fs/read_write.c:652 [inline]
> > >  __ia32_sys_write+0xdb/0x120 fs/read_write.c:652
> > >  do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
> > >  __do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
> > >  do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
> > >  do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
> > >  entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
> > > 
> > > Local variable ib.i created at:
> > >  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4737 [inline]
> > >  rdma_join_multicast+0x586/0x30a0 drivers/infiniband/core/cma.c:4814
> > >  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> > > 
> > > CPU: 0 PID: 29874 Comm: syz-executor.3 Not tainted 5.16.0-rc3-syzkaller #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > =====================================================
> > > 
> > > Fixes: b5de0c60cc30 ("RDMA/cma: Fix use after free race in roce multicast join")
> > > Reported-by: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > >  drivers/infiniband/core/cma.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > > index 69c9a12dd14e..9c53e1e7de50 100644
> > > +++ b/drivers/infiniband/core/cma.c
> > > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> > >  	int err = 0;
> > >  	struct sockaddr *addr = (struct sockaddr *)&mc->addr;
> > >  	struct net_device *ndev = NULL;
> > > -	struct ib_sa_multicast ib;
> > > +	struct ib_sa_multicast ib = {};
> > >  	enum ib_gid_type gid_type;
> > >  	bool send_only;
> > 
> > We shouldn't be able to join anything except a RDMA_PS_UDP to a
> > multicast in the first place:
> > 
> > 	if (id_priv->id.ps == RDMA_PS_UDP)
> > 		ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > 
> > Multicast RC/etc is meaningless. So I guess it should be like this:
> 
> I don't know, I used 0 exactly like we have for cma_join_ib_multicast().
> 
> Where can I read about this PS limitation? I didn't find anything
> relevant in the IBTA spec.

It is a Linux thing

We should probably check the PS even earlier to prevent the IB side
from having the same issue.

multicast should never be used in any place that can omit a qkey,
IIRC..

Jason

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Leon Romanovsky]

On Mon, Jan 10, 2022 at 11:36:19AM -0400, Jason Gunthorpe wrote:
> On Sun, Jan 09, 2022 at 02:22:04PM +0200, Leon Romanovsky wrote:
> > On Thu, Jan 06, 2022 at 01:39:41PM -0400, Jason Gunthorpe wrote:
> > > On Thu, Jan 06, 2022 at 03:15:07PM +0200, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > 
> > > > The ib->rec.qkey field is accessed without being initialized.
> > > > Clear the ib_sa_multicast struct to fix the following syzkaller error/.
> > > > 
> > > > =====================================================
> > > > BUG: KMSAN: uninit-value in cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> > > > BUG: KMSAN: uninit-value in cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
> > > >  cma_set_qkey drivers/infiniband/core/cma.c:510 [inline]
> > > >  cma_make_mc_event+0xb73/0xe00 drivers/infiniband/core/cma.c:4570
> > > >  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4782 [inline]
> > > >  rdma_join_multicast+0x2b83/0x30a0 drivers/infiniband/core/cma.c:4814
> > > >  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> > > >  ucma_join_multicast+0x1e3/0x250 drivers/infiniband/core/ucma.c:1546
> > > >  ucma_write+0x639/0x6d0 drivers/infiniband/core/ucma.c:1732
> > > >  vfs_write+0x8ce/0x2030 fs/read_write.c:588
> > > >  ksys_write+0x28c/0x520 fs/read_write.c:643
> > > >  __do_sys_write fs/read_write.c:655 [inline]
> > > >  __se_sys_write fs/read_write.c:652 [inline]
> > > >  __ia32_sys_write+0xdb/0x120 fs/read_write.c:652
> > > >  do_syscall_32_irqs_on arch/x86/entry/common.c:114 [inline]
> > > >  __do_fast_syscall_32+0x96/0xf0 arch/x86/entry/common.c:180
> > > >  do_fast_syscall_32+0x34/0x70 arch/x86/entry/common.c:205
> > > >  do_SYSENTER_32+0x1b/0x20 arch/x86/entry/common.c:248
> > > >  entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
> > > > 
> > > > Local variable ib.i created at:
> > > >  cma_iboe_join_multicast drivers/infiniband/core/cma.c:4737 [inline]
> > > >  rdma_join_multicast+0x586/0x30a0 drivers/infiniband/core/cma.c:4814
> > > >  ucma_process_join+0xa76/0xf60 drivers/infiniband/core/ucma.c:1479
> > > > 
> > > > CPU: 0 PID: 29874 Comm: syz-executor.3 Not tainted 5.16.0-rc3-syzkaller #0
> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > > =====================================================
> > > > 
> > > > Fixes: b5de0c60cc30 ("RDMA/cma: Fix use after free race in roce multicast join")
> > > > Reported-by: syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com
> > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > > >  drivers/infiniband/core/cma.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > > > index 69c9a12dd14e..9c53e1e7de50 100644
> > > > +++ b/drivers/infiniband/core/cma.c
> > > > @@ -4737,7 +4737,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> > > >  	int err = 0;
> > > >  	struct sockaddr *addr = (struct sockaddr *)&mc->addr;
> > > >  	struct net_device *ndev = NULL;
> > > > -	struct ib_sa_multicast ib;
> > > > +	struct ib_sa_multicast ib = {};
> > > >  	enum ib_gid_type gid_type;
> > > >  	bool send_only;
> > > 
> > > We shouldn't be able to join anything except a RDMA_PS_UDP to a
> > > multicast in the first place:
> > > 
> > > 	if (id_priv->id.ps == RDMA_PS_UDP)
> > > 		ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > > 
> > > Multicast RC/etc is meaningless. So I guess it should be like this:
> > 
> > I don't know, I used 0 exactly like we have for cma_join_ib_multicast().
> > 
> > Where can I read about this PS limitation? I didn't find anything
> > relevant in the IBTA spec.
> 
> It is a Linux thing
> 
> We should probably check the PS even earlier to prevent the IB side
> from having the same issue.

What do you think about this?

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 835ac54d4a24..0a1f008ca929 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
        if (ret)
                return ret;

-       ret = cma_set_qkey(id_priv, 0);
-       if (ret)
-               return ret;
-
        cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid);
-       rec.qkey = cpu_to_be32(id_priv->qkey);
+       rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
        rdma_addr_get_sgid(dev_addr, &rec.port_gid);
        rec.pkey = cpu_to_be16(ib_addr_get_pkey(dev_addr));
        rec.join_state = mc->join_state;
@@ -4748,8 +4744,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
        cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);

        ib.rec.pkey = cpu_to_be16(0xffff);
-       if (id_priv->id.ps == RDMA_PS_UDP)
-               ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
+       ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);

        if (dev_addr->bound_dev_if)
                ndev = dev_get_by_index(dev_addr->net, dev_addr->bound_dev_if);
@@ -4796,6 +4791,9 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr,
        if (WARN_ON(id->qp))
                return -EINVAL;

+       if (id->ps != RDMA_PS_UDP)
+               return -EINVAL;
+
        /* ULP is calling this wrong. */
        if (!id->device || (READ_ONCE(id_priv->state) != RDMA_CM_ADDR_BOUND &&
                            READ_ONCE(id_priv->state) != RDMA_CM_ADDR_RESOLVED))


> 
> multicast should never be used in any place that can omit a qkey,
> IIRC..
> 
> Jason

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Jason Gunthorpe]

On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote:

> > We should probably check the PS even earlier to prevent the IB side
> > from having the same issue.
> 
> What do you think about this?

IB is a bit different, it has a bunch of PS's that are UD compatible..

Probably what we really want here is to check/restrict the CM ID to
SIDR mode, which does have the qkey and is the only mode that makes
sense to be mixed with multicast, and then forget about port space
entirely.

It may be that port space indirectly restricts the CM ID to SIDR mode,
but the language here should be 'is in sidr mode', not some confusing
open coded port space check.

I'm also not sure of the lifecycle of the qkey, qkeys only exist in
SIDR mode so obviously anything that sets/gets a qkey should be
restriced to SIDR CM IDs..

> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 835ac54d4a24..0a1f008ca929 100644
> +++ b/drivers/infiniband/core/cma.c
> @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
>         if (ret)
>                 return ret;
> 
> -       ret = cma_set_qkey(id_priv, 0);
> -       if (ret)
> -               return ret;
> -
>         cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid);
> -       rec.qkey = cpu_to_be32(id_priv->qkey);
> +       rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);

And I'm not sure this makes sense? The UD qkey should still be
negotiated right?

Jason

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Leon Romanovsky]

On Mon, Jan 17, 2022 at 12:16:21PM -0400, Jason Gunthorpe wrote:
> On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote:
> 
> > > We should probably check the PS even earlier to prevent the IB side
> > > from having the same issue.
> > 
> > What do you think about this?
> 
> IB is a bit different, it has a bunch of PS's that are UD compatible..
> 
> Probably what we really want here is to check/restrict the CM ID to
> SIDR mode, which does have the qkey and is the only mode that makes
> sense to be mixed with multicast, and then forget about port space
> entirely.
> 
> It may be that port space indirectly restricts the CM ID to SIDR mode,
> but the language here should be 'is in sidr mode', not some confusing
> open coded port space check.
> 
> I'm also not sure of the lifecycle of the qkey, qkeys only exist in
> SIDR mode so obviously anything that sets/gets a qkey should be
> restriced to SIDR CM IDs..
> 
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 835ac54d4a24..0a1f008ca929 100644
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
> >         if (ret)
> >                 return ret;
> > 
> > -       ret = cma_set_qkey(id_priv, 0);
> > -       if (ret)
> > -               return ret;
> > -
> >         cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid);
> > -       rec.qkey = cpu_to_be32(id_priv->qkey);
> > +       rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> 
> And I'm not sure this makes sense? The UD qkey should still be
> negotiated right?

Yes, I think so, it will be changed in SIDR phase.

The original code has "cma_set_qkey(id_priv, 0)" call, that in IB case will
execute this switch anyway:
   515         switch (id_priv->id.ps) {
   516         case RDMA_PS_UDP:
   517         case RDMA_PS_IB:
   518                 id_priv->qkey = RDMA_UDP_QKEY;

The difference is that we won't store RDMA_UDP_QKEY in id_priv->qkey,
but I'm unsure that this is right.

Thanks

> 
> Jason

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Jason Gunthorpe]

On Mon, Jan 17, 2022 at 08:20:40PM +0200, Leon Romanovsky wrote:
> On Mon, Jan 17, 2022 at 12:16:21PM -0400, Jason Gunthorpe wrote:
> > On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote:
> > 
> > > > We should probably check the PS even earlier to prevent the IB side
> > > > from having the same issue.
> > > 
> > > What do you think about this?
> > 
> > IB is a bit different, it has a bunch of PS's that are UD compatible..
> > 
> > Probably what we really want here is to check/restrict the CM ID to
> > SIDR mode, which does have the qkey and is the only mode that makes
> > sense to be mixed with multicast, and then forget about port space
> > entirely.
> > 
> > It may be that port space indirectly restricts the CM ID to SIDR mode,
> > but the language here should be 'is in sidr mode', not some confusing
> > open coded port space check.
> > 
> > I'm also not sure of the lifecycle of the qkey, qkeys only exist in
> > SIDR mode so obviously anything that sets/gets a qkey should be
> > restriced to SIDR CM IDs..
> > 
> > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > > index 835ac54d4a24..0a1f008ca929 100644
> > > +++ b/drivers/infiniband/core/cma.c
> > > @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
> > >         if (ret)
> > >                 return ret;
> > > 
> > > -       ret = cma_set_qkey(id_priv, 0);
> > > -       if (ret)
> > > -               return ret;
> > > -
> > >         cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid);
> > > -       rec.qkey = cpu_to_be32(id_priv->qkey);
> > > +       rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > 
> > And I'm not sure this makes sense? The UD qkey should still be
> > negotiated right?
> 
> Yes, I think so, it will be changed in SIDR phase.
> 
> The original code has "cma_set_qkey(id_priv, 0)" call, that in IB case will
> execute this switch anyway:
>    515         switch (id_priv->id.ps) {
>    516         case RDMA_PS_UDP:
>    517         case RDMA_PS_IB:
>    518                 id_priv->qkey = RDMA_UDP_QKEY;
> 
> The difference is that we won't store RDMA_UDP_QKEY in id_priv->qkey,
> but I'm unsure that this is right.

Well the whoele cma_set_qkey() function appears to be complete
jumblied nonsense as if qkey is zero then it doesn't do anything if
the qkey was already set.

When called with 0 it is really some sort of 'make a default qkey if
the user hasn't set one already' and in that case defaulting to
RDMA_UDP_QKEY does makes some kind of sense.

The functions purposes should be split into two functions really.

So, we end up with 'make sure the cm id is in SDIR mode' then 'if the
qkey is not set, set it to a default', so that the net result is the
qkey is always set once the function returns.

Though, I'm not sure what the semantics are for qkey during SIDR
negotiation, that should be checked in the spec.

Jason

Re: [PATCH rdma-rc] RDMA/cma: Clear all multicast request fields

[Leon Romanovsky]

On Mon, Jan 17, 2022 at 02:38:32PM -0400, Jason Gunthorpe wrote:
> On Mon, Jan 17, 2022 at 08:20:40PM +0200, Leon Romanovsky wrote:
> > On Mon, Jan 17, 2022 at 12:16:21PM -0400, Jason Gunthorpe wrote:
> > > On Mon, Jan 10, 2022 at 08:05:40PM +0200, Leon Romanovsky wrote:
> > > 
> > > > > We should probably check the PS even earlier to prevent the IB side
> > > > > from having the same issue.
> > > > 
> > > > What do you think about this?
> > > 
> > > IB is a bit different, it has a bunch of PS's that are UD compatible..
> > > 
> > > Probably what we really want here is to check/restrict the CM ID to
> > > SIDR mode, which does have the qkey and is the only mode that makes
> > > sense to be mixed with multicast, and then forget about port space
> > > entirely.
> > > 
> > > It may be that port space indirectly restricts the CM ID to SIDR mode,
> > > but the language here should be 'is in sidr mode', not some confusing
> > > open coded port space check.
> > > 
> > > I'm also not sure of the lifecycle of the qkey, qkeys only exist in
> > > SIDR mode so obviously anything that sets/gets a qkey should be
> > > restriced to SIDR CM IDs..
> > > 
> > > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > > > index 835ac54d4a24..0a1f008ca929 100644
> > > > +++ b/drivers/infiniband/core/cma.c
> > > > @@ -4669,12 +4669,8 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
> > > >         if (ret)
> > > >                 return ret;
> > > > 
> > > > -       ret = cma_set_qkey(id_priv, 0);
> > > > -       if (ret)
> > > > -               return ret;
> > > > -
> > > >         cma_set_mgid(id_priv, (struct sockaddr *) &mc->addr, &rec.mgid);
> > > > -       rec.qkey = cpu_to_be32(id_priv->qkey);
> > > > +       rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> > > 
> > > And I'm not sure this makes sense? The UD qkey should still be
> > > negotiated right?
> > 
> > Yes, I think so, it will be changed in SIDR phase.
> > 
> > The original code has "cma_set_qkey(id_priv, 0)" call, that in IB case will
> > execute this switch anyway:
> >    515         switch (id_priv->id.ps) {
> >    516         case RDMA_PS_UDP:
> >    517         case RDMA_PS_IB:
> >    518                 id_priv->qkey = RDMA_UDP_QKEY;
> > 
> > The difference is that we won't store RDMA_UDP_QKEY in id_priv->qkey,
> > but I'm unsure that this is right.
> 
> Well the whoele cma_set_qkey() function appears to be complete
> jumblied nonsense as if qkey is zero then it doesn't do anything if
> the qkey was already set.
> 
> When called with 0 it is really some sort of 'make a default qkey if
> the user hasn't set one already' and in that case defaulting to
> RDMA_UDP_QKEY does makes some kind of sense.
> 
> The functions purposes should be split into two functions really.
> 
> So, we end up with 'make sure the cm id is in SDIR mode' then 'if the
> qkey is not set, set it to a default', so that the net result is the
> qkey is always set once the function returns.
> 
> Though, I'm not sure what the semantics are for qkey during SIDR
> negotiation, that should be checked in the spec.

There is no negotiation. Device simply sends its qkey to another side
and expects to get this qkey in every packet.

---------------------------------
Queue Key (Q_Key): Enforces access rights for reliable and unreliable
datagram service (RAW datagram service type not included). Administered
by the channel adapter. During communication establishment for datagram
service, nodes exchange Q_Keys for particular queue pairs and a node uses
the value it was passed for a remote QP in all packets it sends to that
remote QP. Likewise, the remote node uses the Q_Key it was provided.
Receipt of a packet with a different Q_Key than the one the node provided
to the remote queue pair means that packet is not valid and thus rejected.
-----------------------------------

Thanks

> 
> Jason