| Message ID | 1549955377-15828-1-git-send-email-parav@mellanox.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,1/2] devlink: Return right error code in case of errors for region read | expand |
Hello! On 12.02.2019 10:09, Parav Pandit wrote: > While finding the devlink device during region reading, > devlink device list is accessed and devlink device is > returned without holding a lock. This could lead to user-after-free Use-after-free, perhaps? > accesses. > > While at it, add lockdep assert to ensure that all future callers hold > the lock when calling devlink_get_from_attrs(). > > Fixes: 4e54795a27f5 ("devlink: Add support for region snapshot read command") > Signed-off-by: Parav Pandit <parav@mellanox.com> > Acked-by: Jiri Pirko <jiri@mellanox.com> [...] MBR, Sergei
> -----Original Message----- > From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Sent: Tuesday, February 12, 2019 3:01 AM > To: Parav Pandit <parav@mellanox.com>; Jiri Pirko <jiri@mellanox.com>; > davem@davemloft.net; netdev@vger.kernel.org > Subject: Re: [PATCH net-next 2/2] devlink: Fix list access without lock while > reading region > > Hello! > > On 12.02.2019 10:09, Parav Pandit wrote: > > > While finding the devlink device during region reading, devlink device > > list is accessed and devlink device is returned without holding a > > lock. This could lead to user-after-free > > Use-after-free, perhaps? > Yep. Sending v1 to fix typo. > > accesses. > > > > While at it, add lockdep assert to ensure that all future callers hold > > the lock when calling devlink_get_from_attrs(). > > > > Fixes: 4e54795a27f5 ("devlink: Add support for region snapshot read > > command") > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > Acked-by: Jiri Pirko <jiri@mellanox.com> > [...] > > MBR, Sergei
diff --git a/net/core/devlink.c b/net/core/devlink.c index 8a198ba..8cd773b 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -116,6 +116,8 @@ static struct devlink *devlink_get_from_attrs(struct net *net, busname = nla_data(attrs[DEVLINK_ATTR_BUS_NAME]); devname = nla_data(attrs[DEVLINK_ATTR_DEV_NAME]); + lockdep_assert_held(&devlink_mutex); + list_for_each_entry(devlink, &devlink_list, list) { if (strcmp(devlink->dev->bus->name, busname) == 0 && strcmp(dev_name(devlink->dev), devname) == 0 && @@ -3642,13 +3644,13 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, if (err) goto out; + mutex_lock(&devlink_mutex); devlink = devlink_get_from_attrs(sock_net(cb->skb->sk), attrs); if (IS_ERR(devlink)) { err = -ENODEV; - goto out; + goto out_dev; } - mutex_lock(&devlink_mutex); mutex_lock(&devlink->lock); if (!attrs[DEVLINK_ATTR_REGION_NAME] || @@ -3723,6 +3725,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, genlmsg_cancel(skb, hdr); out_unlock: mutex_unlock(&devlink->lock); +out_dev: mutex_unlock(&devlink_mutex); out: return err;
While finding the devlink device during region reading, devlink device list is accessed and devlink device is returned without holding a lock. This could lead to user-after-free accesses. While at it, add lockdep assert to ensure that all future callers hold the lock when calling devlink_get_from_attrs(). Fixes: 4e54795a27f5 ("devlink: Add support for region snapshot read command") Signed-off-by: Parav Pandit <parav@mellanox.com> Acked-by: Jiri Pirko <jiri@mellanox.com> --- net/core/devlink.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)