linux-kernel - Re: [PATCH v3] typec: tcpm: fusb302: Resolve out of order messaging events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171121185635.GA8980@roeck-us.net>
Date:   Tue, 21 Nov 2017 10:56:35 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Adam Thomson <Adam.Thomson.Opensource@...semi.com>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Yueyao Zhu <yueyao.zhu@...il.com>,
        Rui Miguel Silva <rmfrfs@...il.com>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        support.opensource@...semi.com
Subject: Re: [PATCH v3] typec: tcpm: fusb302: Resolve out of order messaging
 events

On Tue, Nov 21, 2017 at 02:12:12PM +0000, Adam Thomson wrote:
> The expectation in the FUSB302 driver is that a TX_SUCCESS event
> should occur after a message has been sent, but before a GCRCSENT
> event is raised to indicate successful receipt of a message from
> the partner. However in some circumstances it is possible to see
> the hardware raise a GCRCSENT event before a TX_SUCCESS event
> is raised. The upshot of this is that the GCRCSENT handling portion
> of code ends up reporting the GoodCRC message to TCPM because the
> TX_SUCCESS event hasn't yet arrived to trigger a consumption of it.
> When TX_SUCCESS is then raised by the chip it ends up consuming the
> actual message that was meant for TCPM, and this incorrect sequence
> results in a hard reset from TCPM.
> 
> To avoid this problem, this commit updates the message reading
> code to check whether a GoodCRC message was received or not. Based
> on this check it will either report that the previous transmission
> has completed or it will pass the msg data to TCPM for futher
> processing. This way the incorrect ordering of the events no longer
> matters.
> 
> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@...semi.com>

I am not really happy about the code duplication (two calls to
fusb302_pd_read_message() and identical error messages), but
everything else I came up with was even more messy, so

Reviewed-by: Guenter Roeck <linux@...ck-us.net>

> ---
> 
> Changes in v3:
>  - Always read from FIFO on TX_SUCCES and GCRCSENT events, but decision on how
>    to report to TCPM is no longer based on these event types but instead on type
>    of message read in from FIFO.
> 
> Changes in v2:
>  - Remove erroneous extended header check
> 
> Patch is based on Linux next-20171114 to include move out of staging.
> 
>  drivers/usb/typec/fusb302/fusb302.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/fusb302/fusb302.c b/drivers/usb/typec/fusb302/fusb302.c
> index 72cb060..d200085 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -1543,6 +1543,21 @@ static int fusb302_pd_read_message(struct fusb302_chip *chip,
>  	fusb302_log(chip, "PD message header: %x", msg->header);
>  	fusb302_log(chip, "PD message len: %d", len);
>  
> +	/*
> +	 * Check if we've read off a GoodCRC message. If so then indicate to
> +	 * TCPM that the previous transmission has completed. Otherwise we pass
> +	 * the received message over to TCPM for processing.
> +	 *
> +	 * We make this check here instead of basing the reporting decision on
> +	 * the IRQ event type, as it's possible for the chip to report the
> +	 * TX_SUCCESS and GCRCSENT events out of order on occasion, so we need
> +	 * to check the message type to ensure correct reporting to TCPM.
> +	 */
> +	if ((!len) && (pd_header_type_le(msg->header) == PD_CTRL_GOOD_CRC))
> +		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
> +	else
> +		tcpm_pd_receive(chip->tcpm_port, msg);
> +
>  	return ret;
>  }
>  
> @@ -1650,13 +1665,12 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  
>  	if (interrupta & FUSB_REG_INTERRUPTA_TX_SUCCESS) {
>  		fusb302_log(chip, "IRQ: PD tx success");
> -		/* read out the received good CRC */
>  		ret = fusb302_pd_read_message(chip, &pd_msg);
>  		if (ret < 0) {
> -			fusb302_log(chip, "cannot read in GCRC, ret=%d", ret);
> +			fusb302_log(chip,
> +				    "cannot read in PD message, ret=%d", ret);
>  			goto done;
>  		}
> -		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
>  	}
>  
>  	if (interrupta & FUSB_REG_INTERRUPTA_HARDRESET) {
> @@ -1677,7 +1691,6 @@ static irqreturn_t fusb302_irq_intn(int irq, void *dev_id)
>  				    "cannot read in PD message, ret=%d", ret);
>  			goto done;
>  		}
> -		tcpm_pd_receive(chip->tcpm_port, &pd_msg);
>  	}
>  done:
>  	mutex_unlock(&chip->lock);
> -- 
> 1.9.1
>