diff mbox series

[v1] usb: typec: tcpm: fix SOP' sequences in tcpm_pd_svdm

Message ID 20240223002233.3936275-2-rdbabiera@google.com (mailing list archive)
State Accepted
Commit 0e28790195fa65fde41fa127a89e0903388f6285
Headers show
Series [v1] usb: typec: tcpm: fix SOP' sequences in tcpm_pd_svdm | expand

Commit Message

RD Babiera Feb. 23, 2024, 12:22 a.m. UTC 
The Smatch checker flags svdm_version being uninitialized for Discover
Identity Messages within tcpm_pd_svdm for the CMDT_INIT case. Cable plugs
cannot initialize SVDM commands, however a port partner that isn't allowed
to communicate over SOP' could, which would result in the CMDT_INIT block
running for a received SOP' message.

First, initialize svdm_version for the TCPC_TX_SOP_PRIME case. If the
svdm_version returns as an error, we expect the received svdm to be the
result of Discover Identity that updates the value accordingly.

Next, drop all SOP' messages of type CMDT_INIT within tcpm_pd_svdm.

Finally, remove redundant call that assigns modep and pdev. Smatch will
raise an uninitialized symbol error over modep_prime and pdev_prime, but
both the assignment and use of these variables are guarded behind
a check for rx_sop_type == TCPC_TX_SOP_PRIME.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/a432603b-b801-4001-b309-247dded707d3@moroto.mountain/
Fixes: fb7ff25ae433 ("usb: typec: tcpm: add discover identity support for SOP'")
Signed-off-by: RD Babiera <rdbabiera@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)


base-commit: 3bf0514dc6f36f81ee11b1becd977cb87b4c90c6

Comments

Heikki Krogerus Feb. 26, 2024, 8:16 a.m. UTC | #1 
On Fri, Feb 23, 2024 at 12:22:34AM +0000, RD Babiera wrote:
> The Smatch checker flags svdm_version being uninitialized for Discover
> Identity Messages within tcpm_pd_svdm for the CMDT_INIT case. Cable plugs
> cannot initialize SVDM commands, however a port partner that isn't allowed
> to communicate over SOP' could, which would result in the CMDT_INIT block
> running for a received SOP' message.
> 
> First, initialize svdm_version for the TCPC_TX_SOP_PRIME case. If the
> svdm_version returns as an error, we expect the received svdm to be the
> result of Discover Identity that updates the value accordingly.
> 
> Next, drop all SOP' messages of type CMDT_INIT within tcpm_pd_svdm.
> 
> Finally, remove redundant call that assigns modep and pdev. Smatch will
> raise an uninitialized symbol error over modep_prime and pdev_prime, but
> both the assignment and use of these variables are guarded behind
> a check for rx_sop_type == TCPC_TX_SOP_PRIME.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/a432603b-b801-4001-b309-247dded707d3@moroto.mountain/
> Fixes: fb7ff25ae433 ("usb: typec: tcpm: add discover identity support for SOP'")
> Signed-off-by: RD Babiera <rdbabiera@google.com>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d975fc525eac..55c438c62203 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1878,11 +1878,6 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>  	tcpm_log(port, "Rx VDM cmd 0x%x type %d cmd %d len %d",
>  		 p[0], cmd_type, cmd, cnt);
>  
> -	modep = &port->mode_data;
> -
> -	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
> -				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
> -
>  	switch (rx_sop_type) {
>  	case TCPC_TX_SOP_PRIME:
>  		modep_prime = &port->mode_data_prime;
> @@ -1890,11 +1885,13 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>  						 ALTMODE_DISCOVERY_MAX,
>  						 PD_VDO_VID(p[0]),
>  						 PD_VDO_OPOS(p[0]));
> -		if (!IS_ERR_OR_NULL(port->cable)) {
> -			svdm_version = typec_get_cable_svdm_version(typec);
> -			if (PD_VDO_SVDM_VER(p[0]) < svdm_version)
> -				typec_cable_set_svdm_version(port->cable, svdm_version);
> -		}
> +		svdm_version = typec_get_cable_svdm_version(typec);
> +		/*
> +		 * Update SVDM version if cable was discovered before port partner.
> +		 */
> +		if (!IS_ERR_OR_NULL(port->cable) &&
> +		    PD_VDO_SVDM_VER(p[0]) < svdm_version)
> +			typec_cable_set_svdm_version(port->cable, svdm_version);
>  		break;
>  	case TCPC_TX_SOP:
>  		modep = &port->mode_data;
> @@ -1920,6 +1917,15 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>  
>  	switch (cmd_type) {
>  	case CMDT_INIT:
> +		/*
> +		 * Only the port or port partner is allowed to initialize SVDM
> +		 * commands over SOP'. In case the port partner initializes a
> +		 * sequence when it is not allowed to send SOP' messages, drop
> +		 * the message should the TCPM port try to process it.
> +		 */
> +		if (rx_sop_type == TCPC_TX_SOP_PRIME)
> +			return 0;
> +
>  		switch (cmd) {
>  		case CMD_DISCOVER_IDENT:
>  			if (PD_VDO_VID(p[0]) != USB_SID_PD)
> 
> base-commit: 3bf0514dc6f36f81ee11b1becd977cb87b4c90c6
> -- 
> 2.44.0.rc0.258.g7320e95886-goog
diff mbox series

Patch

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index d975fc525eac..55c438c62203 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1878,11 +1878,6 @@  static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
 	tcpm_log(port, "Rx VDM cmd 0x%x type %d cmd %d len %d",
 		 p[0], cmd_type, cmd, cnt);
 
-	modep = &port->mode_data;
-
-	pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX,
-				   PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
-
 	switch (rx_sop_type) {
 	case TCPC_TX_SOP_PRIME:
 		modep_prime = &port->mode_data_prime;
@@ -1890,11 +1885,13 @@  static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
 						 ALTMODE_DISCOVERY_MAX,
 						 PD_VDO_VID(p[0]),
 						 PD_VDO_OPOS(p[0]));
-		if (!IS_ERR_OR_NULL(port->cable)) {
-			svdm_version = typec_get_cable_svdm_version(typec);
-			if (PD_VDO_SVDM_VER(p[0]) < svdm_version)
-				typec_cable_set_svdm_version(port->cable, svdm_version);
-		}
+		svdm_version = typec_get_cable_svdm_version(typec);
+		/*
+		 * Update SVDM version if cable was discovered before port partner.
+		 */
+		if (!IS_ERR_OR_NULL(port->cable) &&
+		    PD_VDO_SVDM_VER(p[0]) < svdm_version)
+			typec_cable_set_svdm_version(port->cable, svdm_version);
 		break;
 	case TCPC_TX_SOP:
 		modep = &port->mode_data;
@@ -1920,6 +1917,15 @@  static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
 
 	switch (cmd_type) {
 	case CMDT_INIT:
+		/*
+		 * Only the port or port partner is allowed to initialize SVDM
+		 * commands over SOP'. In case the port partner initializes a
+		 * sequence when it is not allowed to send SOP' messages, drop
+		 * the message should the TCPM port try to process it.
+		 */
+		if (rx_sop_type == TCPC_TX_SOP_PRIME)
+			return 0;
+
 		switch (cmd) {
 		case CMD_DISCOVER_IDENT:
 			if (PD_VDO_VID(p[0]) != USB_SID_PD)