Re: [PATCH] USB: gadget: Fix use-after-free Read in usb_udc_uevent()
From: Marek Szyprowski <m.szyprowski@samsung.com>
Date: 2022-08-11 07:31:49
Hi Alan, On 10.08.2022 21:33, Alan Stern wrote:
On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote:quoted
On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote:quoted
This patch landed recently in linux-next as commit 2191c00855b0 ("USB: gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it fixes the issue by introducing another one. It doesn't look very probable, but it would be nice to fix it to make the lock dependency checker happy.Indeed. I suspect the problem is that udc_lock is held for too long. Probably it should be released during the calls to udc->driver->bind and udc->driver->unbind. Getting this right will require some careful study. Marek, if I send you a patch later, will you be able to test it?Here's a patch for you to try, when you have the chance. It reduces the scope of udc_lock to cover only the fields it's supposed to protect and changes the locking in a few other places. There's still the possibility of a locking cycle, because udc_lock is held in the ->disconnect pathway. It's very hard to know whether that might cause any trouble; it depends on how the function drivers handle disconnections.
It looks this fixed the issue I've reported. I've checked it on all my test systems and none reported any issue related to the udc. Feel free to add: Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
quoted hunk ↗ jump to hunk
Alan Stern Index: usb-devel/drivers/usb/gadget/udc/core.c ===================================================================--- usb-devel.orig/drivers/usb/gadget/udc/core.c +++ usb-devel/drivers/usb/gadget/udc/core.c@@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad ret = gadget->ops->pullup(gadget, 0); if (!ret) { gadget->connected = 0; - gadget->udc->driver->disconnect(gadget); + mutex_lock(&udc_lock); + if (gadget->udc->driver) + gadget->udc->driver->disconnect(gadget); + mutex_unlock(&udc_lock); } out:@@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev usb_gadget_udc_set_speed(udc, driver->max_speed); - mutex_lock(&udc_lock); ret = driver->bind(udc->gadget, driver); if (ret) goto err_bind;@@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev goto err_start; usb_gadget_enable_async_callbacks(udc); usb_udc_connect_control(udc); - mutex_unlock(&udc_lock); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0;@@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev dev_err(&udc->dev, "failed to start %s: %d\n", driver->function, ret); + mutex_lock(&udc_lock); udc->driver = NULL; driver->is_bound = false; mutex_unlock(&udc_lock);@@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); - mutex_lock(&udc_lock); usb_gadget_disconnect(gadget); usb_gadget_disable_async_callbacks(udc); if (gadget->irq)@@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct udc->driver->unbind(gadget); usb_gadget_udc_stop(udc); + mutex_lock(&udc_lock); driver->is_bound = false; udc->driver = NULL; mutex_unlock(&udc_lock);@@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct struct usb_udc *udc = container_of(dev, struct usb_udc, dev); ssize_t ret; - mutex_lock(&udc_lock); + device_lock(&udc->gadget->dev); if (!udc->driver) { dev_err(dev, "soft-connect without a gadget driver\n"); ret = -EOPNOTSUPP;@@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct ret = n; out: - mutex_unlock(&udc_lock); + device_unlock(&udc->gadget->dev); return ret; } static DEVICE_ATTR_WO(soft_connect);@@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi char *buf) { struct usb_udc *udc = container_of(dev, struct usb_udc, dev); - struct usb_gadget_driver *drv = udc->driver; + struct usb_gadget_driver *drv; + int rc = 0; - if (!drv || !drv->function) - return 0; - return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); + mutex_lock(&udc_lock); + drv = udc->driver; + if (drv && drv->function) + rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); + mutex_unlock(&udc_lock); + return rc; } static DEVICE_ATTR_RO(function);
Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland