Off-by-one errors happen because nr_snk_pdo and nr_src_pdo are incorrectly added one. The index of the loop is equal to the number of PDOs to be updated when leaving the loop and it doesn't need to be added one.
When doing the power negotiation, TCPM relies on the "nr_snk_pdo" as the size of the local sink PDO array to match the Source capabilities of the partner port. If the off-by-one overflow occurs, a wrong RDO might be sent and unexpected power transfer might happen such as over voltage or over current (than expected).
"nr_src_pdo" is used to set the Rp level when the port is in Source role. It is also the array size of the local Source capabilities when filling up the buffer which will be sent as the Source PDOs (such as in Power Negotiation). If the off-by-one overflow occurs, a wrong Rp level might be set and wrong Source PDOs will be sent to the partner port. This could potentially cause over current or port resets.
Fixes: cd099cde4ed2 ("usb: typec: tcpm: Support multiple capabilities") Cc: stable@vger.kernel.org Signed-off-by: Kyle Tso kyletso@google.com --- v1 -> v2: - update the commit message (adding the problems this patch solves)
drivers/usb/typec/tcpm/tcpm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index ae2b6c94482d..2464710ea0c8 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -6855,14 +6855,14 @@ static int tcpm_pd_set(struct typec_port *p, struct usb_power_delivery *pd) if (data->sink_desc.pdo[0]) { for (i = 0; i < PDO_MAX_OBJECTS && data->sink_desc.pdo[i]; i++) port->snk_pdo[i] = data->sink_desc.pdo[i]; - port->nr_snk_pdo = i + 1; + port->nr_snk_pdo = i; port->operating_snk_mw = data->operating_snk_mw; }
if (data->source_desc.pdo[0]) { for (i = 0; i < PDO_MAX_OBJECTS && data->source_desc.pdo[i]; i++) port->snk_pdo[i] = data->source_desc.pdo[i]; - port->nr_src_pdo = i + 1; + port->nr_src_pdo = i; }
switch (port->state) {
On Tue, Mar 26, 2024 at 11:19:09PM +0800, Kyle Tso wrote:
Off-by-one errors happen because nr_snk_pdo and nr_src_pdo are incorrectly added one. The index of the loop is equal to the number of PDOs to be updated when leaving the loop and it doesn't need to be added one.
When doing the power negotiation, TCPM relies on the "nr_snk_pdo" as the size of the local sink PDO array to match the Source capabilities of the partner port. If the off-by-one overflow occurs, a wrong RDO might be sent and unexpected power transfer might happen such as over voltage or over current (than expected).
"nr_src_pdo" is used to set the Rp level when the port is in Source role. It is also the array size of the local Source capabilities when filling up the buffer which will be sent as the Source PDOs (such as in Power Negotiation). If the off-by-one overflow occurs, a wrong Rp level might be set and wrong Source PDOs will be sent to the partner port. This could potentially cause over current or port resets.
Fixes: cd099cde4ed2 ("usb: typec: tcpm: Support multiple capabilities") Cc: stable@vger.kernel.org Signed-off-by: Kyle Tso kyletso@google.com
Reviewed-by: Heikki Krogerus heikki.krogerus@linux.intel.com
v1 -> v2:
- update the commit message (adding the problems this patch solves)
drivers/usb/typec/tcpm/tcpm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index ae2b6c94482d..2464710ea0c8 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -6855,14 +6855,14 @@ static int tcpm_pd_set(struct typec_port *p, struct usb_power_delivery *pd) if (data->sink_desc.pdo[0]) { for (i = 0; i < PDO_MAX_OBJECTS && data->sink_desc.pdo[i]; i++) port->snk_pdo[i] = data->sink_desc.pdo[i];
port->nr_snk_pdo = i + 1;
port->nr_snk_pdo = i;if (data->source_desc.pdo[0]) { for (i = 0; i < PDO_MAX_OBJECTS && data->source_desc.pdo[i]; i++) port->snk_pdo[i] = data->source_desc.pdo[i];
port->nr_src_pdo = i + 1;
port->nr_src_pdo = i;switch (port->state) { -- 2.44.0.396.g6e790dbe36-goog
On Tue, Mar 26, 2024 at 11:19:09PM +0800, Kyle Tso wrote:
Off-by-one errors happen because nr_snk_pdo and nr_src_pdo are incorrectly added one. The index of the loop is equal to the number of PDOs to be updated when leaving the loop and it doesn't need to be added one.
When doing the power negotiation, TCPM relies on the "nr_snk_pdo" as the size of the local sink PDO array to match the Source capabilities of the partner port. If the off-by-one overflow occurs, a wrong RDO might be sent and unexpected power transfer might happen such as over voltage or over current (than expected).
"nr_src_pdo" is used to set the Rp level when the port is in Source role. It is also the array size of the local Source capabilities when filling up the buffer which will be sent as the Source PDOs (such as in Power Negotiation). If the off-by-one overflow occurs, a wrong Rp level might be set and wrong Source PDOs will be sent to the partner port. This could potentially cause over current or port resets.
Fixes: cd099cde4ed2 ("usb: typec: tcpm: Support multiple capabilities") Cc: stable@vger.kernel.org Signed-off-by: Kyle Tso kyletso@google.com
v1 -> v2:
- update the commit message (adding the problems this patch solves)
drivers/usb/typec/tcpm/tcpm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
This fails to apply to my usb-linus branch :(
Can you rebase and resend?
thanks,
greg k-h
On Thu, Apr 4, 2024 at 9:22 PM Greg KH gregkh@linuxfoundation.org wrote:
On Tue, Mar 26, 2024 at 11:19:09PM +0800, Kyle Tso wrote:
Off-by-one errors happen because nr_snk_pdo and nr_src_pdo are incorrectly added one. The index of the loop is equal to the number of PDOs to be updated when leaving the loop and it doesn't need to be added one.
When doing the power negotiation, TCPM relies on the "nr_snk_pdo" as the size of the local sink PDO array to match the Source capabilities of the partner port. If the off-by-one overflow occurs, a wrong RDO might be sent and unexpected power transfer might happen such as over voltage or over current (than expected).
"nr_src_pdo" is used to set the Rp level when the port is in Source role. It is also the array size of the local Source capabilities when filling up the buffer which will be sent as the Source PDOs (such as in Power Negotiation). If the off-by-one overflow occurs, a wrong Rp level might be set and wrong Source PDOs will be sent to the partner port. This could potentially cause over current or port resets.
Fixes: cd099cde4ed2 ("usb: typec: tcpm: Support multiple capabilities") Cc: stable@vger.kernel.org Signed-off-by: Kyle Tso kyletso@google.com
v1 -> v2:
- update the commit message (adding the problems this patch solves)
drivers/usb/typec/tcpm/tcpm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
This fails to apply to my usb-linus branch :(
Can you rebase and resend?
thanks,
greg k-h
Just sent v3
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Heikki Krogerus -
Kyle Tso