| 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
| ||
|
Date: Sat, 30 Apr 2022 12:05:41 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Paolo Abeni <pabeni@...hat.com>, Oliver Neukum <oneukum@...e.com>,
"David S. Miller" <davem@...emloft.net>,
Jann Horn <jannh@...gle.com>,
Oleksij Rempel <o.rempel@...gutronix.de>,
Eric Dumazet <edumazet@...gle.com>, 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: Re: [PATCH] net: linkwatch: ignore events for unregistered netdevs
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, ®16);
@@ -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, ®8);
- 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);
Powered by blists - more mailing lists