linux-kernel - [PATCH] usb: roles: cache usb roles received during switch registration

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250127230715.6142-1-quic_eserrao@quicinc.com>
Date: Mon, 27 Jan 2025 15:07:15 -0800
From: Elson Roy Serrao <quic_eserrao@...cinc.com>
To: gregkh@...uxfoundation.org, heikki.krogerus@...ux.intel.com,
        xu.yang_2@....com
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Elson Roy Serrao <quic_eserrao@...cinc.com>, stable@...r.kernel.org
Subject: [PATCH] usb: roles: cache usb roles received during switch registration

The role switch registration and set_role() can happen in parallel as they
are invoked independent of each other. There is a possibility that a driver
might spend significant amount of time in usb_role_switch_register() API
due to the presence of time intensive operations like component_add()
which operate under common mutex. This leads to a time window after
allocating the switch and before setting the registered flag where the set
role notifications are dropped. Below timeline summarizes this behavior

Thread1				|	Thread2
usb_role_switch_register()	|
	|			|
	---> allocate switch	|
	|			|
	---> component_add()	|	usb_role_switch_set_role()
	|			|	|
	|			|	--> Drop role notifications
	|			|	    since sw->registered
	|			|	    flag is not set.
	|			|
	--->Set registered flag.|

To avoid this, cache the last role received and set it once the switch
registration is complete. Since we are now caching the roles based on
registered flag, protect this flag with the switch mutex.

Fixes: b787a3e78175 ("usb: roles: don't get/set_role() when usb_role_switch is unregistered")
cc: stable@...r.kernel.org
Signed-off-by: Elson Roy Serrao <quic_eserrao@...cinc.com>
---
 drivers/usb/roles/class.c | 45 ++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index c58a12c147f4..c0149c31c01b 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -26,6 +26,8 @@ struct usb_role_switch {
 	struct mutex lock; /* device lock*/
 	struct module *module; /* the module this device depends on */
 	enum usb_role role;
+	enum usb_role cached_role;
+	bool cached;
 	bool registered;
 
 	/* From descriptor */
@@ -65,6 +67,20 @@ static const struct component_ops connector_ops = {
 	.unbind = connector_unbind,
 };
 
+static int __usb_role_switch_set_role(struct usb_role_switch *sw,
+				      enum usb_role role)
+{
+	int ret;
+
+	ret = sw->set(sw, role);
+	if (!ret) {
+		sw->role = role;
+		kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
+	}
+
+	return ret;
+}
+
 /**
  * usb_role_switch_set_role - Set USB role for a switch
  * @sw: USB role switch
@@ -79,17 +95,21 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
 	if (IS_ERR_OR_NULL(sw))
 		return 0;
 
-	if (!sw->registered)
-		return -EOPNOTSUPP;
-
+	/*
+	 * Since we have a valid sw struct here, role switch registration might
+	 * be in progress. Hence cache the role here and send it out once
+	 * registration is complete.
+	 */
 	mutex_lock(&sw->lock);
-
-	ret = sw->set(sw, role);
-	if (!ret) {
-		sw->role = role;
-		kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
+	if (!sw->registered) {
+		sw->cached = true;
+		sw->cached_role = role;
+		mutex_unlock(&sw->lock);
+		return 0;
 	}
 
+	ret = __usb_role_switch_set_role(sw, role);
+
 	mutex_unlock(&sw->lock);
 
 	return ret;
@@ -399,8 +419,14 @@ usb_role_switch_register(struct device *parent,
 			dev_warn(&sw->dev, "failed to add component\n");
 	}
 
+	mutex_lock(&sw->lock);
 	sw->registered = true;
 
+	if (sw->cached)
+		__usb_role_switch_set_role(sw, sw->cached_role);
+
+	mutex_unlock(&sw->lock);
+
 	/* TODO: Symlinks for the host port and the device controller. */
 
 	return sw;
@@ -417,7 +443,10 @@ void usb_role_switch_unregister(struct usb_role_switch *sw)
 {
 	if (IS_ERR_OR_NULL(sw))
 		return;
+	mutex_lock(&sw->lock);
 	sw->registered = false;
+	sw->cached = false;
+	mutex_unlock(&sw->lock);
 	if (dev_fwnode(&sw->dev))
 		component_del(&sw->dev, &connector_ops);
 	device_unregister(&sw->dev);
-- 
2.17.1