[PATCH] usb: roles: wait for role callbacks on unregister

[PATCH] usb: roles: wait for role callbacks on unregister

[Keisuke Tsukuda]

usb_role_switch_set_role() and usb_role_switch_get_role() check the
registered flag before taking the switch lock. If a provider is
unregistered during a role request, unregister can return while its
callback is still running. The provider may then free memory used by
the callback, causing a use-after-free. A caller that passed the check
can also enter a callback after unregister returns.

Move the registered checks under the switch lock and clear the flag while
holding the same lock on unregister. This waits for active callbacks and
rejects callers that acquire the lock after the flag is cleared.

The Apple DWC3 remove path currently holds its private mutex around
unregister. Its role callbacks take that mutex under the switch lock, so
waiting on the switch lock in unregister would reverse the lock order.
Unregister before taking the Apple mutex.

Fixes: fde0aa6c175a ("usb: common: Small class for USB role switches")
Signed-off-by: Keisuke Tsukuda <tkdkei@outlook.jp>
---
Tested on ARM64 QEMU with Linux 7.3.4 and mainline 7.4-rc2
(067c5c24cef2), with KASAN and lockdep enabled. Synthetic providers
trigger KASAN use-after-free reports in both set and get callbacks before
this change. With the change, unregister waits for the callbacks and
neither test reports KASAN.

A lockdep test modeling the Apple callback/remove lock order warns for the
original order and not for the new one. Apple DWC3 hardware was not tested.

 drivers/usb/roles/class.c     |   18 ++++++++++++++----
 drivers/usb/dwc3/dwc3-apple.c |    4 ++--
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -79,10 +79,11 @@
 	if (IS_ERR_OR_NULL(sw))
 		return 0;
 
-	if (!sw->registered)
-		return -EOPNOTSUPP;
-
 	mutex_lock(&sw->lock);
+	if (!sw->registered) {
+		ret = -EOPNOTSUPP;
+		goto unlock;
+	}
 
 	ret = sw->set(sw, role);
 	if (!ret) {
@@ -90,6 +91,7 @@
 		kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
 	}
 
+unlock:
 	mutex_unlock(&sw->lock);
 
 	return ret;
@@ -107,16 +109,21 @@
 {
 	enum usb_role role;
 
-	if (IS_ERR_OR_NULL(sw) || !sw->registered)
+	if (IS_ERR_OR_NULL(sw))
 		return USB_ROLE_NONE;
 
 	mutex_lock(&sw->lock);
+	if (!sw->registered) {
+		role = USB_ROLE_NONE;
+		goto unlock;
+	}
 
 	if (sw->get)
 		role = sw->get(sw);
 	else
 		role = sw->role;
 
+unlock:
 	mutex_unlock(&sw->lock);
 
 	return role;
@@ -423,7 +430,10 @@
 {
 	if (IS_ERR_OR_NULL(sw))
 		return;
+	/* Wait for in-flight set/get callbacks before unregistering. */
+	mutex_lock(&sw->lock);
 	sw->registered = false;
+	mutex_unlock(&sw->lock);
 	if (dev_fwnode(&sw->dev))
 		component_del(&sw->dev, &connector_ops);
 	device_unregister(&sw->dev);
diff --git a/drivers/usb/dwc3/dwc3-apple.c b/drivers/usb/dwc3/dwc3-apple.c
--- a/drivers/usb/dwc3/dwc3-apple.c
+++ b/drivers/usb/dwc3/dwc3-apple.c
@@ -487,9 +487,9 @@
 	struct dwc3 *dwc = platform_get_drvdata(pdev);
 	struct dwc3_apple *appledwc = to_dwc3_apple(dwc);
 
+	usb_role_switch_unregister(appledwc->role_sw);
+
 	guard(mutex)(&appledwc->lock);
-
-	usb_role_switch_unregister(appledwc->role_sw);
 
 	/*
 	 * If we're still in DWC3_APPLE_PROBE_PENDING we never got any cable connected event and

Re: [PATCH] usb: roles: wait for role callbacks on unregister

[sashiko-bot]

> usb_role_switch_set_role() and usb_role_switch_get_role() check the
> registered flag before taking the switch lock. If a provider is
> unregistered during a role request, unregister can return while its
> callback is still running. The provider may then free memory used by
> the callback, causing a use-after-free. A caller that passed the check
> can also enter a callback after unregister returns.
> 
> Move the registered checks under the switch lock and clear the flag while
> holding the same lock on unregister. This waits for active callbacks and
> rejects callers that acquire the lock after the flag is cleared.
> 
> The Apple DWC3 remove path currently holds its private mutex around
> unregister. Its role callbacks take that mutex under the switch lock, so
> waiting on the switch lock in unregister would reverse the lock order.
> Unregister before taking the Apple mutex.
> 
> Fixes: fde0aa6c175a ("usb: common: Small class for USB role switches")
> Signed-off-by: Keisuke Tsukuda <tkdkei@outlook.jp>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112024101.83321-1-tkdkei@outlook.jp?part=1