lkml.org 
[lkml]   [2022]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.19 135/155] USB: gadget: Fix obscure lockdep violation for udc_mutex
    Date
    From: Alan Stern <stern@rowland.harvard.edu>

    commit 1016fc0c096c92dd0e6e0541daac7a7868169903 upstream.

    A recent commit expanding the scope of the udc_lock mutex in the
    gadget core managed to cause an obscure and slightly bizarre lockdep
    violation. In abbreviated form:

    ======================================================
    WARNING: possible circular locking dependency detected
    5.19.0-rc7+ #12510 Not tainted
    ------------------------------------------------------
    udevadm/312 is trying to acquire lock:
    ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0

    but task is already holding lock:
    ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0

    which lock already depends on the new lock.

    the existing dependency chain (in reverse order) is:

    -> #3 (kn->active#4){++++}-{0:0}:
           lock_acquire+0x68/0x84
           __kernfs_remove+0x268/0x380
           kernfs_remove_by_name_ns+0x58/0xac
           sysfs_remove_file_ns+0x18/0x24
           device_del+0x15c/0x440

    -> #2 (device_links_lock){+.+.}-{3:3}:
           lock_acquire+0x68/0x84
           __mutex_lock+0x9c/0x430
           mutex_lock_nested+0x38/0x64
           device_link_remove+0x3c/0xa0
           _regulator_put.part.0+0x168/0x190
           regulator_put+0x3c/0x54
           devm_regulator_release+0x14/0x20

    -> #1 (regulator_list_mutex){+.+.}-{3:3}:
           lock_acquire+0x68/0x84
           __mutex_lock+0x9c/0x430
           mutex_lock_nested+0x38/0x64
           regulator_lock_dependent+0x54/0x284
           regulator_enable+0x34/0x80
           phy_power_on+0x24/0x130
           __dwc2_lowlevel_hw_enable+0x100/0x130
           dwc2_lowlevel_hw_enable+0x18/0x40
           dwc2_hsotg_udc_start+0x6c/0x2f0
           gadget_bind_driver+0x124/0x1f4

    -> #0 (udc_lock){+.+.}-{3:3}:
           __lock_acquire+0x1298/0x20cc
           lock_acquire.part.0+0xe0/0x230
           lock_acquire+0x68/0x84
           __mutex_lock+0x9c/0x430
           mutex_lock_nested+0x38/0x64
           usb_udc_uevent+0x54/0xe0

    Evidently this was caused by the scope of udc_mutex being too large.
    The mutex is only meant to protect udc->driver along with a few other
    things. As far as I can tell, there's no reason for the mutex to be
    held while the gadget core calls a gadget driver's ->bind or ->unbind
    routine, or while a UDC is being started or stopped. (This accounts
    for link #1 in the chain above, where the mutex is held while the
    dwc2_hsotg_udc is started as part of driver probing.)

    Gadget drivers' ->disconnect callbacks are problematic. Even though
    usb_gadget_disconnect() will now acquire the udc_mutex, there's a
    window in usb_gadget_bind_driver() between the times when the mutex is
    released and the ->bind callback is invoked. If a disconnect occurred
    during that window, we could call the driver's ->disconnect routine
    before its ->bind routine. To prevent this from happening, it will be
    necessary to prevent a UDC from connecting while it has no gadget
    driver. This should be done already but it doesn't seem to be;
    currently usb_gadget_connect() has no check for this. Such a check
    will have to be added later.

    Some degree of mutual exclusion is required in soft_connect_store(),
    which can dereference udc->driver at arbitrary times since it is a
    sysfs callback. The solution here is to acquire the gadget's device
    lock rather than the udc_mutex. Since the driver core guarantees that
    the device lock is always held during driver binding and unbinding,
    this will make the accesses in soft_connect_store() mutually exclusive
    with any changes to udc->driver.

    Lastly, it turns out there is one place which should hold the
    udc_mutex but currently does not: The function_show() routine needs
    protection while it dereferences udc->driver. The missing lock and
    unlock calls are added.

    Link: https://lore.kernel.org/all/b2ba4245-9917-e399-94c8-03a383e7070e@samsung.com/
    Fixes: 2191c00855b0 ("USB: gadget: Fix use-after-free Read in usb_udc_uevent()")
    Cc: Felipe Balbi <balbi@kernel.org>
    Cc: stable@vger.kernel.org
    Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
    Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
    Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
    Link: https://lore.kernel.org/r/YwkfhdxA/I2nOcK7@rowland.harvard.edu
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    drivers/usb/gadget/udc/core.c | 26 ++++++++++++++++----------
    1 file changed, 16 insertions(+), 10 deletions(-)

    --- a/drivers/usb/gadget/udc/core.c
    +++ b/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);


    \
     
     \ /
      Last update: 2022-09-06 16:21    [W:3.712 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site