FAILED: patch "[PATCH] usb: ucsi: Fix ucsi->connector race" failed to apply to 6.2-stable tree

PATCH In review [email protected] 2 participants 4 messages Mar 28, 2023 – Apr 3, 2023

Subsystems (3)

Stable branch USB USB Type-C class

lore

Share & tools

lore

[email protected]

author maintainer

2023-03-28 12:01 UTC #0 lore raw

The patch below does not apply to the 6.2-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.2.y
git checkout FETCH_HEAD
git cherry-pick -x 0482c34ec6f8557e06cd0f8e2d0e20e8ede6a22c
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '[email protected]' --subject-prefix 'PATCH 6.2.y' HEAD^..

Possible dependencies:

0482c34ec6f8 ("usb: ucsi: Fix ucsi->connector race")
f87fb985452a ("usb: ucsi: Fix NULL pointer deref in ucsi_connector_change()") 924fb3ec50f5 ("Merge 6.2-rc7 into usb-next")

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0482c34ec6f8557e06cd0f8e2d0e20e8ede6a22c Mon Sep 17 00:00:00 2001
From: Hans de Goede <[email protected]>
Date: Wed, 8 Mar 2023 16:42:43 +0100
Subject: [PATCH] usb: ucsi: Fix ucsi->connector race

ucsi_init() which runs from a workqueue sets ucsi->connector and on an error will clear it again.

ucsi->connector gets dereferenced by ucsi_resume(), this checks for ucsi->connector being NULL in case ucsi_init() has not finished yet; or in case ucsi_init() has failed.

ucsi_init() setting ucsi->connector and then clearing it again on an error creates a race where the check in ucsi_resume() may pass, only to have ucsi->connector free-ed underneath it when ucsi_init() hits an error.

Fix this race by making ucsi_init() store the connector array in a local variable and only assign it to ucsi->connector on success.

Fixes bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")

Cc [email protected]

Reviewed-by Heikki Krogerus <[email protected]>

Signed-off-by Hans de Goede <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Greg Kroah-Hartman <[email protected]>

drivers/usb/typec/ucsi/ucsi.c +9 ~~-13~~

index 0623861c597b..8d1baf28df55 100644
@@ -1125,12 +1125,11 @@ static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
 	return NULL;
 }
 
-static int ucsi_register_port(struct ucsi *ucsi, int index)
+static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 {
 	struct usb_power_delivery_desc desc = { ucsi->cap.pd_version};
 	struct usb_power_delivery_capabilities_desc pd_caps;
 	struct usb_power_delivery_capabilities *pd_cap;
-	struct ucsi_connector *con = &ucsi->connector[index];
 	struct typec_capability *cap = &con->typec_cap;
 	enum typec_accessory *accessory = cap->accessory;
 	enum usb_role u_role = USB_ROLE_NONE;

@@ -1151,7 +1150,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	init_completion(&con->complete);
 	mutex_init(&con->lock);
 	INIT_LIST_HEAD(&con->partner_tasks);
-	con->num = index + 1;
 	con->ucsi = ucsi;
 
 	cap->fwnode = ucsi_find_fwnode(con);

@@ -1328,7 +1326,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
  */
 static int ucsi_init(struct ucsi *ucsi)
 {
-	struct ucsi_connector *con;
+	struct ucsi_connector *con, *connector;
 	u64 command, ntfy;
 	int ret;
 	int i;

@@ -1359,16 +1357,16 @@ static int ucsi_init(struct ucsi *ucsi)
 	}
 
 	/* Allocate the connectors. Released in ucsi_unregister() */
-	ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1,
-				  sizeof(*ucsi->connector), GFP_KERNEL);
-	if (!ucsi->connector) {
+	connector = kcalloc(ucsi->cap.num_connectors + 1, sizeof(*connector), GFP_KERNEL);
+	if (!connector) {
 		ret = -ENOMEM;
 		goto err_reset;
 	}
 
 	/* Register all connectors */
 	for (i = 0; i < ucsi->cap.num_connectors; i++) {
-		ret = ucsi_register_port(ucsi, i);
+		connector[i].num = i + 1;
+		ret = ucsi_register_port(ucsi, &connector[i]);
 		if (ret)
 			goto err_unregister;
 	}

@@ -1380,11 +1378,12 @@ static int ucsi_init(struct ucsi *ucsi)
 	if (ret < 0)
 		goto err_unregister;
 
+	ucsi->connector = connector;
 	ucsi->ntfy = ntfy;
 	return 0;
 
 err_unregister:
-	for (con = ucsi->connector; con->port; con++) {
+	for (con = connector; con->port; con++) {
 		ucsi_unregister_partner(con);
 		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
 		ucsi_unregister_port_psy(con);

@@ -1400,10 +1399,7 @@ static int ucsi_init(struct ucsi *ucsi)
 		typec_unregister_port(con->port);
 		con->port = NULL;
 	}
-
-	kfree(ucsi->connector);
-	ucsi->connector = NULL;
-
+	kfree(connector);
 err_reset:
 	memset(&ucsi->cap, 0, sizeof(ucsi->cap));
 	ucsi_reset_ppm(ucsi);

Joakim Tjernlund

2023-03-29 08:04 UTC #1 lore raw

PATCH 1/1 usb: ucsi: Fix ucsi->connector race

From: Hans de Goede <[email protected]>

ucsi_init() which runs from a workqueue sets ucsi->connector and on an error will clear it again.

ucsi->connector gets dereferenced by ucsi_resume(), this checks for ucsi->connector being NULL in case ucsi_init() has not finished yet; or in case ucsi_init() has failed.

Fix this race by making ucsi_init() store the connector array in a local variable and only assign it to ucsi->connector on success.

Fixes bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")

Cc [email protected]

Reviewed-by Heikki Krogerus <[email protected]>

Signed-off-by Hans de Goede <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Greg Kroah-Hartman <[email protected]>

(cherry picked from commit 0482c34ec6f8557e06cd0f8e2d0e20e8ede6a22c)

Signed-off-by Joakim Tjernlund <[email protected]>

---

- This is a dry port to 6.1.x, will be some time before it will be tested.

 drivers/usb/typec/ucsi/ucsi.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

drivers/usb/typec/ucsi/ucsi.c +9 ~~-13~~

index 8cbbb002fefe..086b50968983 100644
@@ -1039,9 +1039,8 @@ static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
 	return NULL;
 }
 
-static int ucsi_register_port(struct ucsi *ucsi, int index)
+static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 {
-	struct ucsi_connector *con = &ucsi->connector[index];
 	struct typec_capability *cap = &con->typec_cap;
 	enum typec_accessory *accessory = cap->accessory;
 	enum usb_role u_role = USB_ROLE_NONE;

@@ -1062,7 +1061,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	init_completion(&con->complete);
 	mutex_init(&con->lock);
 	INIT_LIST_HEAD(&con->partner_tasks);
-	con->num = index + 1;
 	con->ucsi = ucsi;
 
 	cap->fwnode = ucsi_find_fwnode(con);

@@ -1204,7 +1202,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
  */
 static int ucsi_init(struct ucsi *ucsi)
 {
-	struct ucsi_connector *con;
+	struct ucsi_connector *con, *connector;
 	u64 command, ntfy;
 	int ret;
 	int i;

@@ -1235,16 +1233,16 @@ static int ucsi_init(struct ucsi *ucsi)
 	}
 
 	/* Allocate the connectors. Released in ucsi_unregister() */
-	ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1,
-				  sizeof(*ucsi->connector), GFP_KERNEL);
-	if (!ucsi->connector) {
+	connector = kcalloc(ucsi->cap.num_connectors + 1, sizeof(*connector), GFP_KERNEL);
+	if (!connector) {
 		ret = -ENOMEM;
 		goto err_reset;
 	}
 
 	/* Register all connectors */
 	for (i = 0; i < ucsi->cap.num_connectors; i++) {
-		ret = ucsi_register_port(ucsi, i);
+		connector[i].num = i + 1;
+		ret = ucsi_register_port(ucsi, &connector[i]);
 		if (ret)
 			goto err_unregister;
 	}

@@ -1256,11 +1254,12 @@ static int ucsi_init(struct ucsi *ucsi)
 	if (ret < 0)
 		goto err_unregister;
 
+	ucsi->connector = connector;
 	ucsi->ntfy = ntfy;
 	return 0;
 
 err_unregister:
-	for (con = ucsi->connector; con->port; con++) {
+	for (con = connector; con->port; con++) {
 		ucsi_unregister_partner(con);
 		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
 		ucsi_unregister_port_psy(con);

@@ -1269,10 +1268,7 @@ static int ucsi_init(struct ucsi *ucsi)
 		typec_unregister_port(con->port);
 		con->port = NULL;
 	}
-
-	kfree(ucsi->connector);
-	ucsi->connector = NULL;
-
+	kfree(connector);
 err_reset:
 	memset(&ucsi->cap, 0, sizeof(ucsi->cap));
 	ucsi_reset_ppm(ucsi);

Signature · 1 line

2.39.1

Joakim Tjernlund

2023-03-30 16:30 UTC #2 lore raw

Re: [PATCH] usb: ucsi: Fix ucsi->connector race

Quoted 25 lines · Joakim Tjernlund

From: Hans de Goede <[email protected]>
ucsi_init() which runs from a workqueue sets ucsi->connector and on an error will clear it again.
ucsi->connector gets dereferenced by ucsi_resume(), this checks for ucsi->connector being NULL in case ucsi_init() has not finished yet; or in case ucsi_init() has failed.
ucsi_init() setting ucsi->connector and then clearing it again on an error creates a race where the check in ucsi_resume() may pass, only to have ucsi->connector free-ed underneath it when ucsi_init() hits an error.
Fix this race by making ucsi_init() store the connector array in a local variable and only assign it to ucsi->connector on success.
Fixes bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
Cc [email protected]
Reviewed-by Heikki Krogerus <[email protected]>
Signed-off-by Hans de Goede <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Greg Kroah-Hartman <[email protected]>
(cherry picked from commit 0482c34ec6f8557e06cd0f8e2d0e20e8ede6a22c)
Signed-off-by Joakim Tjernlund <[email protected]>

Quoted 3 lines

---
- This is a dry port to 6.1.x, will be some time before it will be tested.

Tested OK now on 6.1.22

Jocke

Quoted 83 lines

 drivers/usb/typec/ucsi/ucsi.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

drivers/usb/typec/ucsi/ucsi.c +9 ~~-13~~

index 8cbbb002fefe..086b50968983 100644
@@ -1039,9 +1039,8 @@ static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
 	return NULL;
 }
 
-static int ucsi_register_port(struct ucsi *ucsi, int index)
+static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 {
-	struct ucsi_connector *con = &ucsi->connector[index];
 	struct typec_capability *cap = &con->typec_cap;
 	enum typec_accessory *accessory = cap->accessory;
 	enum usb_role u_role = USB_ROLE_NONE;

@@ -1062,7 +1061,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	init_completion(&con->complete);
 	mutex_init(&con->lock);
 	INIT_LIST_HEAD(&con->partner_tasks);
-	con->num = index + 1;
 	con->ucsi = ucsi;
 
 	cap->fwnode = ucsi_find_fwnode(con);

@@ -1204,7 +1202,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
  */
 static int ucsi_init(struct ucsi *ucsi)
 {
-	struct ucsi_connector *con;
+	struct ucsi_connector *con, *connector;
 	u64 command, ntfy;
 	int ret;
 	int i;

@@ -1235,16 +1233,16 @@ static int ucsi_init(struct ucsi *ucsi)
 	}
 
 	/* Allocate the connectors. Released in ucsi_unregister() */
-	ucsi->connector = kcalloc(ucsi->cap.num_connectors + 1,
-				  sizeof(*ucsi->connector), GFP_KERNEL);
-	if (!ucsi->connector) {
+	connector = kcalloc(ucsi->cap.num_connectors + 1, sizeof(*connector), GFP_KERNEL);
+	if (!connector) {
 		ret = -ENOMEM;
 		goto err_reset;
 	}
 
 	/* Register all connectors */
 	for (i = 0; i < ucsi->cap.num_connectors; i++) {
-		ret = ucsi_register_port(ucsi, i);
+		connector[i].num = i + 1;
+		ret = ucsi_register_port(ucsi, &connector[i]);
 		if (ret)
 			goto err_unregister;
 	}

@@ -1256,11 +1254,12 @@ static int ucsi_init(struct ucsi *ucsi)
 	if (ret < 0)
 		goto err_unregister;
 
+	ucsi->connector = connector;
 	ucsi->ntfy = ntfy;
 	return 0;
 
 err_unregister:
-	for (con = ucsi->connector; con->port; con++) {
+	for (con = connector; con->port; con++) {
 		ucsi_unregister_partner(con);
 		ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
 		ucsi_unregister_port_psy(con);

@@ -1269,10 +1268,7 @@ static int ucsi_init(struct ucsi *ucsi)
 		typec_unregister_port(con->port);
 		con->port = NULL;
 	}
-
-	kfree(ucsi->connector);
-	ucsi->connector = NULL;
-
+	kfree(connector);
 err_reset:
 	memset(&ucsi->cap, 0, sizeof(ucsi->cap));
 	ucsi_reset_ppm(ucsi);

[email protected]

author maintainer

2023-04-03 13:26 UTC #3 lore raw

Re: [PATCH] usb: ucsi: Fix ucsi->connector race

Quoted 28 lines · Joakim Tjernlund

Quoted 27 lines · Joakim Tjernlund
From: Hans de Goede <[email protected]>
ucsi_init() which runs from a workqueue sets ucsi->connector and on an error will clear it again.
ucsi->connector gets dereferenced by ucsi_resume(), this checks for ucsi->connector being NULL in case ucsi_init() has not finished yet; or in case ucsi_init() has failed.
ucsi_init() setting ucsi->connector and then clearing it again on an error creates a race where the check in ucsi_resume() may pass, only to have ucsi->connector free-ed underneath it when ucsi_init() hits an error.
Fix this race by making ucsi_init() store the connector array in a local variable and only assign it to ucsi->connector on success.
Fixes bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
Cc [email protected]
Reviewed-by Heikki Krogerus <[email protected]>
Signed-off-by Hans de Goede <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Greg Kroah-Hartman <[email protected]>
(cherry picked from commit 0482c34ec6f8557e06cd0f8e2d0e20e8ede6a22c)
Signed-off-by Joakim Tjernlund <[email protected]>
---

Quoted 3 lines

Quoted 1 line
- This is a dry port to 6.1.x, will be some time before it will be tested.
Tested OK now on 6.1.22

Thanks, now queued up for 6.2.y and 6.1.y. Still need backports for older kernels if you want to do that...

thanks,

greg k-h