| Message ID | 20240328133542.28572-1-dkirjanov@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,net] Subject: [PATCH] RDMA/core: fix UAF with ib_device_get_netdev() | expand |
On Thu, Mar 28, 2024 at 2:36 PM Denis Kirjanov <kirjanov@gmail.com> wrote: > > A call to ib_device_get_netdev 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 path > v3: update remaining callers of ib_device_get_netdev > > 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/cache.c | 2 ++ > drivers/infiniband/core/device.c | 15 ++++++++++++--- > drivers/infiniband/core/nldev.c | 2 ++ > drivers/infiniband/core/verbs.c | 6 ++++-- > 4 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c > index c02a96d3572a..cf9c826cd520 100644 > --- a/drivers/infiniband/core/cache.c > +++ b/drivers/infiniband/core/cache.c > @@ -1461,7 +1461,9 @@ static int config_non_roce_gid_cache(struct ib_device *device, > if (rdma_protocol_iwarp(device, port)) { > struct net_device *ndev; > > + rtnl_lock(); > ndev = ib_device_get_netdev(device, port); > + rtnl_unlock(); > if (!ndev) > continue; > RCU_INIT_POINTER(gid_attr.ndev, ndev); > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c > index 07cb6c5ffda0..53074a4b04c9 100644 > --- a/drivers/infiniband/core/device.c > +++ b/drivers/infiniband/core/device.c > @@ -2026,9 +2026,12 @@ static int iw_query_port(struct ib_device *device, > > memset(port_attr, 0, sizeof(*port_attr)); > > + rtnl_lock(); > netdev = ib_device_get_netdev(device, port_num); > - if (!netdev) > + if (!netdev) { > + rtnl_unlock(); > return -ENODEV; > + } > > port_attr->max_mtu = IB_MTU_4096; > port_attr->active_mtu = ib_mtu_int_to_enum(netdev->mtu); > @@ -2052,6 +2055,7 @@ static int iw_query_port(struct ib_device *device, > rcu_read_unlock(); > } > > + rtnl_unlock(); > dev_put(netdev); > return device->ops.query_port(device, port_num, port_attr); > } > @@ -2220,6 +2224,8 @@ struct net_device *ib_device_get_netdev(struct ib_device *ib_dev, > struct ib_port_data *pdata; > struct net_device *res; > > + ASSERT_RTNL(); > + > if (!rdma_is_port_valid(ib_dev, port)) > return NULL; > > @@ -2306,12 +2312,15 @@ void ib_enum_roce_netdev(struct ib_device *ib_dev, > > rdma_for_each_port (ib_dev, port) > if (rdma_protocol_roce(ib_dev, port)) { > - struct net_device *idev = > - ib_device_get_netdev(ib_dev, port); > + struct net_device *idev; > + > + rtnl_lock(); > + idev = ib_device_get_netdev(ib_dev, port); > > if (filter(ib_dev, port, idev, filter_cookie)) > cb(ib_dev, port, idev, cookie); > > + rtnl_unlock(); > if (idev) > dev_put(idev); > } > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > index 4900a0848124..cfa204a224f2 100644 > --- a/drivers/infiniband/core/nldev.c > +++ b/drivers/infiniband/core/nldev.c > @@ -360,6 +360,7 @@ static int fill_port_info(struct sk_buff *msg, > if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, attr.phys_state)) > return -EMSGSIZE; > > + rtnl_lock(); I am guessing rtnl is already held here. Please double check all paths you are adding rtnl if this is not going to deadlock. > netdev = ib_device_get_netdev(device, port); > if (netdev && net_eq(dev_net(netdev), net)) { > ret = nla_put_u32(msg, > @@ -371,6 +372,7 @@ static int fill_port_info(struct sk_buff *msg, > } > > Please wait one day before sending a new version, thanks.
On 3/28/24 16:42, Eric Dumazet wrote: > On Thu, Mar 28, 2024 at 2:36 PM Denis Kirjanov <kirjanov@gmail.com> wrote: >> >> A call to ib_device_get_netdev 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 path >> v3: update remaining callers of ib_device_get_netdev >> >> 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/cache.c | 2 ++ >> drivers/infiniband/core/device.c | 15 ++++++++++++--- >> drivers/infiniband/core/nldev.c | 2 ++ >> drivers/infiniband/core/verbs.c | 6 ++++-- >> 4 files changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c >> index c02a96d3572a..cf9c826cd520 100644 >> --- a/drivers/infiniband/core/cache.c >> +++ b/drivers/infiniband/core/cache.c >> @@ -1461,7 +1461,9 @@ static int config_non_roce_gid_cache(struct ib_device *device, >> if (rdma_protocol_iwarp(device, port)) { >> struct net_device *ndev; >> >> + rtnl_lock(); >> ndev = ib_device_get_netdev(device, port); >> + rtnl_unlock(); >> if (!ndev) >> continue; >> RCU_INIT_POINTER(gid_attr.ndev, ndev); >> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c >> index 07cb6c5ffda0..53074a4b04c9 100644 >> --- a/drivers/infiniband/core/device.c >> +++ b/drivers/infiniband/core/device.c >> @@ -2026,9 +2026,12 @@ static int iw_query_port(struct ib_device *device, >> >> memset(port_attr, 0, sizeof(*port_attr)); >> >> + rtnl_lock(); >> netdev = ib_device_get_netdev(device, port_num); >> - if (!netdev) >> + if (!netdev) { >> + rtnl_unlock(); >> return -ENODEV; >> + } >> >> port_attr->max_mtu = IB_MTU_4096; >> port_attr->active_mtu = ib_mtu_int_to_enum(netdev->mtu); >> @@ -2052,6 +2055,7 @@ static int iw_query_port(struct ib_device *device, >> rcu_read_unlock(); >> } >> >> + rtnl_unlock(); >> dev_put(netdev); >> return device->ops.query_port(device, port_num, port_attr); >> } >> @@ -2220,6 +2224,8 @@ struct net_device *ib_device_get_netdev(struct ib_device *ib_dev, >> struct ib_port_data *pdata; >> struct net_device *res; >> >> + ASSERT_RTNL(); >> + >> if (!rdma_is_port_valid(ib_dev, port)) >> return NULL; >> >> @@ -2306,12 +2312,15 @@ void ib_enum_roce_netdev(struct ib_device *ib_dev, >> >> rdma_for_each_port (ib_dev, port) >> if (rdma_protocol_roce(ib_dev, port)) { >> - struct net_device *idev = >> - ib_device_get_netdev(ib_dev, port); >> + struct net_device *idev; >> + >> + rtnl_lock(); >> + idev = ib_device_get_netdev(ib_dev, port); >> >> if (filter(ib_dev, port, idev, filter_cookie)) >> cb(ib_dev, port, idev, cookie); >> >> + rtnl_unlock(); >> if (idev) >> dev_put(idev); >> } >> diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c >> index 4900a0848124..cfa204a224f2 100644 >> --- a/drivers/infiniband/core/nldev.c >> +++ b/drivers/infiniband/core/nldev.c >> @@ -360,6 +360,7 @@ static int fill_port_info(struct sk_buff *msg, >> if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, attr.phys_state)) >> return -EMSGSIZE; >> >> + rtnl_lock(); Hi Eric, the path from rdma_nl_rcv_msg doesn't hold the lock here and the added assertion confirms that: [ 145.932439] ? ib_device_get_netdev+0x1ea/0x3a0 [ib_core] [ 145.932656] ? ib_device_get_netdev+0x1ea/0x3a0 [ib_core] [ 145.932877] ? __report_bug+0x1b8/0x250 [ 145.932944] ? ib_device_get_netdev+0x1ea/0x3a0 [ib_core] [ 145.933165] ? report_bug+0xa4/0x1f0 [ 145.933185] ? ib_device_get_netdev+0x1ea/0x3a0 [ib_core] [ 145.933404] ? handle_bug+0x5e/0xc0 [ 145.933428] ? exc_invalid_op+0x25/0x70 [ 145.933450] ? asm_exc_invalid_op+0x1a/0x20 [ 145.933484] ? __warn_printk+0x16d/0x300 [ 145.933508] ? ib_device_get_netdev+0x1ea/0x3a0 [ib_core] [ 145.933733] fill_port_info+0x1c1/0x460 [ib_core] [ 145.935304] ? __pfx_fill_port_info+0x10/0x10 [ib_core] [ 145.935528] ? __build_skb_around+0x27b/0x3b0 [ 145.935570] ? skb_put+0x118/0x190 [ 145.935594] ? __nlmsg_put+0x159/0x1d0 [ 145.935625] nldev_port_get_doit+0x4f9/0x770 [ib_core] [ 145.935851] ? __pfx_nldev_port_get_doit+0x10/0x10 [ib_core] [ 145.936109] ? __lock_release+0x400/0x850 [ 145.936251] ? rdma_nl_rcv_msg+0x147/0x660 [ib_core] > > I am guessing rtnl is already held here. > > Please double check all paths you are adding rtnl if this is not going > to deadlock. > >> netdev = ib_device_get_netdev(device, port); >> if (netdev && net_eq(dev_net(netdev), net)) { >> ret = nla_put_u32(msg, >> @@ -371,6 +372,7 @@ static int fill_port_info(struct sk_buff *msg, >> } >> >> > > Please wait one day before sending a new version, thanks. >
On Thu, Mar 28, 2024 at 09:35:42AM -0400, Denis Kirjanov wrote: > A call to ib_device_get_netdev 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 path > v3: update remaining callers of ib_device_get_netdev > > 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/cache.c | 2 ++ > drivers/infiniband/core/device.c | 15 ++++++++++++--- > drivers/infiniband/core/nldev.c | 2 ++ > drivers/infiniband/core/verbs.c | 6 ++++-- > 4 files changed, 20 insertions(+), 5 deletions(-) This is a rdma patch you need to cc linux-rdma and ensure it reaches the rdma patchworks or it will not be applied.. Thanks, Jason
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index c02a96d3572a..cf9c826cd520 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -1461,7 +1461,9 @@ static int config_non_roce_gid_cache(struct ib_device *device, if (rdma_protocol_iwarp(device, port)) { struct net_device *ndev; + rtnl_lock(); ndev = ib_device_get_netdev(device, port); + rtnl_unlock(); if (!ndev) continue; RCU_INIT_POINTER(gid_attr.ndev, ndev); diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 07cb6c5ffda0..53074a4b04c9 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -2026,9 +2026,12 @@ static int iw_query_port(struct ib_device *device, memset(port_attr, 0, sizeof(*port_attr)); + rtnl_lock(); netdev = ib_device_get_netdev(device, port_num); - if (!netdev) + if (!netdev) { + rtnl_unlock(); return -ENODEV; + } port_attr->max_mtu = IB_MTU_4096; port_attr->active_mtu = ib_mtu_int_to_enum(netdev->mtu); @@ -2052,6 +2055,7 @@ static int iw_query_port(struct ib_device *device, rcu_read_unlock(); } + rtnl_unlock(); dev_put(netdev); return device->ops.query_port(device, port_num, port_attr); } @@ -2220,6 +2224,8 @@ struct net_device *ib_device_get_netdev(struct ib_device *ib_dev, struct ib_port_data *pdata; struct net_device *res; + ASSERT_RTNL(); + if (!rdma_is_port_valid(ib_dev, port)) return NULL; @@ -2306,12 +2312,15 @@ void ib_enum_roce_netdev(struct ib_device *ib_dev, rdma_for_each_port (ib_dev, port) if (rdma_protocol_roce(ib_dev, port)) { - struct net_device *idev = - ib_device_get_netdev(ib_dev, port); + struct net_device *idev; + + rtnl_lock(); + idev = ib_device_get_netdev(ib_dev, port); if (filter(ib_dev, port, idev, filter_cookie)) cb(ib_dev, port, idev, cookie); + rtnl_unlock(); if (idev) dev_put(idev); } diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index 4900a0848124..cfa204a224f2 100644 --- a/drivers/infiniband/core/nldev.c +++ b/drivers/infiniband/core/nldev.c @@ -360,6 +360,7 @@ static int fill_port_info(struct sk_buff *msg, if (nla_put_u8(msg, RDMA_NLDEV_ATTR_PORT_PHYS_STATE, attr.phys_state)) return -EMSGSIZE; + rtnl_lock(); netdev = ib_device_get_netdev(device, port); if (netdev && net_eq(dev_net(netdev), net)) { ret = nla_put_u32(msg, @@ -371,6 +372,7 @@ static int fill_port_info(struct sk_buff *msg, } out: + rtnl_unlock(); dev_put(netdev); return ret; } diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index 94a7f3b0c71c..6a3757b00c93 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 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 path v3: update remaining callers of ib_device_get_netdev 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/cache.c | 2 ++ drivers/infiniband/core/device.c | 15 ++++++++++++--- drivers/infiniband/core/nldev.c | 2 ++ drivers/infiniband/core/verbs.c | 6 ++++-- 4 files changed, 20 insertions(+), 5 deletions(-)