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@xxxxxxxxxx>
Cc: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx>
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
...
@@ -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
