usb: typec: ucsi: limit the PDO count to the number of PDOs requested

From: Seungjin Bae <[email protected]>

In ucsi_get_pdos(), the number of PDOs is derived from the data length reported in the CCI register, which is provided by the PPM firmware.

The function requests at most UCSI_MAX_PDOS PDOs on the first read and PDO_MAX_OBJECTS - UCSI_MAX_PDOS on the second, and ucsi_send_command() only copies that many bytes into the buffer. However, the returned length is taken from the 8 bit CCI data length field and is not bounded by the size of the request.

If a malicious PPM reports a larger length, e.g. 0xFF, each read is counted as 63 PDOs and ucsi_get_pdos() returns up to 126, beyond PDO_MAX_OBJECTS and the number of PDOs actually read.

ucsi_get_src_pdos() stores this value in con->num_pdos, and ucsi_psy_get_voltage_max() and ucsi_psy_get_current_max() use it to index con->src_pdos[con->num_pdos - 1], resulting in an out-of-bounds read. This happens without any userspace action, since ucsi_get_src_pdos() calls ucsi_port_psy_changed() and the resulting uevent reads every property.

Fix this by limiting the count of each read to the number of PDOs requested, so that the returned value always matches the buffer contents and never exceeds PDO_MAX_OBJECTS.

Fixes b04e1747fbcc ("usb: typec: ucsi: Register USB Power Delivery Capabilities")

Cc [email protected]

Signed-off-by Seungjin Bae <[email protected]>

---
 drivers/usb/typec/ucsi/ucsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/usb/typec/ucsi/ucsi.c +4 -2

index bef3f9b71d71..639f99f49ff9 100644
@@ -898,7 +898,8 @@ static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
 	if (ret < 0)
 		return ret;
 
-	num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
+	/* The PPM may report more data than was requested */
+	num_pdos = min_t(u8, ret / sizeof(u32), UCSI_MAX_PDOS);
 	if (num_pdos < UCSI_MAX_PDOS)
 		return num_pdos;
 

@@ -908,7 +909,8 @@ static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
 	if (ret < 0)
 		return ret;
 
-	return ret / sizeof(u32) + num_pdos;
+	return min_t(u8, ret / sizeof(u32),
+		     PDO_MAX_OBJECTS - UCSI_MAX_PDOS) + num_pdos;
 }
 
 static int ucsi_get_src_pdos(struct ucsi_connector *con)

Signature · 1 line

2.43.0

From: Seungjin Bae <[email protected]>

In ucsi_get_pdos(), the number of PDOs is derived from the data length reported in the CCI register, which is provided by the PPM firmware.

The function requests at most UCSI_MAX_PDOS PDOs on the first read and PDO_MAX_OBJECTS - UCSI_MAX_PDOS on the second, and ucsi_send_command() only copies that many bytes into the buffer. However, the returned length is taken from the 8 bit CCI data length field and is not bounded by the size of the request.

If a malicious PPM reports a larger length, e.g. 0xFF, each read is counted as 63 PDOs and ucsi_get_pdos() returns up to 126, beyond PDO_MAX_OBJECTS and the number of PDOs actually read.

ucsi_get_src_pdos() stores this value in con->num_pdos, and ucsi_psy_get_voltage_max() and ucsi_psy_get_current_max() use it to index con->src_pdos[con->num_pdos - 1], resulting in an out-of-bounds read. This happens without any userspace action, since ucsi_get_src_pdos() calls ucsi_port_psy_changed() and the resulting uevent reads every property.

Fix this by limiting the count of each read to the number of PDOs requested, so that the returned value always matches the buffer contents and never exceeds PDO_MAX_OBJECTS.

Fixes b04e1747fbcc ("usb: typec: ucsi: Register USB Power Delivery Capabilities")

Cc [email protected]

Signed-off-by Seungjin Bae <[email protected]>

---
 drivers/usb/typec/ucsi/ucsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

From: Seungjin Bae <[email protected]>

In ucsi_get_pdos(), the number of PDOs is derived from the data length reported in the CCI register, which is provided by the PPM firmware.

The function requests at most UCSI_MAX_PDOS PDOs on the first read and PDO_MAX_OBJECTS - UCSI_MAX_PDOS on the second, and ucsi_send_command() only copies that many bytes into the buffer. However, the returned length is taken from the 8 bit CCI data length field and is not bounded by the size of the request.

If a malicious PPM reports a larger length, e.g. 0xFF, each read is counted as 63 PDOs and ucsi_get_pdos() returns up to 126, beyond PDO_MAX_OBJECTS and the number of PDOs actually read.

ucsi_get_src_pdos() stores this value in con->num_pdos, and ucsi_psy_get_voltage_max() and ucsi_psy_get_current_max() use it to index con->src_pdos[con->num_pdos - 1], resulting in an out-of-bounds read. This happens without any userspace action, since ucsi_get_src_pdos() calls ucsi_port_psy_changed() and the resulting uevent reads every property.

Reject such responses in ucsi_read_pdos(), if the PPM reports more data than was requested, print an error and return -EPROTO, so that the invalid count is never used.

Fixes b04e1747fbcc ("usb: typec: ucsi: Register USB Power Delivery Capabilities")

Cc [email protected]

Assisted-by LLM

Signed-off-by Seungjin Bae <[email protected]>

---
v1 -> v2: Print an error and fail instead of limiting the count, as suggested by Heikki.

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

drivers/usb/typec/ucsi/ucsi.c +9 -2

index bef3f9b71d71..17beec16fe19 100644
@@ -862,6 +862,7 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
 			  u32 *pdos, int offset, int num_pdos)
 {
 	struct ucsi *ucsi = con->ucsi;
+	int len = num_pdos * sizeof(u32);
 	u64 command;
 	int ret;
 

@@ -875,11 +876,17 @@ 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;
-	ret = ucsi_send_command(ucsi, command, pdos + offset,
-				num_pdos * sizeof(u32));
+	ret = ucsi_send_command(ucsi, command, pdos + offset, len);
 	if (ret < 0 && ret != -ETIMEDOUT)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
 
+	if (ret > len) {
+		dev_err(ucsi->dev,
+			"PPM returned %d bytes of PDOs, but only %d were requested\n",
+			ret, len);
+		return -EPROTO;
+	}
+
 	return ret;
 }
 

Signature · 1 line

2.43.0