Hi Alan,
On 10.08.2022 21:33, Alan Stern wrote:
> On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote:
>> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote:
>>> 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@xxxxxxxxxxx>
Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> 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
