linux-kernel - Re: [PATCH v4 1/2] usb: typec: tcpm: tcpci: Make the driver be compatible with the TCPCI spec [Rev 2.0 Ver 1.0, October 2017]

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: <b5e5cd33-2b59-4e93-8acd-379290917e42@stanley.mountain>
Date: Wed, 18 Dec 2024 12:33:42 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: "Miao.Zhu" <Miao.Zhu@...opsys.com>
Cc: gregkh@...uxfoundation.org, robh@...nel.org, xu.yang_2@....com,
	andre.draszik@...aro.org, emanuele.ghidoli@...adex.com,
	heikki.krogerus@...ux.intel.com, m.felsch@...gutronix.de,
	rdbabiera@...gle.com, u.kleine-koenig@...libre.com,
	conor+dt@...nel.org, jun.li@....com, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
	Jianheng.Zhang@...opsys.com, James.Li1@...opsys.com,
	Martin.McKenny@...opsys.com
Subject: Re: [PATCH v4 1/2] usb: typec: tcpm: tcpci: Make the driver be
 compatible with the TCPCI spec [Rev 2.0 Ver 1.0, October 2017]

The subject is too long.  You've sent v2, v3, and v4 today.  Please, wait
for a day between resends.

On Wed, Dec 18, 2024 at 09:59:32AM +0100, Miao.Zhu wrote:
>  static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
> @@ -741,33 +748,86 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  		struct pd_message msg;
>  		unsigned int cnt, payload_cnt;
>  		u16 header;
> +		unsigned int frame_type;
> +		enum tcpm_transmit_type rx_type;
>  
>  		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
>  		/*
>  		 * 'cnt' corresponds to READABLE_BYTE_COUNT in section 4.4.14
>  		 * of the TCPCI spec [Rev 2.0 Ver 1.0 October 2017] and is
>  		 * defined in table 4-36 as one greater than the number of
> -		 * bytes received. And that number includes the header. So:
> +		 * bytes received. And that number includes the header.
> +		 * In Section 4.4.14 of the TCPCI spec [Rev 2.0 Ver 1.0 October, 2017],
> +		 * the RECEIVE_BUFFER comprises of three sets of registers:
> +		 * READABLE_BYTE_COUNT, RX_BUF_FRAME_TYPE and RX_BUF_BYTE_x.
> +		 * These registers can only be accessed by reading at a common
> +		 * register address 0x30h.
>  		 */
> -		if (cnt > 3)
> -			payload_cnt = cnt - (1 + sizeof(msg.header));
> -		else
> -			payload_cnt = 0;
> +		if (tcpci->data->RX_BUF_BYTE_x_hidden) {
> +			u8 buf[TCPC_RECEIVE_BUFFER_MAX_LEN] = {0,};
> +			u8 pos = 0;
> +
> +			/* Read the count and frame type in RECEIVE_BUFFER */
> +			regmap_raw_read(tcpci->regmap, TCPC_RX_BYTE_CNT, buf, 2);
> +			/* READABLE_BYTE_COUNT */
> +			cnt = buf[0];
> +			/* RX_BUF_FRAME_TYPE */
> +			frame_type = buf[1];
> +
> +			/* Read the content of the USB PD message in RECEIVE_BUFFER */
> +			regmap_raw_read(tcpci->regmap, TCPC_RX_BYTE_CNT, buf, cnt + 1);
                                                                         ^^^
buffer overflow?

> +
> +			pos += 2;
> +			memcpy(&msg.header, &buf[pos], sizeof(msg.header));
> +
> +			if (cnt > 3) {
> +				pos += sizeof(msg.header);
> +				payload_cnt = cnt - (1 + sizeof(msg.header));
> +				if (WARN_ON(payload_cnt > sizeof(msg.payload)))
> +					payload_cnt = sizeof(msg.payload);
> +				memcpy(&msg.payload, &buf[pos], payload_cnt);

There is existing code later which does bounds checking on payload_cnt,
but it's too late.  We would have already overflowed buf[] and
msg.payload here.

> +			}
> +		} else {
> +			regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +			/*
> +			 * 'cnt' corresponds to READABLE_BYTE_COUNT in section 4.4.14
> +			 * of the TCPCI spec [Rev 2.0 Ver 1.0 October 2017] and is
> +			 * defined in table 4-36 as one greater than the number of
> +			 * bytes received. And that number includes the header. So:
> +			 */
> +			if (cnt > 3)
> +				payload_cnt = cnt - (1 + sizeof(msg.header));
> +			else
> +				payload_cnt = 0;
>  
> -		tcpci_read16(tcpci, TCPC_RX_HDR, &header);
> -		msg.header = cpu_to_le16(header);
> +			regmap_read(tcpci->regmap, TCPC_RX_BUF_FRAME_TYPE, &frame_type);
>  
> -		if (WARN_ON(payload_cnt > sizeof(msg.payload)))
> -			payload_cnt = sizeof(msg.payload);

regards,
dan carpenter