netdev - [PATCH] net: linkwatch: ignore events for unregistered netdevs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 17 Apr 2022 09:04:19 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Oliver Neukum <oneukum@...e.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Jann Horn <jannh@...gle.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     netdev@...r.kernel.org, linux-usb@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Jacky Chou <jackychou@...x.com.tw>, Willy Tarreau <w@....eu>,
        Lino Sanfilippo <LinoSanfilippo@....de>,
        Philipp Rosenberger <p.rosenberger@...bus.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: [PATCH] net: linkwatch: ignore events for unregistered netdevs

Jann Horn reports a use-after-free on disconnect of a USB Ethernet
(ax88179_178a.c).  Oleksij Rempel has witnessed the same issue with a
different driver (ax88172a.c).

Jann's report (linked below) explains the root cause in great detail,
but the gist is that USB Ethernet drivers call linkwatch_fire_event()
between unregister_netdev() and free_netdev().  The asynchronous work
linkwatch_event() may thus access the netdev after it's been freed.

USB Ethernet may not even be the only culprit.  To address the problem
in the most general way, ignore link events once a netdev's state has
been set to NETREG_UNREGISTERED.

That happens in netdev_run_todo() immediately before the call to
linkwatch_forget_dev().  Note that lweventlist_lock (and its implied
memory barrier) guarantees that a linkwatch_add_event() running after
linkwatch_forget_dev() will see the netdev's new state and bail out.
An unregistered netdev is therefore never added to link_watch_list
(but may have its __LINK_STATE_LINKWATCH_PENDING bit set, which should
not matter).  That obviates the need to invoke linkwatch_run_queue() in
netdev_wait_allrefs(), so drop it.

In a sense, the present commit is to *no longer* registered netdevs as
commit b47300168e77 ("net: Do not fire linkwatch events until the device
is registered.") is to *not yet* registered netdevs.

Reported-by: Jann Horn <jannh@...gle.com>
Link: https://lore.kernel.org/netdev/CAG48ez0MHBbENX5gCdHAUXZ7h7s20LnepBF-pa5M=7Bi-jZrEA@mail.gmail.com/
Reported-by: Oleksij Rempel <o.rempel@...gutronix.de>
Link: https://lore.kernel.org/netdev/20220315113841.GA22337@pengutronix.de/
Signed-off-by: Lukas Wunner <lukas@...ner.de>
Cc: stable@...r.kernel.org
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: Oliver Neukum <oneukum@...e.com>
Cc: Andrew Lunn <andrew@...n.ch>
---
 include/linux/netdevice.h |  2 --
 net/core/dev.c            | 17 -----------------
 net/core/link_watch.c     | 10 ++--------
 3 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 59e27a2b7bf0..5d950b45b59d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4805,8 +4805,6 @@ extern const struct kobj_ns_type_operations net_ns_type_operations;
 
 const char *netdev_drivername(const struct net_device *dev);
 
-void linkwatch_run_queue(void);
-
 static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
 							  netdev_features_t f2)
 {
diff --git a/net/core/dev.c b/net/core/dev.c
index 8c6c08446556..0ee56965ff76 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10140,23 +10140,6 @@ static struct net_device *netdev_wait_allrefs_any(struct list_head *list)
 			list_for_each_entry(dev, list, todo_list)
 				call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
 
-			__rtnl_unlock();
-			rcu_barrier();
-			rtnl_lock();
-
-			list_for_each_entry(dev, list, todo_list)
-				if (test_bit(__LINK_STATE_LINKWATCH_PENDING,
-					     &dev->state)) {
-					/* We must not have linkwatch events
-					 * pending on unregister. If this
-					 * happens, we simply run the queue
-					 * unscheduled, resulting in a noop
-					 * for this device.
-					 */
-					linkwatch_run_queue();
-					break;
-				}
-
 			__rtnl_unlock();
 
 			rebroadcast_time = jiffies;
diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index 95098d1a49bd..9a0ea7cd68e4 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -107,7 +107,8 @@ static void linkwatch_add_event(struct net_device *dev)
 	unsigned long flags;
 
 	spin_lock_irqsave(&lweventlist_lock, flags);
-	if (list_empty(&dev->link_watch_list)) {
+	if (list_empty(&dev->link_watch_list) &&
+	    dev->reg_state < NETREG_UNREGISTERED) {
 		list_add_tail(&dev->link_watch_list, &lweventlist);
 		dev_hold_track(dev, &dev->linkwatch_dev_tracker, GFP_ATOMIC);
 	}
@@ -250,13 +251,6 @@ void linkwatch_forget_dev(struct net_device *dev)
 }
 
 
-/* Must be called with the rtnl semaphore held */
-void linkwatch_run_queue(void)
-{
-	__linkwatch_run_queue(0);
-}
-
-
 static void linkwatch_event(struct work_struct *dummy)
 {
 	rtnl_lock();
-- 
2.35.2