Re: [PATCH 1/1] tcpm: Fix possible buffer overflows in tcpm_queue_vdm

[Guenter Roeck]

On 12/11/20 2:09 AM, Heikki Krogerus wrote:
> Hi,
> 
> On Wed, Dec 09, 2020 at 11:07:16AM +0800, Xiaohui Zhang wrote:
>> From: Zhang Xiaohui <ruc_zhangxiaohui@xxxxxxx>
>>
>> tcpm_queue_vdm() calls memcpy() without checking the destination
>> size may trigger a buffer overflower.
> 
> Thanks for the patch, but I didn't actually see any place where that
> could happen. I think the idea is that the callers make sure the count
> does not exceed VDO_MAX_SIZE before calling the function.
> 

Yes, when I wrote the code I made sure that this is the case.
If that is no longer true, we have various other problems because
the count is assumed to be in range pretty much everywhere.

Guenter

>> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@xxxxxxx>
>> ---
>>  drivers/usb/typec/tcpm/tcpm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index 55535c4f6..fcd331f33 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -1045,7 +1045,7 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header,
>>  
>>  	port->vdo_count = cnt + 1;
> 
> That should have been fixed as well, no?
> 
>>  	port->vdo_data[0] = header;
>> -	memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt);
>> +	memcpy(&port->vdo_data[1], data, min_t(int, sizeof(u32) * cnt, VDO_MAX_SIZE - 1));
>>  	/* Set ready, vdm state machine will actually send */
>>  	port->vdm_retries = 0;
>>  	port->vdm_state = VDM_STATE_READY;
> 
> Unless I'm missing something, I don't think this patch is needed.
> 
> thanks,
>