Revert "usb: typec: ucsi: Update UCSI structure to have message in and message out fields"

This reverts commit 3e08297.

The new buffer management code has not been tested or reviewed properly
and breaks boot of machines like the Lenovo ThinkPad X13s:

    Unable to handle kernel NULL pointer dereference at virtual address
    0000000000000000

    CPU: 0 UID: 0 PID: 813 Comm: kworker/0:3 Not tainted 6.19.0-rc2 #26 PREEMPT
    Hardware name: LENOVO 21BYZ9SRUS/21BYZ9SRUS, BIOS N3HET87W (1.59 ) 12/05/2023
    Workqueue: events ucsi_handle_connector_change [typec_ucsi]

    Call trace:
     ucsi_sync_control_common+0xe4/0x1ec [typec_ucsi] (P)
     ucsi_run_command+0xcc/0x194 [typec_ucsi]
     ucsi_send_command_common+0x84/0x2a0 [typec_ucsi]
     ucsi_get_connector_status+0x48/0x78 [typec_ucsi]
     ucsi_handle_connector_change+0x5c/0x4f4 [typec_ucsi]
     process_one_work+0x208/0x60c
     worker_thread+0x244/0x388

The new code completely ignores concurrency so that the message length
can be updated while a transaction is ongoing. In the above case, the
length ends up being modified by another thread while processing an ack
so that the NULL cci pointer is dereferenced.

Fixing this will require designing a proper interface for managing these
transactions, something which most likely involves reverting most of the
offending commit anyway.

Revert the broken code to fix the regression and let Intel come up with
a properly tested implementation for a later kernel.

Fixes: 3e08297 ("usb: typec: ucsi: Update UCSI structure to have message in and message out fields")
Signed-off-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Link: https://patch.msgid.link/20251222152204.2846-5-johan@kernel.org

Author: jhovold

drivers/usb/typec/ucsi/cros_ec_ucsi.c

@@ -105,12 +105,13 @@ static int cros_ucsi_async_control(struct ucsi *ucsi, u64 cmd)
 	return 0;
 }
 
-static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd, u32 *cci)
+static int cros_ucsi_sync_control(struct ucsi *ucsi, u64 cmd, u32 *cci,
+				  void *data, size_t size)
 {
 	struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
 	int ret;
 
-	ret = ucsi_sync_control_common(ucsi, cmd, cci);
+	ret = ucsi_sync_control_common(ucsi, cmd, cci, data, size);
 	switch (ret) {
 	case -EBUSY:
 		/* EC may return -EBUSY if CCI.busy is set.

drivers/usb/typec/ucsi/debugfs.c

@@ -37,8 +37,7 @@ static int ucsi_cmd(void *data, u64 val)
 	case UCSI_SET_USB:
 	case UCSI_SET_POWER_LEVEL:
 	case UCSI_READ_POWER_LEVEL:
-		ucsi->message_in_size = 0;
-		ret = ucsi_send_command(ucsi, val);
+		ret = ucsi_send_command(ucsi, val, NULL, 0);
 		break;
 	case UCSI_GET_CAPABILITY:
 	case UCSI_GET_CONNECTOR_CAPABILITY:
@@ -53,9 +52,9 @@ static int ucsi_cmd(void *data, u64 val)
 	case UCSI_GET_ATTENTION_VDO:
 	case UCSI_GET_CAM_CS:
 	case UCSI_GET_LPM_PPM_INFO:
-		ucsi->message_in_size = sizeof(ucsi->debugfs->response);
-		ret = ucsi_send_command(ucsi, val);
-		memcpy(&ucsi->debugfs->response, ucsi->message_in, sizeof(ucsi->debugfs->response));
+		ret = ucsi_send_command(ucsi, val,
+					&ucsi->debugfs->response,
+					sizeof(ucsi->debugfs->response));
 		break;
 	default:
 		ret = -EOPNOTSUPP;

drivers/usb/typec/ucsi/displayport.c

@@ -67,14 +67,11 @@ static int ucsi_displayport_enter(struct typec_altmode *alt, u32 *vdo)
 	}
 
 	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(dp->con->num);
-	ucsi->message_in_size = sizeof(cur);
-	ret = ucsi_send_command(ucsi, command);
+	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur));
 	if (ret < 0) {
 		if (ucsi->version > 0x0100)
 			goto err_unlock;
 		cur = 0xff;
-	} else {
-		memcpy(&cur, ucsi->message_in, ucsi->message_in_size);
 	}
 
 	if (cur != 0xff) {
@@ -129,8 +126,7 @@ static int ucsi_displayport_exit(struct typec_altmode *alt)
 	}
 
 	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 0, dp->offset, 0);
-	dp->con->ucsi->message_in_size = 0;
-	ret = ucsi_send_command(dp->con->ucsi, command);
+	ret = ucsi_send_command(dp->con->ucsi, command, NULL, 0);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -197,8 +193,7 @@ static int ucsi_displayport_configure(struct ucsi_dp *dp)
 
 	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 1, dp->offset, pins);
 
-	dp->con->ucsi->message_in_size = 0;
-	return ucsi_send_command(dp->con->ucsi, command);
+	return ucsi_send_command(dp->con->ucsi, command, NULL, 0);
 }
 
 static int ucsi_displayport_vdm(struct typec_altmode *alt,

drivers/usb/typec/ucsi/ucsi.c

@@ -55,7 +55,8 @@ void ucsi_notify_common(struct ucsi *ucsi, u32 cci)
 }
 EXPORT_SYMBOL_GPL(ucsi_notify_common);
 
-int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci)
+int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci,
+			     void *data, size_t size)
 {
 	bool ack = UCSI_COMMAND(command) == UCSI_ACK_CC_CI;
 	int ret;
@@ -83,10 +84,9 @@ int ucsi_sync_control_common(struct ucsi *ucsi, u64 command, u32 *cci)
 	if (!ret && cci)
 		ret = ucsi->ops->read_cci(ucsi, cci);
 
-	if (!ret && ucsi->message_in_size > 0 &&
+	if (!ret && data &&
 	    (*cci & UCSI_CCI_COMMAND_COMPLETE))
-		ret = ucsi->ops->read_message_in(ucsi, ucsi->message_in,
-						 ucsi->message_in_size);
+		ret = ucsi->ops->read_message_in(ucsi, data, size);
 
 	return ret;
 }
@@ -103,25 +103,23 @@ static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
 		ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
 	}
 
-	ucsi->message_in_size = 0;
-	return ucsi->ops->sync_control(ucsi, ctrl, NULL);
+	return ucsi->ops->sync_control(ucsi, ctrl, NULL, NULL, 0);
 }
 
-static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci, bool conn_ack)
+static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
+			    void *data, size_t size, bool conn_ack)
 {
 	int ret, err;
 
 	*cci = 0;
 
-	if (ucsi->message_in_size > UCSI_MAX_DATA_LENGTH(ucsi))
+	if (size > UCSI_MAX_DATA_LENGTH(ucsi))
 		return -EINVAL;
 
-	ret = ucsi->ops->sync_control(ucsi, command, cci);
+	ret = ucsi->ops->sync_control(ucsi, command, cci, data, size);
 
-	if (*cci & UCSI_CCI_BUSY) {
-		ucsi->message_in_size = 0;
-		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, false) ?: -EBUSY;
-	}
+	if (*cci & UCSI_CCI_BUSY)
+		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, NULL, 0, false) ?: -EBUSY;
 	if (ret)
 		return ret;
 
@@ -153,13 +151,10 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
 	int ret;
 
 	command = UCSI_GET_ERROR_STATUS | UCSI_CONNECTOR_NUMBER(connector_num);
-	ucsi->message_in_size = sizeof(error);
-	ret = ucsi_run_command(ucsi, command, &cci, false);
+	ret = ucsi_run_command(ucsi, command, &cci, &error, sizeof(error), false);
 	if (ret < 0)
 		return ret;
 
-	memcpy(&error, ucsi->message_in, sizeof(error));
-
 	switch (error) {
 	case UCSI_ERROR_INCOMPATIBLE_PARTNER:
 		return -EOPNOTSUPP;
@@ -205,7 +200,8 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
 	return -EIO;
 }
 
-static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd, bool conn_ack)
+static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
+				    void *data, size_t size, bool conn_ack)
 {
 	u8 connector_num;
 	u32 cci;
@@ -233,7 +229,7 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd, bool conn_ack)
 
 	mutex_lock(&ucsi->ppm_lock);
 
-	ret = ucsi_run_command(ucsi, cmd, &cci, conn_ack);
+	ret = ucsi_run_command(ucsi, cmd, &cci, data, size, conn_ack);
 
 	if (cci & UCSI_CCI_ERROR)
 		ret = ucsi_read_error(ucsi, connector_num);
@@ -242,9 +238,10 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd, bool conn_ack)
 	return ret;
 }
 
-int ucsi_send_command(struct ucsi *ucsi, u64 command)
+int ucsi_send_command(struct ucsi *ucsi, u64 command,
+		      void *data, size_t size)
 {
-	return ucsi_send_command_common(ucsi, command, false);
+	return ucsi_send_command_common(ucsi, command, data, size, false);
 }
 EXPORT_SYMBOL_GPL(ucsi_send_command);
 
@@ -322,17 +319,14 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
 	int i;
 
 	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(con->num);
-	con->ucsi->message_in_size = sizeof(cur);
-	ret = ucsi_send_command(con->ucsi, command);
+	ret = ucsi_send_command(con->ucsi, command, &cur, sizeof(cur));
 	if (ret < 0) {
 		if (con->ucsi->version > 0x0100) {
 			dev_err(con->ucsi->dev,
 				"GET_CURRENT_CAM command failed\n");
 			return;
 		}
 		cur = 0xff;
-	} else {
-		memcpy(&cur, con->ucsi->message_in, sizeof(cur));
 	}
 
 	if (cur < UCSI_MAX_ALTMODES)
@@ -516,8 +510,7 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
 		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
-		ucsi->message_in_size = sizeof(alt);
-		len = ucsi_send_command(con->ucsi, command);
+		len = ucsi_send_command(con->ucsi, command, &alt, sizeof(alt));
 		/*
 		 * We are collecting all altmodes first and then registering.
 		 * Some type-C device will return zero length data beyond last
@@ -526,8 +519,6 @@ ucsi_register_altmodes_nvidia(struct ucsi_connector *con, u8 recipient)
 		if (len < 0)
 			return len;
 
-		memcpy(&alt, ucsi->message_in, sizeof(alt));
-
 		/* We got all altmodes, now break out and register them */
 		if (!len || !alt.svid)
 			break;
@@ -595,15 +586,12 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
-		con->ucsi->message_in_size = sizeof(alt);
-		len = ucsi_send_command(con->ucsi, command);
+		len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt));
 		if (len == -EBUSY)
 			continue;
 		if (len <= 0)
 			return len;
 
-		memcpy(&alt, con->ucsi->message_in, sizeof(alt));
-
 		/*
 		 * This code is requesting one alt mode at a time, but some PPMs
 		 * may still return two. If that happens both alt modes need be
@@ -671,9 +659,7 @@ static int ucsi_get_connector_status(struct ucsi_connector *con, bool conn_ack)
 			  UCSI_MAX_DATA_LENGTH(con->ucsi));
 	int ret;
 
-	con->ucsi->message_in_size = size;
-	ret = ucsi_send_command_common(con->ucsi, command, conn_ack);
-	memcpy(&con->status, con->ucsi->message_in, size);
+	ret = ucsi_send_command_common(con->ucsi, command, &con->status, size, conn_ack);
 
 	return ret < 0 ? ret : 0;
 }
@@ -696,9 +682,8 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
 	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
 	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
 	command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
-	ucsi->message_in_size = num_pdos * sizeof(u32);
-	ret = ucsi_send_command(ucsi, command);
-	memcpy(pdos + offset, ucsi->message_in, num_pdos * sizeof(u32));
+	ret = ucsi_send_command(ucsi, command, pdos + offset,
+				num_pdos * sizeof(u32));
 	if (ret < 0 && ret != -ETIMEDOUT)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
 
@@ -785,9 +770,7 @@ static int ucsi_get_pd_message(struct ucsi_connector *con, u8 recipient,
 		command |= UCSI_GET_PD_MESSAGE_BYTES(len);
 		command |= UCSI_GET_PD_MESSAGE_TYPE(type);
 
-		con->ucsi->message_in_size = len;
-		ret = ucsi_send_command(con->ucsi, command);
-		memcpy(data + offset, con->ucsi->message_in, len);
+		ret = ucsi_send_command(con->ucsi, command, data + offset, len);
 		if (ret < 0)
 			return ret;
 	}
@@ -952,9 +935,7 @@ static int ucsi_register_cable(struct ucsi_connector *con)
 	int ret;
 
 	command = UCSI_GET_CABLE_PROPERTY | UCSI_CONNECTOR_NUMBER(con->num);
-	con->ucsi->message_in_size = sizeof(cable_prop);
-	ret = ucsi_send_command(con->ucsi, command);
-	memcpy(&cable_prop, con->ucsi->message_in, sizeof(cable_prop));
+	ret = ucsi_send_command(con->ucsi, command, &cable_prop, sizeof(cable_prop));
 	if (ret < 0) {
 		dev_err(con->ucsi->dev, "GET_CABLE_PROPERTY failed (%d)\n", ret);
 		return ret;
@@ -1015,9 +996,7 @@ static int ucsi_check_connector_capability(struct ucsi_connector *con)
 		return 0;
 
 	command = UCSI_GET_CONNECTOR_CAPABILITY | UCSI_CONNECTOR_NUMBER(con->num);
-	con->ucsi->message_in_size = sizeof(con->cap);
-	ret = ucsi_send_command(con->ucsi, command);
-	memcpy(&con->cap, con->ucsi->message_in, sizeof(con->cap));
+	ret = ucsi_send_command(con->ucsi, command, &con->cap, sizeof(con->cap));
 	if (ret < 0) {
 		dev_err(con->ucsi->dev, "GET_CONNECTOR_CAPABILITY failed (%d)\n", ret);
 		return ret;
@@ -1401,8 +1380,7 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
 	else if (con->ucsi->version >= UCSI_VERSION_2_0)
 		command |= hard ? 0 : UCSI_CONNECTOR_RESET_DATA_VER_2_0;
 
-	con->ucsi->message_in_size = 0;
-	return ucsi_send_command(con->ucsi, command);
+	return ucsi_send_command(con->ucsi, command, NULL, 0);
 }
 
 static int ucsi_reset_ppm(struct ucsi *ucsi)
@@ -1483,17 +1461,15 @@ static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
 {
 	int ret;
 
-	con->ucsi->message_in_size = 0;
-	ret = ucsi_send_command(con->ucsi, command);
+	ret = ucsi_send_command(con->ucsi, command, NULL, 0);
 	if (ret == -ETIMEDOUT) {
 		u64 c;
 
 		/* PPM most likely stopped responding. Resetting everything. */
 		ucsi_reset_ppm(con->ucsi);
 
 		c = UCSI_SET_NOTIFICATION_ENABLE | con->ucsi->ntfy;
-		con->ucsi->message_in_size = 0;
-		ucsi_send_command(con->ucsi, c);
+		ucsi_send_command(con->ucsi, c, NULL, 0);
 
 		ucsi_reset_connector(con, true);
 	}
@@ -1646,13 +1622,10 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
 	/* Get connector capability */
 	command = UCSI_GET_CONNECTOR_CAPABILITY;
 	command |= UCSI_CONNECTOR_NUMBER(con->num);
-	ucsi->message_in_size = sizeof(con->cap);
-	ret = ucsi_send_command(ucsi, command);
+	ret = ucsi_send_command(ucsi, command, &con->cap, sizeof(con->cap));
 	if (ret < 0)
 		goto out_unlock;
 
-	memcpy(&con->cap, ucsi->message_in, sizeof(con->cap));
-
 	if (UCSI_CONCAP(con, OPMODE_DRP))
 		cap->data = TYPEC_PORT_DRD;
 	else if (UCSI_CONCAP(con, OPMODE_DFP))
@@ -1849,20 +1822,17 @@ static int ucsi_init(struct ucsi *ucsi)
 	/* Enable basic notifications */
 	ntfy = UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
 	command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
-	ucsi->message_in_size = 0;
-	ret = ucsi_send_command(ucsi, command);
+	ret = ucsi_send_command(ucsi, command, NULL, 0);
 	if (ret < 0)
 		goto err_reset;
 
 	/* Get PPM capabilities */
 	command = UCSI_GET_CAPABILITY;
-	ucsi->message_in_size = BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE);
-	ret = ucsi_send_command(ucsi, command);
+	ret = ucsi_send_command(ucsi, command, &ucsi->cap,
+				BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE));
 	if (ret < 0)
 		goto err_reset;
 
-	memcpy(&ucsi->cap, ucsi->message_in, BITS_TO_BYTES(UCSI_GET_CAPABILITY_SIZE));
-
 	if (!ucsi->cap.num_connectors) {
 		ret = -ENODEV;
 		goto err_reset;
@@ -1892,8 +1862,7 @@ static int ucsi_init(struct ucsi *ucsi)
 	/* Enable all supported notifications */
 	ntfy = ucsi_get_supported_notifications(ucsi);
 	command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
-	ucsi->message_in_size = 0;
-	ret = ucsi_send_command(ucsi, command);
+	ret = ucsi_send_command(ucsi, command, NULL, 0);
 	if (ret < 0)
 		goto err_unregister;
 
@@ -1944,8 +1913,7 @@ static void ucsi_resume_work(struct work_struct *work)
 
 	/* Restore UCSI notification enable mask after system resume */
 	command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
-	ucsi->message_in_size = 0;
-	ret = ucsi_send_command(ucsi, command);
+	ret = ucsi_send_command(ucsi, command, NULL, 0);
 	if (ret < 0) {
 		dev_err(ucsi->dev, "failed to re-enable notifications (%d)\n", ret);
 		return;