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

[Heikki Krogerus]

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@xxxxxxxxxx>
> 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@xxxxxxxxxx>

Acked-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>

> ---
>  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

-- 
heikki