Re: [PATCH] usb: typec: tcpci: pass correct rx_type to tcpm_pd_receive()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The newly added regmap_read() for TCPC_RX_BUF_FRAME_TYPE ignores its return value, leading to the use of an uninitialized stack variable.

Pre-existing issues:
- [High] Unchecked I2C read failure at the end of the IRQ handler causes an infinite loop and CPU hang.
- [High] Incorrect endianness conversion for 16-bit registers on Big Endian architectures corrupts message headers and status flags.
- [Medium] Missing I2C read error checks lead to the use of uninitialized stack variables for `cnt` and `header`.
--

commit 81a7c680e2fcddf86dbe13e10560745c1e22e61c
Author: Xu Yang <xu.yang_2@nxp.com>

usb: typec: tcpci: pass correct rx_type to tcpm_pd_receive()

This commit reads the frame type from the TCPC_RX_BUF_FRAME_TYPE register
after receiving a PD message. It maps the read value to the appropriate
transmit type and passes it to tcpm_pd_receive() to correctly distinguish
SOP from SOP' messages.

> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 7ac7000b2d139..c19413f41bcb6 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
[ ... ]
> @@ -748,7 +748,8 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  
>  	if (status & TCPC_ALERT_RX_STATUS) {
>  		struct pd_message msg;
> -		unsigned int cnt, payload_cnt;
> +		unsigned int cnt, type, payload_cnt;
> +		enum tcpm_transmit_type rx_type;
>  		u16 header;
>  
>  		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);

[Severity: Medium]
This is a pre-existing issue, but what happens if the I2C read for
TCPC_RX_BYTE_CNT fails?

If regmap_read() returns an error, the local stack variable `cnt` remains
uninitialized. This random stack data would then be used for calculating
`payload_cnt` further down.

>  		/*
>  		 * 'cnt' corresponds to READABLE_BYTE_COUNT in section 4.4.14
[ ... ]
>  		if (cnt > 3)
>  			payload_cnt = cnt - (1 + sizeof(msg.header));
>  		else
>  			payload_cnt = 0;
>  
> +		regmap_read(tcpci->regmap, TCPC_RX_BUF_FRAME_TYPE, &type);
> +		switch (type) {

[Severity: High]
Does this code use an uninitialized value if the I2C read fails?

If regmap_read() returns an error code due to an I2C failure, `type` will be
left uninitialized and contain random stack memory. This garbage value is
then evaluated in the switch statement, which could incorrectly map a normal
SOP message as an SOP' message and disrupt Power Delivery negotiations.

> +		case TCPC_RX_BUF_FRAME_TYPE_SOP1:
> +			rx_type = TCPC_TX_SOP_PRIME;
> +			break;
> +		case TCPC_RX_BUF_FRAME_TYPE_SOP:
> +		default:
> +			rx_type = TCPC_TX_SOP;
> +			break;
> +		}
> +
>  		tcpci_read16(tcpci, TCPC_RX_HDR, &header);

[Severity: Medium]
This is a pre-existing issue, but similar to the TCPC_RX_BYTE_CNT read,
should the return value of tcpci_read16() be checked here?

If the read fails, `header` remains uninitialized before being evaluated
on the next line.

>  		msg.header = cpu_to_le16(header);

[Severity: High]
This is a pre-existing issue, but could this byte swap result in corrupted
message headers on Big Endian architectures?

Looking at tcpci_read16(), it uses regmap_raw_read() to read a 16-bit
register into a host u16 variable. regmap_raw_read() bypasses endianness
formatting, meaning the buffer gets the raw little-endian bytes from the
device.

On a Big Endian CPU, reading this u16 produces a byte-swapped value. When
cpu_to_le16() is then applied, it byte-swaps the value again, resulting in
an incorrect big-endian byte sequence in memory. This breaks the
interpretation of the header in the TCPM core.

>  
[ ... ]
>  		/* Read complete, clear RX status alert bit */
>  		tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
>  
> -		tcpm_pd_receive(tcpci->port, &msg, TCPC_TX_SOP);
> +		tcpm_pd_receive(tcpci->port, &msg, rx_type);
>  	}
>  
>  	if (tcpci->data->vbus_vsafe0v && (status & TCPC_ALERT_EXTENDED_STATUS)) {

[Severity: High]
This is a pre-existing issue, but further down in tcpci_irq(), does the
unchecked I2C read at the end of the IRQ handler cause an infinite loop?

drivers/usb/typec/tcpm/tcpci.c:tcpci_irq() {
    ...
    process_status:
    ...
    tcpci_read16(tcpci, TCPC_ALERT, &status);

    if (status & tcpci->alert_mask)
        goto process_status;
}

If this final I2C read fails, the local `status` variable is unmodified and
retains its previous value. Because the previous value triggered the loop,
this causes an unconditional `goto process_status;`, potentially hanging
the CPU if the I2C bus is wedged.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703063247.2521524-1-xu.yang_2@oss.nxp.com?part=1