linux-kernel - Re: [PATCH 1/1] tcpm: Fix possible buffer overflows in tcpm_queue

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 11 Dec 2020 07:12:39 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Xiaohui Zhang <ruc_zhangxiaohui@....com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] tcpm: Fix possible buffer overflows in tcpm_queue_vdm

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