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

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

[Xiaohui Zhang]

From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>

tcpm_queue_vdm() calls memcpy() without checking the destination
size may trigger a buffer overflower.

Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
---
 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;
 	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;
-- 
2.17.1

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

[Heikki Krogerus]

Hi,

On Wed, Dec 09, 2020 at 11:07:16AM +0800, Xiaohui Zhang wrote:
> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> 
> 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.

> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> ---
>  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,

-- 
heikki

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@163.com>
>>
>> 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@163.com>
>> ---
>>  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,
>