| Message ID | 20240328113233.21388-1-dkirjanov@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] RDMA/core: fix UAF in ib_get_eth_speed | expand |
On Thu, Mar 28, 2024 at 12:32 PM Denis Kirjanov <kirjanov@gmail.com> wrote: > > A call to ib_device_get_netdev from ib_get_eth_speed > may lead to a race condition while accessing a netdevice > instance since we don't hold the rtnl lock while checking > the registration state: > if (res && res->reg_state != NETREG_REGISTERED) { > > v2: unlock rtnl on error patch > What about other callers of ib_device_get_netdev() ? It seems they also could be racy. > Reported-by: syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com > Fixes: d41861942fc55 ("IB/core: Add generic function to extract IB speed from netdev") > Signed-off-by: Denis Kirjanov <dkirjanov@suse.de> > --- > drivers/infiniband/core/verbs.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > index 94a7f3b0c71c..9c09d8c328b4 100644 > --- a/drivers/infiniband/core/verbs.c > +++ b/drivers/infiniband/core/verbs.c > @@ -1976,11 +1976,13 @@ int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width) > if (rdma_port_get_link_layer(dev, port_num) != IB_LINK_LAYER_ETHERNET) > return -EINVAL; > > + rtnl_lock(); > netdev = ib_device_get_netdev(dev, port_num); > - if (!netdev) > + if (!netdev) { > + rtnl_unlock() > return -ENODEV; > + } > > - rtnl_lock(); > rc = __ethtool_get_link_ksettings(netdev, &lksettings); > rtnl_unlock(); > > -- > 2.30.2 >
On 2024-03-28 at 17:02:33, Denis Kirjanov (kirjanov@gmail.com) wrote: > A call to ib_device_get_netdev from ib_get_eth_speed > may lead to a race condition while accessing a netdevice > instance since we don't hold the rtnl lock while checking > the registration state: > if (res && res->reg_state != NETREG_REGISTERED) { > > v2: unlock rtnl on error patch > > Reported-by: syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com > Fixes: d41861942fc55 ("IB/core: Add generic function to extract IB speed from netdev") > Signed-off-by: Denis Kirjanov <dkirjanov@suse.de> > --- > drivers/infiniband/core/verbs.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > index 94a7f3b0c71c..9c09d8c328b4 100644 > --- a/drivers/infiniband/core/verbs.c > +++ b/drivers/infiniband/core/verbs.c > @@ -1976,11 +1976,13 @@ int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width) > if (rdma_port_get_link_layer(dev, port_num) != IB_LINK_LAYER_ETHERNET) > return -EINVAL; > > + rtnl_lock(); > netdev = ib_device_get_netdev(dev, port_num); ib_device_get_netdev() hold ref to dev before checking reg_state, isn't it enough ? > - if (!netdev) > + if (!netdev) { > + rtnl_unlock() > return -ENODEV; > + } > > - rtnl_lock(); > rc = __ethtool_get_link_ksettings(netdev, &lksettings); > rtnl_unlock(); > > -- > 2.30.2 >
> -----Original Message----- > From: Denis Kirjanov <kirjanov@gmail.com> > Sent: Thursday, March 28, 2024 5:03 PM > To: netdev@vger.kernel.org > Cc: edumazet@google.com; jgg@ziepe.ca; leon@kernel.org; Denis Kirjanov > <dkirjanov@suse.de>; syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com > Subject: [PATCH v2 net] RDMA/core: fix UAF in ib_get_eth_speed > > A call to ib_device_get_netdev from ib_get_eth_speed may lead to a race > condition while accessing a netdevice instance since we don't hold the rtnl lock > while checking the registration state: > if (res && res->reg_state != NETREG_REGISTERED) { > > v2: unlock rtnl on error patch > > Reported-by: syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com > Fixes: d41861942fc55 ("IB/core: Add generic function to extract IB speed from > netdev") > Signed-off-by: Denis Kirjanov <dkirjanov@suse.de> > --- > drivers/infiniband/core/verbs.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index > 94a7f3b0c71c..9c09d8c328b4 100644 > --- a/drivers/infiniband/core/verbs.c > +++ b/drivers/infiniband/core/verbs.c > @@ -1976,11 +1976,13 @@ int ib_get_eth_speed(struct ib_device *dev, u32 > port_num, u16 *speed, u8 *width) > if (rdma_port_get_link_layer(dev, port_num) != > IB_LINK_LAYER_ETHERNET) > return -EINVAL; > > + rtnl_lock(); > netdev = ib_device_get_netdev(dev, port_num); > - if (!netdev) > + if (!netdev) { > + rtnl_unlock() > return -ENODEV; > + } > > - rtnl_lock(); > rc = __ethtool_get_link_ksettings(netdev, &lksettings); > rtnl_unlock(); > > -- > 2.30.2 > Reviewed-by: Naveen Mamindlapalli <naveenm@marvell.com>
On 3/28/24 14:51, Eric Dumazet wrote: > On Thu, Mar 28, 2024 at 12:32 PM Denis Kirjanov <kirjanov@gmail.com> wrote: >> >> A call to ib_device_get_netdev from ib_get_eth_speed >> may lead to a race condition while accessing a netdevice >> instance since we don't hold the rtnl lock while checking >> the registration state: >> if (res && res->reg_state != NETREG_REGISTERED) { >> >> v2: unlock rtnl on error patch >> > > What about other callers of ib_device_get_netdev() ? I'll add ASSERT_RTNL() to ib_device_get_netdev and update the remaining callers. Thanks! > > It seems they also could be racy. > >> Reported-by: syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com >> Fixes: d41861942fc55 ("IB/core: Add generic function to extract IB speed from netdev") >> Signed-off-by: Denis Kirjanov <dkirjanov@suse.de> >> --- >> drivers/infiniband/core/verbs.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c >> index 94a7f3b0c71c..9c09d8c328b4 100644 >> --- a/drivers/infiniband/core/verbs.c >> +++ b/drivers/infiniband/core/verbs.c >> @@ -1976,11 +1976,13 @@ int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width) >> if (rdma_port_get_link_layer(dev, port_num) != IB_LINK_LAYER_ETHERNET) >> return -EINVAL; >> >> + rtnl_lock(); >> netdev = ib_device_get_netdev(dev, port_num); >> - if (!netdev) >> + if (!netdev) { >> + rtnl_unlock() >> return -ENODEV; >> + } >> >> - rtnl_lock(); >> rc = __ethtool_get_link_ksettings(netdev, &lksettings); >> rtnl_unlock(); >> >> -- >> 2.30.2 >>
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index 94a7f3b0c71c..9c09d8c328b4 100644 --- a/drivers/infiniband/core/verbs.c +++ b/drivers/infiniband/core/verbs.c @@ -1976,11 +1976,13 @@ int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width) if (rdma_port_get_link_layer(dev, port_num) != IB_LINK_LAYER_ETHERNET) return -EINVAL; + rtnl_lock(); netdev = ib_device_get_netdev(dev, port_num); - if (!netdev) + if (!netdev) { + rtnl_unlock() return -ENODEV; + } - rtnl_lock(); rc = __ethtool_get_link_ksettings(netdev, &lksettings); rtnl_unlock();
A call to ib_device_get_netdev from ib_get_eth_speed may lead to a race condition while accessing a netdevice instance since we don't hold the rtnl lock while checking the registration state: if (res && res->reg_state != NETREG_REGISTERED) { v2: unlock rtnl on error patch Reported-by: syzbot+5fe14f2ff4ccbace9a26@syzkaller.appspotmail.com Fixes: d41861942fc55 ("IB/core: Add generic function to extract IB speed from netdev") Signed-off-by: Denis Kirjanov <dkirjanov@suse.de> --- drivers/infiniband/core/verbs.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)