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

Date:   Mon, 14 Aug 2017 18:36:52 +0200
From:   Danilo Krummrich <danilokrummrich@...develop.de>
To:     linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        gregkh@...uxfoundation.org, balbi@...nel.org, k.opasiak@...sung.com
Cc:     Danilo Krummrich <danilokrummrich@...develop.de>
Subject: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

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.

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);
 
 	udc->driver = NULL;
 	udc->dev.driver = NULL;
-- 
2.14.1