Re: [PATCH 1/1] [PATCH] net: rose: fix unregistered netdevice: waiting for rose0 to become free

From: Eric Dumazet
Date: Wed Jul 27 2022 - 05:07:07 EST


On Tue, Jul 26, 2022 at 8:25 PM Bernard Pidoux <f6bvp@xxxxxxx> 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++;