linux-kernel - Re: [PATCH v2] usb: hub: Do not attempt to autosuspend disconnected devices

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 Mar 2017 17:12:20 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Guenter Roeck <linux@...ck-us.net>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Douglas Anderson <dianders@...omium.org>,
        Brian Norris <briannorris@...omium.org>
Subject: Re: [PATCH v2] usb: hub: Do not attempt to autosuspend disconnected
 devices

On Mon, 20 Mar 2017, Guenter Roeck wrote:

> While running a bind/unbind stress test with the dwc3 usb driver on rk3399,
> the following crash was observed.
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000218
> pgd = ffffffc00165f000
> [00000218] *pgd=000000000174f003, *pud=000000000174f003,
> 				*pmd=0000000001750003, *pte=00e8000001751713

...

> Code analysis shows that intf is set to NULL in usb_disable_device() prior
> to setting actconfig to NULL. At the same time, usb_runtime_idle() does not
> lock the usb device, and neither does any of the functions in the
> traceback. This means that there is no protection against a race condition
> where usb_disable_device() is removing dev->actconfig->interface[] pointers
> while those are being accessed from autosuspend_check().
> 
> To solve the problem, synchronize and validate device state between
> autosuspend_check() and usb_disconnect().
> 
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
> v2: Do not disable autosuspend in usb_disconnect(). Instead, check the
>     usb device state in autosuspend_check().
>     In usb_disconnect(), call pm_runtime_barrier() earlier, immediately
>     after changing the USB device state.
> 
>  drivers/usb/core/driver.c | 3 +++
>  drivers/usb/core/hub.c    | 7 +++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 7ebdf2a4e8fe..eb87a259d55c 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1781,6 +1781,9 @@ static int autosuspend_check(struct usb_device *udev)
>  	int			w, i;
>  	struct usb_interface	*intf;
>  
> +	if (udev->state == USB_STATE_NOTATTACHED)
> +		return -ENODEV;
> +
>  	/* Fail if autosuspend is disabled, or any interfaces are in use, or
>  	 * any interface drivers require remote wakeup but it isn't available.
>  	 */
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 57bb427c8878..fd80513aaeff 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2084,6 +2084,13 @@ void usb_disconnect(struct usb_device **pdev)
>  	 * this quiesces everything except pending urbs.
>  	 */
>  	usb_set_device_state(udev, USB_STATE_NOTATTACHED);
> +
> +	/*
> +	 * Ensure that the pm runtime code knows that the USB device
> +	 * is in the process of being disconnected.
> +	 */
> +	pm_runtime_barrier(&udev->dev);
> +
>  	dev_info(&udev->dev, "USB disconnect, device number %d\n",
>  			udev->devnum);
>  

Very minor nit: I would place the pm_runtime_barrier() after the 
dev_info(), so that we would know the device is going away if anything 
should go wrong during the barrier call.

Aside from that,

Acked-by: Alan Stern <stern@...land.harvard.edu>

Alan Stern