Skip to contentSkip to conversation

usb: typec: ucsi: Retrieve all the PDOs instead of just the first 4

PATCH Superseded Jack Pham 3 participants 7 messages Apr 26, 2021 – May 3, 2021

Subsystems (3)
Stable branch USB USB Type-C class

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Jack Pham

author
#0 lore raw

PATCH 1/1 usb: typec: ucsi: Retrieve all the PDOs instead of just the first 4

commit 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode") introduced retrieval of the PDOs when connected to a PD-capable source. But only the first 4 PDOs are received since that is the maximum number that can be fetched at a time given the MESSAGE_IN length limitation (16 bytes). However, as per the PD spec a connected source may advertise up to a maximum of 7 PDOs.

If such a source is connected it's possible the UCSI FW could have negotiated a power contract with one of the PDOs at index greater than 4, and would be reflected in the request data object's (RDO) object position field. This would result in an out-of-bounds access when the rdo_index() is used to index into the src_pdos array in ucsi_psy_get_voltage_now().

We can resolve this by instead retrieving and storing up to the maximum of 7 PDOs in the con->src_pdos array. This would involve two calls to the GET_PDOS command.

Fixes 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
Fixes 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
Cc [email protected]
Signed-off-by Jack Pham <[email protected]>
---
 drivers/usb/typec/ucsi/ucsi.c | 41 +++++++++++++++++++++++++++--------
 drivers/usb/typec/ucsi/ucsi.h |  6 +++--
 2 files changed, 36 insertions(+), 11 deletions(-)
drivers/usb/typec/ucsi/ucsi.c +32 -9
index 244270755ae6..ac214b855986 100644
@@ -495,7 +495,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 	}
 }
 
-static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
+static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
+		u32 *pdos, int offset, int num_pdos)
 {
 	struct ucsi *ucsi = con->ucsi;
 	u64 command;

@@ -503,17 +504,39 @@ static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
 
 	command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
 	command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
-	command |= UCSI_GET_PDOS_NUM_PDOS(UCSI_MAX_PDOS - 1);
+	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
+	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
 	command |= UCSI_GET_PDOS_SRC_PDOS;
-	ret = ucsi_send_command(ucsi, command, con->src_pdos,
-			       sizeof(con->src_pdos));
-	if (ret < 0) {
+	ret = ucsi_send_command(ucsi, command, pdos + offset,
+			num_pdos * sizeof(u32));
+	if (ret < 0)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
+	if (ret == 0 && offset == 0)
+		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
+
+	return ret;
+}
+
+static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
+{
+	int ret;
+
+	/* UCSI max payload means only getting at most 4 PDOs at a time */
+	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
+	if (ret < 0)
 		return;
-	}
+
 	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
-	if (ret == 0)
-		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
+	if (con->num_pdos < UCSI_MAX_PDOS)
+		return;
+
+	/* get the remaining PDOs, if any */
+	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
+			PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
+	if (ret < 0)
+		return;
+
+	con->num_pdos += ret / sizeof(u32);
 }
 
 static void ucsi_pwr_opmode_change(struct ucsi_connector *con)

@@ -522,7 +545,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 	case UCSI_CONSTAT_PWR_OPMODE_PD:
 		con->rdo = con->status.request_data_obj;
 		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
-		ucsi_get_pdos(con, 1);
+		ucsi_get_src_pdos(con, 1);
 		break;
 	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
 		con->rdo = 0;
drivers/usb/typec/ucsi/ucsi.h +4 -2
index 3920e20a9e9e..cee666790907 100644
@@ -8,6 +8,7 @@
 #include <linux/power_supply.h>
 #include <linux/types.h>
 #include <linux/usb/typec.h>
+#include <linux/usb/pd.h>
 #include <linux/usb/role.h>
 
 /* -------------------------------------------------------------------------- */

@@ -134,7 +135,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 
 /* GET_PDOS command bits */
 #define UCSI_GET_PDOS_PARTNER_PDO(_r_)		((u64)(_r_) << 23)
+#define UCSI_GET_PDOS_PDO_OFFSET(_r_)		((u64)(_r_) << 24)
 #define UCSI_GET_PDOS_NUM_PDOS(_r_)		((u64)(_r_) << 32)
+#define UCSI_MAX_PDOS				(4)
 #define UCSI_GET_PDOS_SRC_PDOS			((u64)1 << 34)
 
 /* -------------------------------------------------------------------------- */

@@ -302,7 +305,6 @@ struct ucsi {
 
 #define UCSI_MAX_SVID		5
 #define UCSI_MAX_ALTMODES	(UCSI_MAX_SVID * 6)
-#define UCSI_MAX_PDOS		(4)
 
 #define UCSI_TYPEC_VSAFE5V	5000
 #define UCSI_TYPEC_1_5_CURRENT	1500

@@ -330,7 +332,7 @@ struct ucsi_connector {
 	struct power_supply *psy;
 	struct power_supply_desc psy_desc;
 	u32 rdo;
-	u32 src_pdos[UCSI_MAX_PDOS];
+	u32 src_pdos[PDO_MAX_OBJECTS];
 	int num_pdos;
 
 	struct usb_role_switch *usb_role_sw;
Signature · 1 line
2.24.0
author
#1 lore raw

Forgot to Cc: Abhilash who introduced the PDO code

Quoted 139 lines · Jack Pham

commit 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode") introduced retrieval of the PDOs when connected to a PD-capable source. But only the first 4 PDOs are received since that is the maximum number that can be fetched at a time given the MESSAGE_IN length limitation (16 bytes). However, as per the PD spec a connected source may advertise up to a maximum of 7 PDOs.

If such a source is connected it's possible the UCSI FW could have negotiated a power contract with one of the PDOs at index greater than 4, and would be reflected in the request data object's (RDO) object position field. This would result in an out-of-bounds access when the rdo_index() is used to index into the src_pdos array in ucsi_psy_get_voltage_now().

We can resolve this by instead retrieving and storing up to the maximum of 7 PDOs in the con->src_pdos array. This would involve two calls to the GET_PDOS command.

Fixes 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
Fixes 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
Cc [email protected]
Signed-off-by Jack Pham <[email protected]>
---
 drivers/usb/typec/ucsi/ucsi.c | 41 +++++++++++++++++++++++++++--------
 drivers/usb/typec/ucsi/ucsi.h |  6 +++--
 2 files changed, 36 insertions(+), 11 deletions(-)
drivers/usb/typec/ucsi/ucsi.c +32 -9
index 244270755ae6..ac214b855986 100644
@@ -495,7 +495,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 	}
 }
 
-static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
+static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
+		u32 *pdos, int offset, int num_pdos)
 {
 	struct ucsi *ucsi = con->ucsi;
 	u64 command;

@@ -503,17 +504,39 @@ static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
 
 	command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
 	command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
-	command |= UCSI_GET_PDOS_NUM_PDOS(UCSI_MAX_PDOS - 1);
+	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
+	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
 	command |= UCSI_GET_PDOS_SRC_PDOS;
-	ret = ucsi_send_command(ucsi, command, con->src_pdos,
-			       sizeof(con->src_pdos));
-	if (ret < 0) {
+	ret = ucsi_send_command(ucsi, command, pdos + offset,
+			num_pdos * sizeof(u32));
+	if (ret < 0)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
+	if (ret == 0 && offset == 0)
+		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
+
+	return ret;
+}
+
+static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
+{
+	int ret;
+
+	/* UCSI max payload means only getting at most 4 PDOs at a time */
+	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
+	if (ret < 0)
 		return;
-	}
+
 	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
-	if (ret == 0)
-		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
+	if (con->num_pdos < UCSI_MAX_PDOS)
+		return;
+
+	/* get the remaining PDOs, if any */
+	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
+			PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
+	if (ret < 0)
+		return;
+
+	con->num_pdos += ret / sizeof(u32);
 }
 
 static void ucsi_pwr_opmode_change(struct ucsi_connector *con)

@@ -522,7 +545,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 	case UCSI_CONSTAT_PWR_OPMODE_PD:
 		con->rdo = con->status.request_data_obj;
 		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
-		ucsi_get_pdos(con, 1);
+		ucsi_get_src_pdos(con, 1);
 		break;
 	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
 		con->rdo = 0;
drivers/usb/typec/ucsi/ucsi.h +4 -2
index 3920e20a9e9e..cee666790907 100644
@@ -8,6 +8,7 @@
 #include <linux/power_supply.h>
 #include <linux/types.h>
 #include <linux/usb/typec.h>
+#include <linux/usb/pd.h>
 #include <linux/usb/role.h>
 
 /* -------------------------------------------------------------------------- */

@@ -134,7 +135,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 
 /* GET_PDOS command bits */
 #define UCSI_GET_PDOS_PARTNER_PDO(_r_)		((u64)(_r_) << 23)
+#define UCSI_GET_PDOS_PDO_OFFSET(_r_)		((u64)(_r_) << 24)
 #define UCSI_GET_PDOS_NUM_PDOS(_r_)		((u64)(_r_) << 32)
+#define UCSI_MAX_PDOS				(4)
 #define UCSI_GET_PDOS_SRC_PDOS			((u64)1 << 34)
 
 /* -------------------------------------------------------------------------- */

@@ -302,7 +305,6 @@ struct ucsi {
 
 #define UCSI_MAX_SVID		5
 #define UCSI_MAX_ALTMODES	(UCSI_MAX_SVID * 6)
-#define UCSI_MAX_PDOS		(4)
 
 #define UCSI_TYPEC_VSAFE5V	5000
 #define UCSI_TYPEC_1_5_CURRENT	1500

@@ -330,7 +332,7 @@ struct ucsi_connector {
 	struct power_supply *psy;
 	struct power_supply_desc psy_desc;
 	u32 rdo;
-	u32 src_pdos[UCSI_MAX_PDOS];
+	u32 src_pdos[PDO_MAX_OBJECTS];
 	int num_pdos;
 
 	struct usb_role_switch *usb_role_sw;
Signature · 1 line
2.24.0
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#2 lore raw
Quoted 1 line

If such a source is connected it's possible the UCSI FW could have

s/UCSI FW/PPM

Quoted 4 lines

We can resolve this by instead retrieving and storing up to the maximum of 7 PDOs in the con->src_pdos array. This would involve two calls to the GET_PDOS command.

This issue (see the signature below) is found by enabling UBSAN and connecting a charger adapter that can advertise 5 PDOs and RPDO selected by PPM is 5.

[  151.545106][   T70] Unexpected kernel BRK exception at EL1
[  151.545112][   T70] Internal error: BRK handler: f2005512 [#1] PREEMPT SMP
...
[  151.545499][   T70] pc : ucsi_psy_get_prop+0x208/0x20c
[  151.545507][   T70] lr : power_supply_show_property+0xc0/0x328
...
[  151.545542][   T70] Call trace:
[  151.545544][   T70]  ucsi_psy_get_prop+0x208/0x20c
[  151.545546][   T70]  power_supply_uevent+0x1a4/0x2f0
[  151.545550][   T70]  dev_uevent+0x200/0x384
[  151.545555][   T70]  kobject_uevent_env+0x1d4/0x7e8
[  151.545557][   T70]  power_supply_changed_work+0x174/0x31c
[  151.545562][   T70]  process_one_work+0x244/0x6f0
[  151.545564][   T70]  worker_thread+0x3e0/0xa64
Quoted 2 lines
Fixes 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
Fixes 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
author
#3 lore raw
Quoted 3 lines · Subbaraman Narayanamurthy
Quoted 1 line

If such a source is connected it's possible the UCSI FW could have

s/UCSI FW/PPM

Right, PPM is a more apt descriptor. Waiting to see if Heikki has any feedback first before sending a V2.

Quoted 9 lines
Quoted 4 lines

We can resolve this by instead retrieving and storing up to the maximum of 7 PDOs in the con->src_pdos array. This would involve two calls to the GET_PDOS command.

This issue (see the signature below) is found by enabling UBSAN and connecting a charger adapter that can advertise 5 PDOs and RPDO selected by PPM is 5.

Quoted 14 lines
[  151.545106][   T70] Unexpected kernel BRK exception at EL1
[  151.545112][   T70] Internal error: BRK handler: f2005512 [#1] PREEMPT SMP
...
[  151.545499][   T70] pc : ucsi_psy_get_prop+0x208/0x20c
[  151.545507][   T70] lr : power_supply_show_property+0xc0/0x328
...
[  151.545542][   T70] Call trace:
[  151.545544][   T70]  ucsi_psy_get_prop+0x208/0x20c
[  151.545546][   T70]  power_supply_uevent+0x1a4/0x2f0
[  151.545550][   T70]  dev_uevent+0x200/0x384
[  151.545555][   T70]  kobject_uevent_env+0x1d4/0x7e8
[  151.545557][   T70]  power_supply_changed_work+0x174/0x31c
[  151.545562][   T70]  process_one_work+0x244/0x6f0
[  151.545564][   T70]  worker_thread+0x3e0/0xa64

UBSAN FTW. Want me to copy this trace into the commit text as well?

Jack

Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#4 lore raw
Quoted 1 line

UBSAN FTW. Want me to copy this trace into the commit text as well?

Yes, I think it would be good as this issue is found by enabling UBSAN. Also, it gives the context that by inserting a PD charger adapter that can support more than 4 PDOs, device can crash because of an OOB access.

-Subbaraman

maintainer Reviewed-by
#5 lore raw
In reply to Jack Pham

Hi,

Quoted 20 lines · Jack Pham

Forgot to Cc: Abhilash who introduced the PDO code

Quoted 17 lines · Jack Pham

commit 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode") introduced retrieval of the PDOs when connected to a PD-capable source. But only the first 4 PDOs are received since that is the maximum number that can be fetched at a time given the MESSAGE_IN length limitation (16 bytes). However, as per the PD spec a connected source may advertise up to a maximum of 7 PDOs.

If such a source is connected it's possible the UCSI FW could have negotiated a power contract with one of the PDOs at index greater than 4, and would be reflected in the request data object's (RDO) object position field. This would result in an out-of-bounds access when the rdo_index() is used to index into the src_pdos array in ucsi_psy_get_voltage_now().

We can resolve this by instead retrieving and storing up to the maximum of 7 PDOs in the con->src_pdos array. This would involve two calls to the GET_PDOS command.

This makes sense to me. A few nitpicks below. Besides those:

Reviewed-by Heikki Krogerus <[email protected]>
Quoted 17 lines
Quoted 17 lines
Fixes 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
Fixes 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
Cc [email protected]
Signed-off-by Jack Pham <[email protected]>
---
 drivers/usb/typec/ucsi/ucsi.c | 41 +++++++++++++++++++++++++++--------
 drivers/usb/typec/ucsi/ucsi.h |  6 +++--
 2 files changed, 36 insertions(+), 11 deletions(-)
drivers/usb/typec/ucsi/ucsi.c +0 -0
index 244270755ae6..ac214b855986 100644
@@ -495,7 +495,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 	}
 }
Quoted 3 lines

drivers/usb/typec/ucsi/ucsi.c static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient) · line 499

Quoted 3 lines
drivers/usb/typec/ucsi/ucsi.c +2 -1
-static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
+static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
+		u32 *pdos, int offset, int num_pdos)

That should be aligned properly.

Quoted 5 lines
Quoted 5 lines
drivers/usb/typec/ucsi/ucsi.c +0 -0
 {
 	struct ucsi *ucsi = con->ucsi;
 	u64 command;

@@ -503,17 +504,39 @@ static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
Quoted 11 lines

drivers/usb/typec/ucsi/ucsi.c static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner) · line 511

Quoted 11 lines
drivers/usb/typec/ucsi/ucsi.c +4 -4
 	command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
 	command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
-	command |= UCSI_GET_PDOS_NUM_PDOS(UCSI_MAX_PDOS - 1);
+	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
+	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
 	command |= UCSI_GET_PDOS_SRC_PDOS;
-	ret = ucsi_send_command(ucsi, command, con->src_pdos,
-			       sizeof(con->src_pdos));
-	if (ret < 0) {
+	ret = ucsi_send_command(ucsi, command, pdos + offset,
+			num_pdos * sizeof(u32));

That too. I think if you run scripts/checkpatch.pl it will complain about those.

Quoted 81 lines
Quoted 81 lines
drivers/usb/typec/ucsi/ucsi.c +26 -4
+	if (ret < 0)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
+	if (ret == 0 && offset == 0)
+		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
+
+	return ret;
+}
+
+static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
+{
+	int ret;
+
+	/* UCSI max payload means only getting at most 4 PDOs at a time */
+	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
+	if (ret < 0)
 		return;
-	}
+
 	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
-	if (ret == 0)
-		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
+	if (con->num_pdos < UCSI_MAX_PDOS)
+		return;
+
+	/* get the remaining PDOs, if any */
+	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
+			PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
+	if (ret < 0)
+		return;
+
+	con->num_pdos += ret / sizeof(u32);
 }
 
 static void ucsi_pwr_opmode_change(struct ucsi_connector *con)

@@ -522,7 +545,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 	case UCSI_CONSTAT_PWR_OPMODE_PD:
 		con->rdo = con->status.request_data_obj;
 		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
-		ucsi_get_pdos(con, 1);
+		ucsi_get_src_pdos(con, 1);
 		break;
 	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
 		con->rdo = 0;
drivers/usb/typec/ucsi/ucsi.h +4 -2
index 3920e20a9e9e..cee666790907 100644
@@ -8,6 +8,7 @@
 #include <linux/power_supply.h>
 #include <linux/types.h>
 #include <linux/usb/typec.h>
+#include <linux/usb/pd.h>
 #include <linux/usb/role.h>
 
 /* -------------------------------------------------------------------------- */

@@ -134,7 +135,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 
 /* GET_PDOS command bits */
 #define UCSI_GET_PDOS_PARTNER_PDO(_r_)		((u64)(_r_) << 23)
+#define UCSI_GET_PDOS_PDO_OFFSET(_r_)		((u64)(_r_) << 24)
 #define UCSI_GET_PDOS_NUM_PDOS(_r_)		((u64)(_r_) << 32)
+#define UCSI_MAX_PDOS				(4)
 #define UCSI_GET_PDOS_SRC_PDOS			((u64)1 << 34)
 
 /* -------------------------------------------------------------------------- */

@@ -302,7 +305,6 @@ struct ucsi {
 
 #define UCSI_MAX_SVID		5
 #define UCSI_MAX_ALTMODES	(UCSI_MAX_SVID * 6)
-#define UCSI_MAX_PDOS		(4)
 
 #define UCSI_TYPEC_VSAFE5V	5000
 #define UCSI_TYPEC_1_5_CURRENT	1500

@@ -330,7 +332,7 @@ struct ucsi_connector {
 	struct power_supply *psy;
 	struct power_supply_desc psy_desc;
 	u32 rdo;
-	u32 src_pdos[UCSI_MAX_PDOS];
+	u32 src_pdos[PDO_MAX_OBJECTS];
 	int num_pdos;
Quoted 1 line

drivers/usb/typec/ucsi/ucsi.h struct ucsi_connector { · line 338

Quoted 1 line
drivers/usb/typec/ucsi/ucsi.h +0 -0
 	struct usb_role_switch *usb_role_sw;

thanks,

Signature · 1 line
heikki
author
#6 lore raw
Quoted 18 lines · Heikki Krogerus
Quoted 17 lines · Jack Pham

Forgot to Cc: Abhilash who introduced the PDO code

Quoted 14 lines · Jack Pham

commit 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode") introduced retrieval of the PDOs when connected to a PD-capable source. But only the first 4 PDOs are received since that is the maximum number that can be fetched at a time given the MESSAGE_IN length limitation (16 bytes). However, as per the PD spec a connected source may advertise up to a maximum of 7 PDOs.

If such a source is connected it's possible the UCSI FW could have negotiated a power contract with one of the PDOs at index greater than 4, and would be reflected in the request data object's (RDO) object position field. This would result in an out-of-bounds access when the rdo_index() is used to index into the src_pdos array in ucsi_psy_get_voltage_now().

Quoted 7 lines
Quoted 3 lines
Quoted 3 lines

We can resolve this by instead retrieving and storing up to the maximum of 7 PDOs in the con->src_pdos array. This would involve two calls to the GET_PDOS command.

This makes sense to me. A few nitpicks below. Besides those:

Reviewed-by Heikki Krogerus <[email protected]>

Thanks Heikki! I'll send a V2 along with the commit text improvements Subbaraman had suggested.

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Fixes 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
Fixes 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
Cc [email protected]
Signed-off-by Jack Pham <[email protected]>
---
 drivers/usb/typec/ucsi/ucsi.c | 41 +++++++++++++++++++++++++++--------
 drivers/usb/typec/ucsi/ucsi.h |  6 +++--
 2 files changed, 36 insertions(+), 11 deletions(-)
drivers/usb/typec/ucsi/ucsi.c +0 -0
index 244270755ae6..ac214b855986 100644
@@ -495,7 +495,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 	}
 }
Quoted 5 lines
Quoted 3 lines

drivers/usb/typec/ucsi/ucsi.c static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient) · line 499

Quoted 3 lines
drivers/usb/typec/ucsi/ucsi.c +2 -1
-static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
+static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
+		u32 *pdos, int offset, int num_pdos)

That should be aligned properly.

Will do.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/usb/typec/ucsi/ucsi.c +0 -0
 {
 	struct ucsi *ucsi = con->ucsi;
 	u64 command;

@@ -503,17 +504,39 @@ static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
Quoted 14 lines
Quoted 11 lines

drivers/usb/typec/ucsi/ucsi.c static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner) · line 511

Quoted 11 lines
drivers/usb/typec/ucsi/ucsi.c +4 -4
 	command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
 	command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
-	command |= UCSI_GET_PDOS_NUM_PDOS(UCSI_MAX_PDOS - 1);
+	command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
+	command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
 	command |= UCSI_GET_PDOS_SRC_PDOS;
-	ret = ucsi_send_command(ucsi, command, con->src_pdos,
-			       sizeof(con->src_pdos));
-	if (ret < 0) {
+	ret = ucsi_send_command(ucsi, command, pdos + offset,
+			num_pdos * sizeof(u32));

That too. I think if you run scripts/checkpatch.pl it will complain about those.

Got it.

Actually checkpatch didn't warn about this. But I'll align to follow the style in the rest of this file.

Quoted 81 lines
Quoted 81 lines
Quoted 81 lines
drivers/usb/typec/ucsi/ucsi.c +26 -4
+	if (ret < 0)
 		dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
+	if (ret == 0 && offset == 0)
+		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
+
+	return ret;
+}
+
+static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
+{
+	int ret;
+
+	/* UCSI max payload means only getting at most 4 PDOs at a time */
+	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
+	if (ret < 0)
 		return;
-	}
+
 	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
-	if (ret == 0)
-		dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
+	if (con->num_pdos < UCSI_MAX_PDOS)
+		return;
+
+	/* get the remaining PDOs, if any */
+	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
+			PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
+	if (ret < 0)
+		return;
+
+	con->num_pdos += ret / sizeof(u32);
 }
 
 static void ucsi_pwr_opmode_change(struct ucsi_connector *con)

@@ -522,7 +545,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 	case UCSI_CONSTAT_PWR_OPMODE_PD:
 		con->rdo = con->status.request_data_obj;
 		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
-		ucsi_get_pdos(con, 1);
+		ucsi_get_src_pdos(con, 1);
 		break;
 	case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
 		con->rdo = 0;
drivers/usb/typec/ucsi/ucsi.h +4 -2
index 3920e20a9e9e..cee666790907 100644
@@ -8,6 +8,7 @@
 #include <linux/power_supply.h>
 #include <linux/types.h>
 #include <linux/usb/typec.h>
+#include <linux/usb/pd.h>
 #include <linux/usb/role.h>
 
 /* -------------------------------------------------------------------------- */

@@ -134,7 +135,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 
 /* GET_PDOS command bits */
 #define UCSI_GET_PDOS_PARTNER_PDO(_r_)		((u64)(_r_) << 23)
+#define UCSI_GET_PDOS_PDO_OFFSET(_r_)		((u64)(_r_) << 24)
 #define UCSI_GET_PDOS_NUM_PDOS(_r_)		((u64)(_r_) << 32)
+#define UCSI_MAX_PDOS				(4)
 #define UCSI_GET_PDOS_SRC_PDOS			((u64)1 << 34)
 
 /* -------------------------------------------------------------------------- */

@@ -302,7 +305,6 @@ struct ucsi {
 
 #define UCSI_MAX_SVID		5
 #define UCSI_MAX_ALTMODES	(UCSI_MAX_SVID * 6)
-#define UCSI_MAX_PDOS		(4)
 
 #define UCSI_TYPEC_VSAFE5V	5000
 #define UCSI_TYPEC_1_5_CURRENT	1500

@@ -330,7 +332,7 @@ struct ucsi_connector {
 	struct power_supply *psy;
 	struct power_supply_desc psy_desc;
 	u32 rdo;
-	u32 src_pdos[UCSI_MAX_PDOS];
+	u32 src_pdos[PDO_MAX_OBJECTS];
 	int num_pdos;
Quoted 1 line
Quoted 1 line

drivers/usb/typec/ucsi/ucsi.h struct ucsi_connector { · line 338

Quoted 1 line
drivers/usb/typec/ucsi/ucsi.h +0 -0
 	struct usb_role_switch *usb_role_sw;

Thanks,
Jack

Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project