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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201123152654.GB708586@rowland.harvard.edu>
Date:   Mon, 23 Nov 2020 10:26:54 -0500
From:   Alan Stern <stern@...land.harvard.edu>
To:     John Boero <boeroboy@...il.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...nel.org>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: core: Null deref in kernel with USB webcams.

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@...il.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