| Message ID | 20230821144547.6658-3-socketcan@hartkopp.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c275a176e4b69868576e543409927ae75e3a3288 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | CAN fixes for 6.5-rc7 | expand |
On Mon, Aug 21, 2023 at 04:45:47PM +0200, Oliver Hartkopp wrote: > Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced > a new reference to the CAN netdevice that has assigned CAN filters. > But this new ro->dev reference did not maintain its own refcount which > lead to another KASAN use-after-free splat found by Eric Dumazet. > > This patch ensures a proper refcount for the CAN nedevice. > > Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak") > Reported-by: Eric Dumazet <edumazet@google.com> > Cc: Ziyang Xuan <william.xuanziyang@huawei.com> > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> ... > @@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) > if (!dev) { > err = -ENODEV; > goto out; > } > if (dev->type != ARPHRD_CAN) { > - dev_put(dev); > err = -ENODEV; > - goto out; > + goto out_put_dev; > } > + > if (!(dev->flags & IFF_UP)) > notify_enetdown = 1; > > ifindex = dev->ifindex; > > /* filters set by default/setsockopt */ > err = raw_enable_allfilters(sock_net(sk), dev, sk); > - dev_put(dev); > + if (err) > + goto out_put_dev; > + > } else { > ifindex = 0; > > /* filters set by default/setsockopt */ > err = raw_enable_allfilters(sock_net(sk), NULL, sk); > } > > if (!err) { > if (ro->bound) { > /* unregister old filters */ > - if (ro->dev) > + if (ro->dev) { > raw_disable_allfilters(dev_net(ro->dev), > ro->dev, sk); > - else > + /* drop reference to old ro->dev */ > + netdev_put(ro->dev, &ro->dev_tracker); > + } else { > raw_disable_allfilters(sock_net(sk), NULL, sk); > + } > } > ro->ifindex = ifindex; > ro->bound = 1; > + /* bind() ok -> hold a reference for new ro->dev */ > ro->dev = dev; > + if (ro->dev) > + netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL); > } > > - out: > +out_put_dev: > + /* remove potential reference from dev_get_by_index() */ > + if (dev) > + dev_put(dev); Hi Oliver, this is possibly not worth a respin, but there is no need to check if dev is NULL before calling dev_put(), dev_put() will effectively be a no-op with a NULL argument. > +out: > release_sock(sk); > rtnl_unlock(); > > if (notify_enetdown) { > sk->sk_err = ENETDOWN; > -- > 2.39.2 > >
On 22.08.23 09:59, Simon Horman wrote: > On Mon, Aug 21, 2023 at 04:45:47PM +0200, Oliver Hartkopp wrote: >> Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced >> a new reference to the CAN netdevice that has assigned CAN filters. >> But this new ro->dev reference did not maintain its own refcount which >> lead to another KASAN use-after-free splat found by Eric Dumazet. >> >> This patch ensures a proper refcount for the CAN nedevice. >> >> Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak") >> Reported-by: Eric Dumazet <edumazet@google.com> >> Cc: Ziyang Xuan <william.xuanziyang@huawei.com> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > > ... > >> @@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) >> if (!dev) { >> err = -ENODEV; >> goto out; >> } >> if (dev->type != ARPHRD_CAN) { >> - dev_put(dev); >> err = -ENODEV; >> - goto out; >> + goto out_put_dev; >> } >> + >> if (!(dev->flags & IFF_UP)) >> notify_enetdown = 1; >> >> ifindex = dev->ifindex; >> >> /* filters set by default/setsockopt */ >> err = raw_enable_allfilters(sock_net(sk), dev, sk); >> - dev_put(dev); >> + if (err) >> + goto out_put_dev; >> + >> } else { >> ifindex = 0; >> >> /* filters set by default/setsockopt */ >> err = raw_enable_allfilters(sock_net(sk), NULL, sk); >> } >> >> if (!err) { >> if (ro->bound) { >> /* unregister old filters */ >> - if (ro->dev) >> + if (ro->dev) { >> raw_disable_allfilters(dev_net(ro->dev), >> ro->dev, sk); >> - else >> + /* drop reference to old ro->dev */ >> + netdev_put(ro->dev, &ro->dev_tracker); >> + } else { >> raw_disable_allfilters(sock_net(sk), NULL, sk); >> + } >> } >> ro->ifindex = ifindex; >> ro->bound = 1; >> + /* bind() ok -> hold a reference for new ro->dev */ >> ro->dev = dev; >> + if (ro->dev) >> + netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL); >> } >> >> - out: >> +out_put_dev: >> + /* remove potential reference from dev_get_by_index() */ >> + if (dev) >> + dev_put(dev); > > Hi Oliver, > > this is possibly not worth a respin, but there is no need to check if dev > is NULL before calling dev_put(), dev_put() will effectively be a no-op > with a NULL argument. > Hi Simon, thanks for your feedback. In fact I had in mind that someone recently removed some of these "if (dev)" statements before "dev_put(dev)" in the netdev subtree. The reason why I still wanted to point out this check is because of dev == NULL is also a valid value for CAN_RAW sockets that are not bound to a specific netdev but to 'ALL' CAN netdevs. So it was more like a documentation purpose than a programming need. As you don't see a need for a respin too, I can send a patch to can-next to remove it, if that fits for you. Best regards, Oliver >> +out: >> release_sock(sk); >> rtnl_unlock(); >> >> if (notify_enetdown) { >> sk->sk_err = ENETDOWN; >> -- >> 2.39.2 >> >> >
On Tue, Aug 22, 2023 at 06:45:25PM +0200, Oliver Hartkopp wrote: > > > On 22.08.23 09:59, Simon Horman wrote: > > On Mon, Aug 21, 2023 at 04:45:47PM +0200, Oliver Hartkopp wrote: > > > Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced > > > a new reference to the CAN netdevice that has assigned CAN filters. > > > But this new ro->dev reference did not maintain its own refcount which > > > lead to another KASAN use-after-free splat found by Eric Dumazet. > > > > > > This patch ensures a proper refcount for the CAN nedevice. > > > > > > Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak") > > > Reported-by: Eric Dumazet <edumazet@google.com> > > > Cc: Ziyang Xuan <william.xuanziyang@huawei.com> > > > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > > > > ... > > > > > @@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) > > > if (!dev) { > > > err = -ENODEV; > > > goto out; > > > } > > > if (dev->type != ARPHRD_CAN) { > > > - dev_put(dev); > > > err = -ENODEV; > > > - goto out; > > > + goto out_put_dev; > > > } > > > + > > > if (!(dev->flags & IFF_UP)) > > > notify_enetdown = 1; > > > ifindex = dev->ifindex; > > > /* filters set by default/setsockopt */ > > > err = raw_enable_allfilters(sock_net(sk), dev, sk); > > > - dev_put(dev); > > > + if (err) > > > + goto out_put_dev; > > > + > > > } else { > > > ifindex = 0; > > > /* filters set by default/setsockopt */ > > > err = raw_enable_allfilters(sock_net(sk), NULL, sk); > > > } > > > if (!err) { > > > if (ro->bound) { > > > /* unregister old filters */ > > > - if (ro->dev) > > > + if (ro->dev) { > > > raw_disable_allfilters(dev_net(ro->dev), > > > ro->dev, sk); > > > - else > > > + /* drop reference to old ro->dev */ > > > + netdev_put(ro->dev, &ro->dev_tracker); > > > + } else { > > > raw_disable_allfilters(sock_net(sk), NULL, sk); > > > + } > > > } > > > ro->ifindex = ifindex; > > > ro->bound = 1; > > > + /* bind() ok -> hold a reference for new ro->dev */ > > > ro->dev = dev; > > > + if (ro->dev) > > > + netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL); > > > } > > > - out: > > > +out_put_dev: > > > + /* remove potential reference from dev_get_by_index() */ > > > + if (dev) > > > + dev_put(dev); > > > > Hi Oliver, > > > > this is possibly not worth a respin, but there is no need to check if dev > > is NULL before calling dev_put(), dev_put() will effectively be a no-op > > with a NULL argument. > > > > Hi Simon, > > thanks for your feedback. > > In fact I had in mind that someone recently removed some of these "if (dev)" > statements before "dev_put(dev)" in the netdev subtree. > > The reason why I still wanted to point out this check is because of dev == > NULL is also a valid value for CAN_RAW sockets that are not bound to a > specific netdev but to 'ALL' CAN netdevs. > > So it was more like a documentation purpose than a programming need. > > As you don't see a need for a respin too, I can send a patch to can-next to > remove it, if that fits for you. Thanks Oliver, that would be fine by me.
diff --git a/net/can/raw.c b/net/can/raw.c index e10f59375659..d50c3f3d892f 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -83,10 +83,11 @@ struct uniqframe { struct raw_sock { struct sock sk; int bound; int ifindex; struct net_device *dev; + netdevice_tracker dev_tracker; struct list_head notifier; int loopback; int recv_own_msgs; int fd_frames; int xl_frames; @@ -283,12 +284,14 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg, switch (msg) { case NETDEV_UNREGISTER: lock_sock(sk); /* remove current filters & unregister */ - if (ro->bound) + if (ro->bound) { raw_disable_allfilters(dev_net(dev), dev, sk); + netdev_put(dev, &ro->dev_tracker); + } if (ro->count > 1) kfree(ro->filter); ro->ifindex = 0; @@ -389,14 +392,16 @@ static int raw_release(struct socket *sock) rtnl_lock(); lock_sock(sk); /* remove current filters & unregister */ if (ro->bound) { - if (ro->dev) + if (ro->dev) { raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk); - else + netdev_put(ro->dev, &ro->dev_tracker); + } else { raw_disable_allfilters(sock_net(sk), NULL, sk); + } } if (ro->count > 1) kfree(ro->filter); @@ -443,44 +448,56 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len) if (!dev) { err = -ENODEV; goto out; } if (dev->type != ARPHRD_CAN) { - dev_put(dev); err = -ENODEV; - goto out; + goto out_put_dev; } + if (!(dev->flags & IFF_UP)) notify_enetdown = 1; ifindex = dev->ifindex; /* filters set by default/setsockopt */ err = raw_enable_allfilters(sock_net(sk), dev, sk); - dev_put(dev); + if (err) + goto out_put_dev; + } else { ifindex = 0; /* filters set by default/setsockopt */ err = raw_enable_allfilters(sock_net(sk), NULL, sk); } if (!err) { if (ro->bound) { /* unregister old filters */ - if (ro->dev) + if (ro->dev) { raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk); - else + /* drop reference to old ro->dev */ + netdev_put(ro->dev, &ro->dev_tracker); + } else { raw_disable_allfilters(sock_net(sk), NULL, sk); + } } ro->ifindex = ifindex; ro->bound = 1; + /* bind() ok -> hold a reference for new ro->dev */ ro->dev = dev; + if (ro->dev) + netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL); } - out: +out_put_dev: + /* remove potential reference from dev_get_by_index() */ + if (dev) + dev_put(dev); +out: release_sock(sk); rtnl_unlock(); if (notify_enetdown) { sk->sk_err = ENETDOWN;
Commit ee8b94c8510c ("can: raw: fix receiver memory leak") introduced a new reference to the CAN netdevice that has assigned CAN filters. But this new ro->dev reference did not maintain its own refcount which lead to another KASAN use-after-free splat found by Eric Dumazet. This patch ensures a proper refcount for the CAN nedevice. Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak") Reported-by: Eric Dumazet <edumazet@google.com> Cc: Ziyang Xuan <william.xuanziyang@huawei.com> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> --- net/can/raw.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-)