linux-kernel - Re: [PATCH] usb: gadget: udc: udc

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]

Message-ID: <87efsd6tcn.fsf@linux.intel.com>
Date:   Tue, 15 Aug 2017 13:03:20 +0300
From:   Felipe Balbi <balbi@...nel.org>
To:     Danilo Krummrich <danilokrummrich@...develop.de>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        gregkh@...uxfoundation.org, k.opasiak@...sung.com
Cc:     Danilo Krummrich <danilokrummrich@...develop.de>
Subject: Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind


Hi,

Danilo Krummrich <danilokrummrich@...develop.de> writes:
> udc_stop needs to be called before gadget driver unbind. Otherwise it
> might happen that udc drivers still call into the gadget driver (e.g.
> to reset gadget after OTG event). If this happens it is likely to get
> panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
> is set to NULL on unbind.

seems like the problem here is with the OTG layer, not UDC core.

> Signed-off-by: Danilo Krummrich <danilokrummrich@...develop.de>
> ---
> Actually there could still be a race:
> (CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as exsample)
>
> CPU0					CPU1
> ----					----
> usb_gadget_disconnect(udc->gadget);
> udc->driver->disconnect(udc->gadget);
> 					if (dwc->gadget_driver && dwc->gadget_driver->disconnect)
> usb_gadget_udc_stop(udc);
> udc->driver->unbind(udc->gadget);
> 					dwc->gadget_driver->disconnect(&dwc->gadget);
>
> UDC drivers typically set their gadget driver pointer to NULL in udc_stop
> and check for it before calling into the gadget driver. To fix the issue
> above every udc driver could apply a lock around this.
>
> If you see the need for having this or another solutions I can provide
> further patches. This patch could also just serve as a base for discussion
> if someone knows a smarter solution.
>
> I saw this problem causing a panic on hikey960 board and provided a quick
> workaround for the same problem here:
> https://android-review.googlesource.com/#/c/kernel/common/+/457476/
> (panic log in the commit message of the linked patch)
> ---
>  drivers/usb/gadget/udc/core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index efce68e9a8e0..8155468afc0d 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct usb_udc *udc)
>  
>  	usb_gadget_disconnect(udc->gadget);
>  	udc->driver->disconnect(udc->gadget);
> -	udc->driver->unbind(udc->gadget);
> +	/* udc_stop needs to be called before gadget driver unbind to prevent
> +	 * udc driver calls into gadget driver after unbind which could cause
> +	 * a nullptr exception.
> +	 */
>  	usb_gadget_udc_stop(udc);
> +	udc->driver->unbind(udc->gadget);

This patch is incorrect, it will prevent us from giving back requests to
gadget driver properly. ->unbind() has to happen before ->udc_stop().

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)