Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220722103750.1938776d@kernel.org> <20220726182518.47047-1-f6bvp@free.fr>
In-Reply-To: <20220726182518.47047-1-f6bvp@free.fr>
From:   Eric Dumazet <edumazet@google.com>
Date:   Wed, 27 Jul 2022 11:06:08 +0200
Message-ID: <CANn89i+FBa-KLJz5xPvk3jO3Miww4Vs+qw4nPf_9SPwiWpyTWw@mail.gmail.com>
Subject: Re: [PATCH 1/1] [PATCH] net: rose: fix unregistered netdevice:
 waiting for rose0 to become free
To:     Bernard Pidoux <f6bvp@free.fr>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        David Miller <davem@davemloft.net>,
        Duoming Zhou <duoming@zju.edu.cn>, linux-hams@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>, Paolo Abeni <pabeni@redhat.com>,
        Ralf Baechle <ralf@linux-mips.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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++;