Re: [PATCH] net: linkwatch: ignore events for unregistered netdevs

[Lukas Wunner <lukas@wunner.de>]

On Mon, Apr 25, 2022 at 07:41:46AM -0700, Jakub Kicinski wrote:
> On Sat, 23 Apr 2022 18:07:23 +0200 Lukas Wunner wrote:
> > > Looking at the original report it looks like the issue could be
> > > resolved with a more usb-specific change: e.g. it looks like
> > > usbnet_defer_kevent() is not acquiring a dev reference as it should.
> > > 
> > > Have you considered that path?
> > 
> > First of all, the diffstat of the patch shows this is an opportunity
> > to reduce LoC as well as simplify and speed up device teardown.
> > 
> > Second, the approach you're proposing won't work if a driver calls
> > netif_carrier_on/off() after unregister_netdev().
> > 
> > It seems prudent to prevent such a misbehavior in *any* driver,
> > not just usbnet.  usbnet may not be the only one doing it wrong.
> > Jann pointed out that there are more syzbot reports related
> > to a UAF in linkwatch:
> > 
> > https://lore.kernel.org/netdev/?q=__linkwatch_run_queue+syzbot
> > 
> > Third, I think an API which schedules work, invisibly to the driver,
> > is dangerous and misguided.  If it is illegal to call
> > netif_carrier_on/off() for an unregistered but not yet freed netdev,
> > catch that in core networking code and don't expect drivers to respect
> > a rule which isn't even documented.
> 
> Doesn't mean we should make it legal. We can add a warning to catch
> abuses.

It turns out that no, we *cannot* add a warning to catch abuses.

I've identified all the places in USB Ethernet drivers which are
susceptible to calling linkwatch_fire_event() after unregister_netdev(),
see patch below.

I'm fixing each one like this:

-		netif_carrier_on(dev->net);
+		dev_hold(dev->net);
+		if (dev->net->reg_state < NETREG_UNREGISTERED)
+			netif_carrier_on(dev->net);
+		dev_put(dev->net);

If this is called after unregister_netdev(), it becomes a no-op.

However if it is called concurrently to unregister_netdev(),
the reg_state may change to NETREG_UNREGISTERED after the if-clause
has been evaluated and before netif_carrier_on() is called.
Then a linkwatch event *will* be fired.  There won't be a use-after-free
because of the ref I'm acquiring here.  (unregister_netdev() will spin
in netdev_wait_allrefs_any() until the linkwatch event has been handled.)

But this means that we may still call linkwatch_fire_event() after
unregister_netdev()!  So we cannot emit a WARN splat and we cannot
catch use-after-frees outside of the USB Ethernet drivers I'm fixing
in the below patch.  It may thus very well happen that a use-after-free
may still occur for such other drivers and we cannot even WARN about it.

For this reason I would strongly prefer the $SUBJECT_PATCH ("net: linkwatch:
ignore events for unregistered netdevs") instead of the patch below.
I think you are wrong to stall the patch.  It avoids UAFs in *any*
driver, not just the USB Ethernet ones, it reduces LoC and speeds up
netdev unregistration.  What more do you want?

Thanks,

Lukas

-- >8 --

diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c
index ea06d10..279a7ca 100644
--- a/drivers/net/usb/aqc111.c
+++ b/drivers/net/usb/aqc111.c
@@ -962,7 +962,11 @@ static int aqc111_link_reset(struct usbnet *dev)
 		aqc111_write16_cmd(dev, AQ_ACCESS_MAC, SFR_RX_CTL,
 				   2, &aqc111_data->rxctl);
 
-		netif_carrier_on(dev->net);
+		dev_hold(dev->net);
+		if (dev->net->reg_state < NETREG_UNREGISTERED)
+			netif_carrier_on(dev->net);
+		dev_put(dev->net);
+
 	} else {
 		aqc111_read16_cmd(dev, AQ_ACCESS_MAC, SFR_MEDIUM_STATUS_MODE,
 				  2, &reg16);
@@ -981,7 +985,10 @@ static int aqc111_link_reset(struct usbnet *dev)
 		aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_BULK_OUT_CTRL,
 				 1, 1, &reg8);
 
-		netif_carrier_off(dev->net);
+		dev_hold(dev->net);
+		if (dev->net->reg_state < NETREG_UNREGISTERED)
+			netif_carrier_off(dev->net);
+		dev_put(dev->net);
 	}
 	return 0;
 }
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 0872ca12..1e97c0a 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -173,7 +173,11 @@ static int ax88172_link_reset(struct usbnet *dev)
 	u8 mode;
 	struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET };
 
-	mii_check_media(&dev->mii, 1, 1);
+	dev_hold(dev->net);
+	if (dev->net->reg_state < NETREG_UNREGISTERED)
+		mii_check_media(&dev->mii, 1, 1);
+	dev_put(dev->net);
+
 	mii_ethtool_gset(&dev->mii, &ecmd);
 	mode = AX88172_MEDIUM_DEFAULT;
 
@@ -1013,7 +1017,11 @@ static int ax88178_link_reset(struct usbnet *dev)
 
 	netdev_dbg(dev->net, "ax88178_link_reset()\n");
 
-	mii_check_media(&dev->mii, 1, 1);
+	dev_hold(dev->net);
+	if (dev->net->reg_state < NETREG_UNREGISTERED)
+		mii_check_media(&dev->mii, 1, 1);
+	dev_put(dev->net);
+
 	mii_ethtool_gset(&dev->mii, &ecmd);
 	mode = AX88178_MEDIUM_DEFAULT;
 	speed = ethtool_cmd_speed(&ecmd);
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index a310989..279ddf2 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1632,7 +1632,10 @@ static int ax88179_link_reset(struct usbnet *dev)
 
 	ax179_data->eee_enabled = ax88179_chk_eee(dev);
 
-	netif_carrier_on(dev->net);
+	dev_hold(dev->net);
+	if (dev->net->reg_state < NETREG_UNREGISTERED)
+		netif_carrier_on(dev->net);
+	dev_put(dev->net);
 
 	return 0;
 }
diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c
index f69d9b9..5c7904c 100644
--- a/drivers/net/usb/ch9200.c
+++ b/drivers/net/usb/ch9200.c
@@ -214,7 +214,11 @@ static int ch9200_link_reset(struct usbnet *dev)
 {
 	struct ethtool_cmd ecmd;
 
-	mii_check_media(&dev->mii, 1, 1);
+	dev_hold(dev->net);
+	if (dev->net->reg_state < NETREG_UNREGISTERED)
+		mii_check_media(&dev->mii, 1, 1);
+	dev_put(dev->net);
+
 	mii_ethtool_gset(&dev->mii, &ecmd);
 
 	netdev_dbg(dev->net, "%s() speed:%d duplex:%d\n",
diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index bb4cbe8f..9ae9359 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -427,7 +427,11 @@ static void sierra_net_handle_lsi(struct usbnet *dev, char *data,
 	} else {
 		priv->link_up = 0;
 	}
-	usbnet_link_change(dev, link_up, 0);
+
+	dev_hold(dev->net);
+	if (dev->net->reg_state < NETREG_UNREGISTERED)
+		usbnet_link_change(dev, link_up, 0);
+	dev_put(dev->net);
 }
 
 static void sierra_net_dosync(struct usbnet *dev)
@@ -758,6 +762,8 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)
 
 	dev_dbg(&dev->udev->dev, "%s", __func__);
 
+	usbnet_status_stop(dev);
+
 	/* kill the timer and work */
 	del_timer_sync(&priv->sync_timer);
 	cancel_work_sync(&priv->sierra_net_kevent);
@@ -769,8 +775,6 @@ static void sierra_net_unbind(struct usbnet *dev, struct usb_interface *intf)
 		netdev_err(dev->net,
 			"usb_control_msg failed, status %d\n", status);
 
-	usbnet_status_stop(dev);
-
 	sierra_net_set_private(dev, NULL);
 	kfree(priv);
 }
diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 95de452..b7f608a 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -640,7 +640,11 @@ static int smsc75xx_link_reset(struct usbnet *dev)
 		return ret;
 	}
 
-	mii_check_media(mii, 1, 1);
+	dev_hold(dev->net);
+	if (dev->net->reg_state < NETREG_UNREGISTERED)
+		mii_check_media(mii, 1, 1);
+	dev_put(dev->net);
+
 	mii_ethtool_gset(&dev->mii, &ecmd);
 	lcladv = smsc75xx_mdio_read(dev->net, mii->phy_id, MII_ADVERTISE);
 	rmtadv = smsc75xx_mdio_read(dev->net, mii->phy_id, MII_LPA);