[PATCH] usb: core: Null deref in kernel with USB webcams.

[PATCH] usb: core: Null deref in kernel with USB webcams.

[John Boero]

From 54f9886454e9a28e8d943c1cef15df9c11555df7 Mon Sep 17 00:00:00 2001
From: JohnnyB <jboero@users.noreply.github.com>
Date: Thu, 12 Nov 2020 15:28:29 +0000
Subject: [PATCH] usb: core: Null deref in kernel with USB webcams.

Fixes: Ubuntu Launchpad bug 1827452

This is my first attempt at a kernel contribution so sorry if sloppy.

There is some kind of race condition affecting Logitech
webcams that crash USB with a null dereference.
Affects raspberry pi devices as well as x86.
No check on dev before dereference.
Simple fix for issue experienced for months in
both x86 and arm/rpi environments.

Signed-off-by: John Boero <boeroboy@gmail.com>

---
drivers/usb/core/usb.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index d8756ffe513a..9b4ac4415f1a 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -272,13 +272,9 @@ EXPORT_SYMBOL_GPL(usb_find_alt_setting);
struct usb_interface *usb_ifnum_to_if(const struct usb_device *dev,
                                     unsigned ifnum)
{
-       struct usb_host_config *config = NULL;
+       struct usb_host_config *config = dev->actconfig;
       int i;

-       if (!dev)
-               return NULL;
-
-       config = dev->actconfig;
       if (!config)
               return NULL;
       for (i = 0; i < config->desc.bNumInterfaces; i++)
--
2.26.2

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[John Boero]

Oh for pete's sake.  That patch is backwards.  Sorry.
John

On Thu, Nov 12, 2020 at 3:52 PM John Boero <boeroboy@gmail.com> wrote:
>
> From 54f9886454e9a28e8d943c1cef15df9c11555df7 Mon Sep 17 00:00:00 2001
> From: JohnnyB <jboero@users.noreply.github.com>
> Date: Thu, 12 Nov 2020 15:28:29 +0000
> Subject: [PATCH] usb: core: Null deref in kernel with USB webcams.
>
> Fixes: Ubuntu Launchpad bug 1827452
>
> This is my first attempt at a kernel contribution so sorry if sloppy.
>
> There is some kind of race condition affecting Logitech
> webcams that crash USB with a null dereference.
> Affects raspberry pi devices as well as x86.
> No check on dev before dereference.
> Simple fix for issue experienced for months in
> both x86 and arm/rpi environments.
>
> Signed-off-by: John Boero <boeroboy@gmail.com>
>
> ---
> drivers/usb/core/usb.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index d8756ffe513a..9b4ac4415f1a 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -272,13 +272,9 @@ EXPORT_SYMBOL_GPL(usb_find_alt_setting);
> struct usb_interface *usb_ifnum_to_if(const struct usb_device *dev,
>                                      unsigned ifnum)
> {
> -       struct usb_host_config *config = NULL;
> +       struct usb_host_config *config = dev->actconfig;
>        int i;
>
> -       if (!dev)
> -               return NULL;
> -
> -       config = dev->actconfig;
>        if (!config)
>                return NULL;
>        for (i = 0; i < config->desc.bNumInterfaces; i++)
> --
> 2.26.2

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[Greg Kroah-Hartman]

On Thu, Nov 12, 2020 at 03:52:02PM +0000, John Boero wrote:
> >From 54f9886454e9a28e8d943c1cef15df9c11555df7 Mon Sep 17 00:00:00 2001
> From: JohnnyB <jboero@users.noreply.github.com>

Why all this header here?

And the from: line doesn't match your Signed-off-by: line :(

> Date: Thu, 12 Nov 2020 15:28:29 +0000
> Subject: [PATCH] usb: core: Null deref in kernel with USB webcams.
> 
> Fixes: Ubuntu Launchpad bug 1827452
> 
> This is my first attempt at a kernel contribution so sorry if sloppy.

No need to put this in the changelog text and have it be in the kernel
for foever :)

> 
> There is some kind of race condition affecting Logitech
> webcams that crash USB with a null dereference.
> Affects raspberry pi devices as well as x86.
> No check on dev before dereference.
> Simple fix for issue experienced for months in
> both x86 and arm/rpi environments.
> 
> Signed-off-by: John Boero <boeroboy@gmail.com>
> 
> ---
> drivers/usb/core/usb.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index d8756ffe513a..9b4ac4415f1a 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -272,13 +272,9 @@ EXPORT_SYMBOL_GPL(usb_find_alt_setting);
> struct usb_interface *usb_ifnum_to_if(const struct usb_device *dev,
>                                      unsigned ifnum)
> {
> -       struct usb_host_config *config = NULL;
> +       struct usb_host_config *config = dev->actconfig;
>        int i;
> 
> -       if (!dev)
> -               return NULL;
> -
> -       config = dev->actconfig;
>        if (!config)
>                return NULL;
>        for (i = 0; i < config->desc.bNumInterfaces; i++)

This patch is corrupted and can not be applied, but also, it looks
backwards, right?

And how about we find the race condition and fix that instead of trying
to paper over it here?

thanks,

greg k-h

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[John Boero]

Sorry header was generated by git email and I should have
paid closer attention to it before sending.
Long time listener, first time caller.

Yes the patch is backwards sorry.  Testing alt proposal from
stern@rowland.harvard.edu.  It may be a buggy driver
but it would be nice if a buggy driver couldn't bring down
the entire usb core. lsusb hangs until reboot or reset of usb.

It seems to behave fine on first use.  Run Zoom or cheese
works fine first time.  Subsequent runs, no device found
and usb is crashed with trace in dmesg.

Thanks
John


On Thu, Nov 12, 2020 at 5:04 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Nov 12, 2020 at 03:52:02PM +0000, John Boero wrote:
> > >From 54f9886454e9a28e8d943c1cef15df9c11555df7 Mon Sep 17 00:00:00 2001
> > From: JohnnyB <jboero@users.noreply.github.com>
>
> Why all this header here?
>
> And the from: line doesn't match your Signed-off-by: line :(
>
> > Date: Thu, 12 Nov 2020 15:28:29 +0000
> > Subject: [PATCH] usb: core: Null deref in kernel with USB webcams.
> >
> > Fixes: Ubuntu Launchpad bug 1827452
> >
> > This is my first attempt at a kernel contribution so sorry if sloppy.
>
> No need to put this in the changelog text and have it be in the kernel
> for foever :)
>
> >
> > There is some kind of race condition affecting Logitech
> > webcams that crash USB with a null dereference.
> > Affects raspberry pi devices as well as x86.
> > No check on dev before dereference.
> > Simple fix for issue experienced for months in
> > both x86 and arm/rpi environments.
> >
> > Signed-off-by: John Boero <boeroboy@gmail.com>
> >
> > ---
> > drivers/usb/core/usb.c | 6 +-----
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index d8756ffe513a..9b4ac4415f1a 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -272,13 +272,9 @@ EXPORT_SYMBOL_GPL(usb_find_alt_setting);
> > struct usb_interface *usb_ifnum_to_if(const struct usb_device *dev,
> >                                      unsigned ifnum)
> > {
> > -       struct usb_host_config *config = NULL;
> > +       struct usb_host_config *config = dev->actconfig;
> >        int i;
> >
> > -       if (!dev)
> > -               return NULL;
> > -
> > -       config = dev->actconfig;
> >        if (!config)
> >                return NULL;
> >        for (i = 0; i < config->desc.bNumInterfaces; i++)
>
> This patch is corrupted and can not be applied, but also, it looks
> backwards, right?
>
> And how about we find the race condition and fix that instead of trying
> to paper over it here?
>
> thanks,
>
> greg k-h

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[Greg Kroah-Hartman]

On Thu, Nov 12, 2020 at 05:13:30PM +0000, John Boero wrote:
> Yes the patch is backwards sorry.  Testing alt proposal from
> stern@rowland.harvard.edu.  It may be a buggy driver
> but it would be nice if a buggy driver couldn't bring down
> the entire usb core. lsusb hangs until reboot or reset of usb.

buggy kernel drivers can do anything, including deleting the contents of
your disk.  We don't protect the kernel from itself, sorry :)

good luck!

greg k-h

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[John Boero]

Then why does line 278 right below it check for NULL?

On Thu, Nov 12, 2020 at 5:56 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Nov 12, 2020 at 05:13:30PM +0000, John Boero wrote:
> > Yes the patch is backwards sorry.  Testing alt proposal from
> > stern@rowland.harvard.edu.  It may be a buggy driver
> > but it would be nice if a buggy driver couldn't bring down
> > the entire usb core. lsusb hangs until reboot or reset of usb.
>
> buggy kernel drivers can do anything, including deleting the contents of
> your disk.  We don't protect the kernel from itself, sorry :)
>
> good luck!
>
> greg k-h

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[Greg Kroah-Hartman]

On Thu, Nov 12, 2020 at 06:15:08PM +0000, John Boero wrote:
> Then why does line 278 right below it check for NULL?

I am sorry, but there is no context here at all.  There's a reason you
don't top-post for technical discussions :)

thanks,

greg k-h

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[Alan Stern]

On Thu, Nov 12, 2020 at 06:15:08PM +0000, John Boero wrote:
> Then why does line 278 right below it check for NULL?

Are you asking about line 278 in drivers/usb/core/usb.c?  The statement 
which says:

	if (!config)
		return NULL;

This is because it is perfectly valid for config to be NULL at this 
point.  But it is not valid for dev to be NULL.  If dev is NULL then 
there is a bug in the caller.

Alan Stern

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[John Boero]

Thanks for the tips.

I've spent some more time on this this morning.
It looks like it's not the dev after all.
Every interface in the dev is set NULL after init.

Just like in the original Ubuntu bug 1827452 filed by someone else
the device seems to disconnect itself after uvcvideo initialization.
Then there is a 5 second pause before usb_ifnum_to_if tries
to iterate through its 8 interfaces - all of which are null.
It looks like uvc properly locks the dev, so maybe this could
be caused by any device being unplugged after init?

The WARNING handle preserves USB function though,
and subsequent lsusb behaves fine:

$ lsusb | fold -w 80
Bus 002 Device 002: ID 8087:8002 Intel Corp. 8 channel internal hub
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 002: ID 8087:800a Intel Corp. Hub
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 004 Device 002: ID 0451:8140 Texas Instruments, Inc. TUSB8041 4-Port Hub
Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 003 Device 005: ID 1ea7:0064 SHARKOON Technologies GmbH 2.4GHz Wireless rech
argeable vertical mouse [More&Better]
Bus 003 Device 004: ID 145f:025c Trust Trust USB Microphone
Bus 003 Device 002: ID 1050:0407 Yubico.com Yubikey 4/5 OTP+U2F+CCID
Bus 003 Device 009: ID 0a5c:21e8 Broadcom Corp. BCM20702A0 Bluetooth 4.0
Bus 003 Device 008: ID 0451:8142 Texas Instruments, Inc. TUSB8041 4-Port Hub
Bus 003 Device 006: ID 062a:4101 MosArt Semiconductor Corp. Wireless Keyboard/Mo
use
Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

8x (0-7) occurences of the following WARNING:

[ 140.678756] usb 3-4: USB disconnect, device number 3
[ 145.995855] ------------[ cut here ]------------
[ 145.995863] dev interface is NULL in usb_ifnum_to_if
[ 145.995907] WARNING: CPU: 31 PID: 5617 at drivers/usb/core/usb.c:289
usb_ifnum_to_if+0x58/0x80

On Thu, Nov 12, 2020 at 7:25 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Nov 12, 2020 at 06:15:08PM +0000, John Boero wrote:
> > Then why does line 278 right below it check for NULL?
>
> Are you asking about line 278 in drivers/usb/core/usb.c?  The statement
> which says:
>
>         if (!config)
>                 return NULL;
>
> This is because it is perfectly valid for config to be NULL at this
> point.  But it is not valid for dev to be NULL.  If dev is NULL then
> there is a bug in the caller.
>
> Alan Stern

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[Alan Stern]

On Fri, Nov 13, 2020 at 01:18:05PM +0000, John Boero wrote:
> Thanks for the tips.
> 
> I've spent some more time on this this morning.
> It looks like it's not the dev after all.

What isn't the dev?

> Every interface in the dev is set NULL after init.

I can't tell what this means.  Please be more explicit.

> Just like in the original Ubuntu bug 1827452 filed by someone else
> the device seems to disconnect itself after uvcvideo initialization.
> Then there is a 5 second pause before usb_ifnum_to_if tries
> to iterate through its 8 interfaces - all of which are null.
> It looks like uvc properly locks the dev, so maybe this could
> be caused by any device being unplugged after init?

More likely there is a bug in the uvcvideo driver.

> The WARNING handle preserves USB function though,
> and subsequent lsusb behaves fine:

No, the WARN only writes a message to the system log.  The "return" 
statement is what prevented the system from crashing.

> $ lsusb | fold -w 80
> Bus 002 Device 002: ID 8087:8002 Intel Corp. 8 channel internal hub
> Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 001 Device 002: ID 8087:800a Intel Corp. Hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 004 Device 002: ID 0451:8140 Texas Instruments, Inc. TUSB8041 4-Port Hub
> Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 003 Device 005: ID 1ea7:0064 SHARKOON Technologies GmbH 2.4GHz Wireless rech
> argeable vertical mouse [More&Better]
> Bus 003 Device 004: ID 145f:025c Trust Trust USB Microphone
> Bus 003 Device 002: ID 1050:0407 Yubico.com Yubikey 4/5 OTP+U2F+CCID
> Bus 003 Device 009: ID 0a5c:21e8 Broadcom Corp. BCM20702A0 Bluetooth 4.0
> Bus 003 Device 008: ID 0451:8142 Texas Instruments, Inc. TUSB8041 4-Port Hub
> Bus 003 Device 006: ID 062a:4101 MosArt Semiconductor Corp. Wireless Keyboard/Mo
> use
> Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> 
> 8x (0-7) occurences of the following WARNING:
> 
> [ 140.678756] usb 3-4: USB disconnect, device number 3
> [ 145.995855] ------------[ cut here ]------------
> [ 145.995863] dev interface is NULL in usb_ifnum_to_if
> [ 145.995907] WARNING: CPU: 31 PID: 5617 at drivers/usb/core/usb.c:289
> usb_ifnum_to_if+0x58/0x80

You removed the most important part of the log message!  What appears 
below this point?

In fact, you should just post the entire log (or put it on a server 
somewhere and post a URL).

Alan Stern

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[John Boero]

Sorry I wanted to include a pastebin or link but was trying to follow maillist
guidelines and not include links or exceed wrap guidelines.  Full contents:
https://paste.centos.org/view/3746bc40

Yes I understand the return dodges the config dereference.

Original line usb.c:281 is the original error:

280| for (i = 0; i < config->desc.bNumInterfaces; i++)
281|  if (config->interface[i]->altsetting[0]
282|    .desc.bInterfaceNumber == ifnum)
283|  return config->interface[i];

Thanks
John

On Fri, Nov 13, 2020 at 4:34 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, Nov 13, 2020 at 01:18:05PM +0000, John Boero wrote:
> > Thanks for the tips.
> >
> > I've spent some more time on this this morning.
> > It looks like it's not the dev after all.
>
> What isn't the dev?
>
> > Every interface in the dev is set NULL after init.
>
> I can't tell what this means.  Please be more explicit.
>
> > Just like in the original Ubuntu bug 1827452 filed by someone else
> > the device seems to disconnect itself after uvcvideo initialization.
> > Then there is a 5 second pause before usb_ifnum_to_if tries
> > to iterate through its 8 interfaces - all of which are null.
> > It looks like uvc properly locks the dev, so maybe this could
> > be caused by any device being unplugged after init?
>
> More likely there is a bug in the uvcvideo driver.
>
> > The WARNING handle preserves USB function though,
> > and subsequent lsusb behaves fine:
>
> No, the WARN only writes a message to the system log.  The "return"
> statement is what prevented the system from crashing.
>
> > $ lsusb | fold -w 80
> > Bus 002 Device 002: ID 8087:8002 Intel Corp. 8 channel internal hub
> > Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 001 Device 002: ID 8087:800a Intel Corp. Hub
> > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 004 Device 002: ID 0451:8140 Texas Instruments, Inc. TUSB8041 4-Port Hub
> > Bus 004 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > Bus 003 Device 005: ID 1ea7:0064 SHARKOON Technologies GmbH 2.4GHz Wireless rech
> > argeable vertical mouse [More&Better]
> > Bus 003 Device 004: ID 145f:025c Trust Trust USB Microphone
> > Bus 003 Device 002: ID 1050:0407 Yubico.com Yubikey 4/5 OTP+U2F+CCID
> > Bus 003 Device 009: ID 0a5c:21e8 Broadcom Corp. BCM20702A0 Bluetooth 4.0
> > Bus 003 Device 008: ID 0451:8142 Texas Instruments, Inc. TUSB8041 4-Port Hub
> > Bus 003 Device 006: ID 062a:4101 MosArt Semiconductor Corp. Wireless Keyboard/Mo
> > use
> > Bus 003 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> >
> > 8x (0-7) occurences of the following WARNING:
> >
> > [ 140.678756] usb 3-4: USB disconnect, device number 3
> > [ 145.995855] ------------[ cut here ]------------
> > [ 145.995863] dev interface is NULL in usb_ifnum_to_if
> > [ 145.995907] WARNING: CPU: 31 PID: 5617 at drivers/usb/core/usb.c:289
> > usb_ifnum_to_if+0x58/0x80
>
> You removed the most important part of the log message!  What appears
> below this point?
>
> In fact, you should just post the entire log (or put it on a server
> somewhere and post a URL).
>
> Alan Stern

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[Alan Stern]

On Fri, Nov 13, 2020 at 04:45:52PM +0000, John Boero wrote:
> Sorry I wanted to include a pastebin or link but was trying to follow maillist
> guidelines and not include links or exceed wrap guidelines.  Full contents:
> https://paste.centos.org/view/3746bc40
> 
> Yes I understand the return dodges the config dereference.
> 
> Original line usb.c:281 is the original error:
> 
> 280| for (i = 0; i < config->desc.bNumInterfaces; i++)
> 281|  if (config->interface[i]->altsetting[0]
> 282|    .desc.bInterfaceNumber == ifnum)
> 283|  return config->interface[i];

Okay.  Without having looked at the code, I would guess that uvcvideo's 
uvc_ioctl_streamon() handler -- or some routine beneath it -- either 
doesn't lock the USB interface while starting I/O, or doesn't check 
(while holding the lock) to see whether the driver has been unbound.

This sort of error (config->interface[i] == NULL) is what you expect to 
see if a driver tries to carry out I/O to a device that has been 
unplugged and that it has been unbound from.

Alan Stern

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[John Boero]

Thanks Alan
I just spent some more time investigating and playing with different patches,
locks, mutexes, and sleeps, and I think I see exactly what's happening now.
I now understand why it:
A) seems to happen randomly during uvc start_stream
B) affects multiple device vendors
C) has been reported in RaspberryPi and IoT threads

I put a trace on usb/core/hub.c:usb_disconnect to identify why the device was
disconnecting and it seems this is a low power issue.  An idle webcam appears
fine to usbcore but as soon as you initialize it or uvc starts a stream, it
consumes more power, might find the cable can't supply it, and then disconnects
via interrupt. In my case I can reproduce this consistently with a cheap USB
extension cable, but this issue appears common to passive hubs, and IoT or SBCs
that don't always supply clean power over USB.  My simplified patch can at least
protect usbcore from crashing on a bad device:

From 73019d79fe4fd8b2c945005f8a067f528d8056fd Mon Sep 17 00:00:00 2001
From: jboero <boeroboy@gmail.com>
Date: Sun, 22 Nov 2020 19:30:41 +0000
Subject: [PATCH] USBCore NULL interface deref fix

---
drivers/usb/core/usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index bafc113f2b3e..da46c84c87ce 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -278,7 +278,7 @@ struct usb_interface *usb_ifnum_to_if(const struct
usb_device *dev,
       if (!config)
               return NULL;
       for (i = 0; i < config->desc.bNumInterfaces; i++)
-               if (config->interface[i]->altsetting[0]
+               if (config->interface[i] && config->interface[i]->altsetting[0]
                               .desc.bInterfaceNumber == ifnum)
                       return config->interface[i];

--
2.26.2



This protects ongoing USB functionality including lsusb, and prevents a hang on
reboot after error.  It doesn't help a user diagnose the error on the UVC side.
A fix from the uvc side is a little trickier and I'd like an opinion on how
best to handle locks in uvc_video_start_transfer. I've tried a few options
around uvcvideo.c:1874

ret = usb_set_interface(stream->dev->udev, intfnum, altsetting);

I've even used multiple msleeps and checked for NULL interfaces but that feels
like a cheap trick and I was wondering what lock solution might help best here?

Thanks!
John Boero

Trace:
[ 115.872606] uvcvideo: Device requested 2688 B/frame bandwidth.
[ 115.872610] uvcvideo: Selecting alternate setting 10 (2688 B/frame bandwidth).
[ 115.872611] uvcvideo: jboero sleeping 5ms for race condition.
[ 115.886305] ------------[ cut here ]------------
[ 115.886310] jboero dev disconnecting - 200ms sleep!
[ 115.886354] WARNING: CPU: 18 PID: 321 at drivers/usb/core/hub.c:2194
usb_disconnect+0x25/0xb0
[ 115.886359] Modules linked in: snd_seq_dummy snd_hrtimer rfcomm
rc_cec nouveau vboxnetadp(OE) vboxnetflt(OE) mxm_wmi video vboxdrv(OE)
xt_CHECKSUM xt_MASQUERADE xt_conntrack ipt_REJECT nf_nat_tftp
nft_objref nf_conntrack_tftp tun bridge stp llc rpcsec_gss_krb5
auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache nft_fib_inet
nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4
nf_reject_ipv6 nft_reject nft_ct nft_chain_nat ip6table_nat
ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_nat
nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 iptable_mangle iptable_raw
iptable_security ip_set nf_tables nfnetlink ip6table_filter ip6_tables
iptable_filter overlay cmac bnep lm75 sunrpc vfat fat squashfs loop
intel_rapl_msr intel_rapl_common btusb btrtl snd_hda_codec_realtek
sb_edac btbcm btintel x86_pkg_temp_thermal snd_hda_codec_generic
snd_hda_codec_hdmi ledtrig_audio uvcvideo intel_powerclamp ucsi_ccg
videobuf2_vmalloc snd_hda_intel coretemp dm_cache_smq videobuf2_memops
[ 115.886422] bluetooth typec_ucsi snd_intel_dspcfg videobuf2_v4l2
kvm_intel iTCO_wdt intel_pmc_bxt typec iTCO_vendor_support
snd_usb_audio videobuf2_common snd_hda_codec dm_cache ecdh_generic ecc
pktcdvd kvm dm_persistent_data snd_usbmidi_lib videodev snd_hda_core
dm_bio_prison snd_hwdep snd_rawmidi snd_seq mc joydev irqbypass
snd_seq_device ocrdma rapl intel_cstate ib_uverbs snd_pcm hp_wmi
sparse_keymap intel_uncore pcspkr rfkill wmi_bmof ib_core snd_timer
snd i2c_i801 i2c_smbus lpc_ich soundcore i2c_nvidia_gpu tpm_infineon
binfmt_misc ip_tables amdgpu iommu_v2 gpu_sched crct10dif_pclmul
crc32_pclmul i2c_algo_bit crc32c_intel ttm drm_kms_helper cec
ghash_clmulni_intel serio_raw nvme drm e1000e be2net nvme_core wmi nbd
fuse
[ 115.886482] CPU: 18 PID: 321 Comm: kworker/18:1 Tainted: G W OE
5.8.18-200.jboero.fc32.x86_64 #1
[ 115.886485] Hardware name: Hewlett-Packard HP Z640 Workstation/212A,
BIOS M60 v02.54 06/12/2020
[ 115.886492] Workqueue: usb_hub_wq hub_event
[ 115.886498] RIP: 0010:usb_disconnect+0x25/0xb0
[ 115.886503] Code: 00 00 0f 1f 00 0f 1f 44 00 00 41 57 41 56 41 55 41
54 55 53 48 89 fb 48 83 ec 20 4c 8b 27 48 c7 c7 a0 29 68 98 e8 9f 86
78 ff <0f> 0b bf c8 00 00 00 e8 8f d7 83 ff 48 c7 c7 c0 b7 ea 98 e8 53
c5
[ 115.886507] RSP: 0018:ffffb96645bffd28 EFLAGS: 00010292
[ 115.886512] RAX: 0000000000000027 RBX: ffff92eab42e9000 RCX: 0000000000000000
[ 115.886515] RDX: ffff92eab4d38000 RSI: ffffffff9716eac5 RDI: 0000000000000246
[ 115.886519] RBP: 0000000000000100 R08: 0000001afb5cc4e1 R09: 0000000000000027
[ 115.886522] R10: 0000000000000000 R11: 0000000000000000 R12: ffff92eab0793000
[ 115.886526] R13: 000000000000000e R14: ffff92eab42eab70 R15: 000000000000000e
[ 115.886530] FS: 0000000000000000(0000) GS:ffff92eacd200000(0000)
knlGS:0000000000000000
[ 115.886534] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 115.886537] CR2: 00007f11514b2f40 CR3: 0000001fb95a0004 CR4: 00000000001606e0
[ 115.886541] Call Trace:
[ 115.886554] ? __mutex_unlock_slowpath+0x35/0x270
[ 115.886562] hub_event+0xc0e/0x1890
[ 115.886582] process_one_work+0x25d/0x570
[ 115.886592] worker_thread+0x55/0x3c0
[ 115.886598] ? process_one_work+0x570/0x570
[ 115.886605] kthread+0x13a/0x150
[ 115.886611] ? kthread_create_worker_on_cpu+0x40/0x40
[ 115.886621] ret_from_fork+0x22/0x30
[ 115.886638] irq event stamp: 11070
[ 115.886646] hardirqs last enabled at (11069): [<ffffffff97cc66a4>]
_raw_spin_unlock_irq+0x24/0x40
[ 115.886651] hardirqs last disabled at (11070): [<ffffffff97cbf702>]
__schedule+0xd2/0xa40
[ 115.886656] softirqs last enabled at (6882): [<ffffffff9800033d>]
__do_softirq+0x33d/0x44c
[ 115.886661] softirqs last disabled at (6871): [<ffffffff97e010d2>]
asm_call_irq_on_stack+0x12/0x20
[ 115.886664] ---[ end trace d02727115f119a2f ]---
[ 116.094920] usb 3-14: USB disconnect, device number 7
[ 120.933639] uvcvideo: uvc_v4l2_release

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[Alan Stern]

To recap: The problem is that uvcvideo tries to change an interface 
altsetting (from within uvc_video_start_streaming -> 
uvc_video_start_transfer) after the driver has been unbound from the 
device.  This triggers an invalid memory reference.

On Sun, Nov 22, 2020 at 08:03:43PM +0000, John Boero wrote:
> Thanks Alan
> I just spent some more time investigating and playing with different patches,
> locks, mutexes, and sleeps, and I think I see exactly what's happening now.
> I now understand why it:
> A) seems to happen randomly during uvc start_stream
> B) affects multiple device vendors
> C) has been reported in RaspberryPi and IoT threads
> 
> I put a trace on usb/core/hub.c:usb_disconnect to identify why the device was
> disconnecting and it seems this is a low power issue.  An idle webcam appears
> fine to usbcore but as soon as you initialize it or uvc starts a stream, it
> consumes more power, might find the cable can't supply it, and then disconnects
> via interrupt. In my case I can reproduce this consistently with a cheap USB
> extension cable, but this issue appears common to passive hubs, and IoT or SBCs
> that don't always supply clean power over USB.  My simplified patch can at least
> protect usbcore from crashing on a bad device:
> 
> From 73019d79fe4fd8b2c945005f8a067f528d8056fd Mon Sep 17 00:00:00 2001
> From: jboero <boeroboy@gmail.com>
> Date: Sun, 22 Nov 2020 19:30:41 +0000
> Subject: [PATCH] USBCore NULL interface deref fix
> 
> ---
> drivers/usb/core/usb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index bafc113f2b3e..da46c84c87ce 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -278,7 +278,7 @@ struct usb_interface *usb_ifnum_to_if(const struct
> usb_device *dev,
>        if (!config)
>                return NULL;
>        for (i = 0; i < config->desc.bNumInterfaces; i++)
> -               if (config->interface[i]->altsetting[0]
> +               if (config->interface[i] && config->interface[i]->altsetting[0]
>                                .desc.bInterfaceNumber == ifnum)
>                        return config->interface[i];

I really dislike the idea of papering over a problem instead of fixing 
it properly.

> This protects ongoing USB functionality including lsusb, and prevents a hang on
> reboot after error.  It doesn't help a user diagnose the error on the UVC side.
> A fix from the uvc side is a little trickier and I'd like an opinion on how
> best to handle locks in uvc_video_start_transfer. I've tried a few options
> around uvcvideo.c:1874
> 
> ret = usb_set_interface(stream->dev->udev, intfnum, altsetting);
> 
> I've even used multiple msleeps and checked for NULL interfaces but that feels
> like a cheap trick and I was wondering what lock solution might help best here?

Laurent Pinchart is the person to ask.  He is the maintainer of the 
uvcvideo driver.  I know very little about it.

Alan Stern

Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

[Alan Stern]

On Thu, Nov 12, 2020 at 05:13:30PM +0000, John Boero wrote:
> Sorry header was generated by git email and I should have
> paid closer attention to it before sending.
> Long time listener, first time caller.
> 
> Yes the patch is backwards sorry.  Testing alt proposal from
> stern@rowland.harvard.edu.  It may be a buggy driver
> but it would be nice if a buggy driver couldn't bring down
> the entire usb core. lsusb hangs until reboot or reset of usb.
> 
> It seems to behave fine on first use.  Run Zoom or cheese
> works fine first time.  Subsequent runs, no device found
> and usb is crashed with trace in dmesg.

The whole point of asking you to try out that change was to see the 
trace in dmesg.  What does it say?

Alan Stern