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]

[Dan Carpenter]

On Wed, Dec 18, 2024 at 12:33:42PM +0300, Dan Carpenter wrote:
> 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.
> 

This line is obviously fine.  It's only buf[] earlier from regmap_raw_read()
I'm worried about.

regards,
dan carpenter