diff mbox series

usbnet: Fix use-after-free on disconnect

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

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 22 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Lukas Wunner April 13, 2022, 2:16 p.m. UTC
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(-)

Comments

Oliver Neukum April 13, 2022, 6:59 p.m. UTC | #1
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
Lukas Wunner April 14, 2022, 10:58 a.m. UTC | #2
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
Greg Kroah-Hartman April 14, 2022, 11:07 a.m. UTC | #3
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
Oliver Neukum April 14, 2022, 11:20 a.m. UTC | #4
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
Lukas Wunner April 17, 2022, 7:28 a.m. UTC | #5
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 mbox series

Patch

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