On 04.08.23 14:12, Eric Dumazet wrote:
On Fri, Aug 4, 2023 at 1:28 PM Oliver Hartkopp <socketcan@xxxxxxxxxxxx> 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.
Hi Olivier
I would feel a bit safer with new longterm device references if you
could add an ro->dev_tracker and use netdev_put() netdev_hold() and
friends ;)
Thanks !
Hi Eric,
the netdev_hold() mechanic has been introduced in Linux 6.0.
Would it make sense to take this patch as fix for all the current stable
kernels and move to the dev-tracker as improvement via net-next as usual?
Best,
Oliver
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>
---
net/can/raw.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/net/can/raw.c b/net/can/raw.c
index e10f59375659..a61c417fdd27 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -283,12 +283,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);
+ dev_put(dev);
+ }
if (ro->count > 1)
kfree(ro->filter);
ro->ifindex = 0;
@@ -389,14 +391,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
+ dev_put(ro->dev);
+ } else {
raw_disable_allfilters(sock_net(sk), NULL, sk);
+ }
}
if (ro->count > 1)
kfree(ro->filter);
@@ -447,36 +451,43 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
if (dev->type != ARPHRD_CAN) {
dev_put(dev);
err = -ENODEV;
goto out;
}
- 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) {
+ dev_put(dev);
+ goto out;
+ }
+
+ ifindex = dev->ifindex;
+ if (!(dev->flags & IFF_UP))
+ notify_enetdown = 1;
} 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 */
+ dev_put(ro->dev);
+ } else {
raw_disable_allfilters(sock_net(sk), NULL, sk);
+ }
}
ro->ifindex = ifindex;
ro->bound = 1;
+ /* bind() ok -> holding a reference to new ro->dev */
ro->dev = dev;
}
out:
release_sock(sk);
--
2.39.2
