| 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
| ||
|
Message-Id: <20220906132835.175305634@linuxfoundation.org>
Date: Tue, 6 Sep 2022 15:31:23 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: linux-kernel@...r.kernel.org
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
stable@...r.kernel.org, Felipe Balbi <balbi@...nel.org>,
Marek Szyprowski <m.szyprowski@...sung.com>,
Alan Stern <stern@...land.harvard.edu>
Subject: [PATCH 5.19 135/155] USB: gadget: Fix obscure lockdep violation for udc_mutex
From: Alan Stern <stern@...land.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@...nel.org>
Cc: stable@...r.kernel.org
Reported-by: Marek Szyprowski <m.szyprowski@...sung.com>
Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>
Signed-off-by: Alan Stern <stern@...land.harvard.edu>
Link: https://lore.kernel.org/r/YwkfhdxA/I2nOcK7@rowland.harvard.edu
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.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);
Powered by blists - more mailing lists