Messages in this thread Patch in this message | | | From | Eric Dumazet <> | Date | Wed, 27 Jul 2022 11:06:08 +0200 | Subject | Re: [PATCH 1/1] [PATCH] net: rose: fix unregistered netdevice: waiting for rose0 to become free |
| |
On Tue, Jul 26, 2022 at 8:25 PM Bernard Pidoux <f6bvp@free.fr> wrote: > > Here is the context. > > This patch adds dev_put(dev) in order to allow removal of rose module > after use of AX25 and ROSE via rose0 device. > > Otherwise when trying to remove rose module via rmmod rose an infinite > loop message was displayed on all consoles with xx being a random number. > > unregistered_netdevice: waiting for rose0 to become free. Usage count = xx > > unregistered_netdevice: waiting for rose0 to become free. Usage count = xx > > ... > > With the patch it is ok to rmmod rose.
But removing a net device will leave a dangling pointer, leading to UAF.
We must keep a reference and remove it when the socket is dismantled.
Also rose_dev_first() is buggy, because it leaves the rcu section without taking first a reference on the found device.
Here is a probably not complete patch, can you give it a try ?
(Also enable CONFIG_NET_DEV_REFCNT_TRACKER=y in your .config to ease debugging)
(I can send you privately the patch, just ask me, I include it inline here for clarity only)
Thanks.
diff --git a/include/net/rose.h b/include/net/rose.h index 0f0a4ce0fee7cc5e125507a8fc3cfb8cb826be73..64f808eed0e15a2482e8ce010d712eef1e0b9d85 100644 --- a/include/net/rose.h +++ b/include/net/rose.h @@ -131,7 +131,8 @@ struct rose_sock { ax25_address source_digis[ROSE_MAX_DIGIS]; ax25_address dest_digis[ROSE_MAX_DIGIS]; struct rose_neigh *neighbour; - struct net_device *device; + struct net_device *device; + netdevice_tracker dev_tracker; unsigned int lci, rand; unsigned char state, condition, qbitincl, defer; unsigned char cause, diagnostic; diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c index bf2d986a6bc392a9d830b1dfa7fbaa3bca969aa3..520a48999f1bf8a41d66e8a4f86606b66f2b9408 100644 --- a/net/rose/af_rose.c +++ b/net/rose/af_rose.c @@ -192,6 +192,7 @@ static void rose_kill_by_device(struct net_device *dev) rose_disconnect(s, ENETUNREACH, ROSE_OUT_OF_ORDER, 0); if (rose->neighbour) rose->neighbour->use--; + dev_put_track(rose->device, &rose->dev_tracker); rose->device = NULL; } } @@ -592,6 +593,8 @@ static struct sock *rose_make_new(struct sock *osk) rose->idle = orose->idle; rose->defer = orose->defer; rose->device = orose->device; + if (rose->device) + dev_hold_track(rose->device, &rose->dev_tracker, GFP_ATOMIC); rose->qbitincl = orose->qbitincl;
return sk; @@ -695,7 +698,11 @@ static int rose_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) }
rose->source_addr = addr->srose_addr; + // TODO: should probably hold socket lock at this point ? + WARN_ON_ONCE(rose->device); rose->device = dev; + netdev_tracker_alloc(rose->device, &rose->dev_tracker, GFP_KERNEL); + rose->source_ndigis = addr->srose_ndigis;
if (addr_len == sizeof(struct full_sockaddr_rose)) { @@ -721,7 +728,6 @@ static int rose_connect(struct socket *sock, struct sockaddr *uaddr, int addr_le struct rose_sock *rose = rose_sk(sk); struct sockaddr_rose *addr = (struct sockaddr_rose *)uaddr; unsigned char cause, diagnostic; - struct net_device *dev; ax25_uid_assoc *user; int n, err = 0;
@@ -778,9 +784,12 @@ static int rose_connect(struct socket *sock, struct sockaddr *uaddr, int addr_le }
if (sock_flag(sk, SOCK_ZAPPED)) { /* Must bind first - autobinding in this may or may not work */ + struct net_device *dev; + sock_reset_flag(sk, SOCK_ZAPPED);
- if ((dev = rose_dev_first()) == NULL) { + dev = rose_dev_first(); + if (!dev) { err = -ENETUNREACH; goto out_release; } @@ -788,12 +797,15 @@ static int rose_connect(struct socket *sock, struct sockaddr *uaddr, int addr_le user = ax25_findbyuid(current_euid()); if (!user) { err = -EINVAL; + dev_put(dev); goto out_release; }
memcpy(&rose->source_addr, dev->dev_addr, ROSE_ADDR_LEN); rose->source_call = user->call; rose->device = dev; + netdev_tracker_alloc(rose->device, &rose->dev_tracker, + GFP_KERNEL); ax25_uid_put(user);
rose_insert_socket(sk); /* Finish the bind */ @@ -1017,6 +1029,7 @@ int rose_rx_call_request(struct sk_buff *skb, struct net_device *dev, struct ros make_rose->source_digis[n] = facilities.source_digis[n]; make_rose->neighbour = neigh; make_rose->device = dev; + dev_hold_track(make_rose->device, &make_rose->dev_tracker, GFP_ATOMIC); make_rose->facilities = facilities;
make_rose->neighbour->use++;
| |