| Message ID | 127121d9d933ebe3fc13f9f91cc33363d6a8a8ac.1649859147.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | usbnet: Fix use-after-free on disconnect | expand |
On 13.04.22 16:16, Lukas Wunner wrote: > 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). I see. Very good catch > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -469,6 +469,9 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb, > */ > void usbnet_defer_kevent (struct usbnet *dev, int work) > { > + if (dev->intf->condition == USB_INTERFACE_UNBINDING) > + return; But, no, you cannot do this. This is a very blatant layering violation. You cannot use states internal to usb core like that in a driver. I see two options. 1. A dedicated flag in usbnet (then please with the correct smp barriers) 2. You introduce an API to usb core to query this. Regards Oliver
On Wed, Apr 13, 2022 at 08:59:48PM +0200, Oliver Neukum wrote: > On 13.04.22 16:16, Lukas Wunner wrote: > > 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). > > I see. Very good catch > > > --- a/drivers/net/usb/usbnet.c > > +++ b/drivers/net/usb/usbnet.c > > @@ -469,6 +469,9 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb, > > */ > > void usbnet_defer_kevent (struct usbnet *dev, int work) > > { > > + if (dev->intf->condition == USB_INTERFACE_UNBINDING) > > + return; > > But, no, you cannot do this. This is a very blatant layering violation. > You cannot use states internal to usb core like that in a driver. Why do you think it's internal? enum usb_interface_condition is defined in include/linux/usb.h for everyone to see and use. If it was meant to be private, I'd expect it to be marked as such or live in drivers/usb/core/usb.h. Adding Greg to clarify. > I see two options. > 1. A dedicated flag in usbnet (then please with the correct smp barriers) > 2. You introduce an API to usb core to query this. I'd definitely prefer option 2 as I'd hate to duplicate functionality. What do you have in mind? A simple accessor to return intf->condition or something like usb_interface_unbinding() which returns a bool? Thanks, Lukas
On Thu, Apr 14, 2022 at 12:58:58PM +0200, Lukas Wunner wrote: > On Wed, Apr 13, 2022 at 08:59:48PM +0200, Oliver Neukum wrote: > > On 13.04.22 16:16, Lukas Wunner wrote: > > > 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). > > > > I see. Very good catch > > > > > --- a/drivers/net/usb/usbnet.c > > > +++ b/drivers/net/usb/usbnet.c > > > @@ -469,6 +469,9 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb, > > > */ > > > void usbnet_defer_kevent (struct usbnet *dev, int work) > > > { > > > + if (dev->intf->condition == USB_INTERFACE_UNBINDING) > > > + return; > > > > But, no, you cannot do this. This is a very blatant layering violation. > > You cannot use states internal to usb core like that in a driver. > > Why do you think it's internal? > > enum usb_interface_condition is defined in include/linux/usb.h > for everyone to see and use. If it was meant to be private, > I'd expect it to be marked as such or live in drivers/usb/core/usb.h. Because we didn't think people would do crazy things like this. > Adding Greg to clarify. Oliver is right. Also what prevents the condition from changing _right_ after you tested for it? thanks, greg k-h
On 14.04.22 12:58, Lukas Wunner wrote: > On Wed, Apr 13, 2022 at 08:59:48PM +0200, Oliver Neukum wrote: >> On 13.04.22 16:16, Lukas Wunner wrote: >> I see two options. >> 1. A dedicated flag in usbnet (then please with the correct smp barriers) >> 2. You introduce an API to usb core to query this. > I'd definitely prefer option 2 as I'd hate to duplicate functionality. Extremely valid point > What do you have in mind? A simple accessor to return intf->condition > or something like usb_interface_unbinding() which returns a bool? That is a question we also need wider and especially Greg's input. We definitely need to make sure that the interface is not already rebound, so somebody needs to look at locking. Regards Oliver
On Thu, Apr 14, 2022 at 01:07:35PM +0200, Greg Kroah-Hartman wrote: > On Thu, Apr 14, 2022 at 12:58:58PM +0200, Lukas Wunner wrote: > > On Wed, Apr 13, 2022 at 08:59:48PM +0200, Oliver Neukum wrote: > > > On 13.04.22 16:16, Lukas Wunner wrote: > > > > --- a/drivers/net/usb/usbnet.c > > > > +++ b/drivers/net/usb/usbnet.c > > > > @@ -469,6 +469,9 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb, > > > > */ > > > > void usbnet_defer_kevent (struct usbnet *dev, int work) > > > > { > > > > + if (dev->intf->condition == USB_INTERFACE_UNBINDING) > > > > + return; > > > > > > But, no, you cannot do this. This is a very blatant layering violation. > > > You cannot use states internal to usb core like that in a driver. > > > > Why do you think it's internal? > > > > enum usb_interface_condition is defined in include/linux/usb.h > > for everyone to see and use. If it was meant to be private, > > I'd expect it to be marked as such or live in drivers/usb/core/usb.h. > > Because we didn't think people would do crazy things like this. I assume "crazy things" encompasses reading and writing intf->condition without any locking or explicit memory barriers. However many drivers do that through the exported functions: usb_reset_device() usb_lock_device_for_reset() usb_driver_claim_interface() usb_driver_release_interface() In any case, I've decided to pursue a different approach which fixes the issue in core networking code rather than usbnet. USB Ethernet may not be the only culprit after all. A replacement patch superseding this one was just submitted: https://lore.kernel.org/netdev/18b3541e5372bc9b9fc733d422f4e698c089077c.1650177997.git.lukas@wunner.de Thanks, Lukas
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 9a6450f796dc..6c67ae48afeb 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -469,6 +469,9 @@ static enum skb_state defer_bh(struct usbnet *dev, struct sk_buff *skb, */ void usbnet_defer_kevent (struct usbnet *dev, int work) { + if (dev->intf->condition == USB_INTERFACE_UNBINDING) + return; + set_bit (work, &dev->flags); if (!schedule_work (&dev->kevent)) netdev_dbg(dev->net, "kevent %s may have been dropped\n", usbnet_event_names[work]); @@ -1619,11 +1622,11 @@ void usbnet_disconnect (struct usb_interface *intf) if (dev->driver_info->unbind) dev->driver_info->unbind(dev, intf); + cancel_work_sync(&dev->kevent); + net = dev->net; unregister_netdev (net); - cancel_work_sync(&dev->kevent); - usb_scuttle_anchored_urbs(&dev->deferred); usb_kill_urb(dev->interrupt);
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. Briefly, USB Ethernet drivers schedule work (usbnet_deferred_kevent()) which in turn schedules another work (linkwatch_event()). The problem is that usbnet_disconnect() first synchronizes with linkwatch_event() and only then with usbnet_deferred_kevent(). That allows usbnet_deferred_kevent() to schedule another linkwatch_event() after synchronization with the latter. In other words, scheduling happens in AB order and synchronization on disconnect happens in BA order. The correct order is to first synchronize with usbnet_deferred_kevent() (and prevent any future execution), then with linkwatch_event(), i.e. in AB order. Reported-by: Jann Horn <jannh@google.com> Link: https://lore.kernel.org/netdev/CAG48ez0MHBbENX5gCdHAUXZ7h7s20LnepBF-pa5M=7Bi-jZrEA@mail.gmail.com/ Reported-by: Oleksij Rempel <o.rempel@pengutronix.de> Link: https://lore.kernel.org/netdev/20220315113841.GA22337@pengutronix.de/ Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: stable@vger.kernel.org Cc: Oliver Neukum <oneukum@suse.com> Cc: Andrew Lunn <andrew@lunn.ch> --- drivers/net/usb/usbnet.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)