Re: [PATCH 4/4] imx-usb-loader: fix windows usage

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 18.10.23 14:53, Marco Felsch wrote:
> On 23-10-18, Ahmad Fatoum wrote:
>> On 17.10.23 23:36, Marco Felsch wrote:
>>> This ports commit 561f0377db5e ("Transfer always specified report 2
>>> length") from imx_usb_loader[1] to our implementation.
>>>
>>> | commit 561f0377db5e1a36f5ab5e17f97d774f9f1e22ab
>>> | Author: Stefan Agner <stefan.agner@toradex.com>
>>> | Date:   Wed Nov 2 16:51:24 2016 -0700
>>> |
>>> |     Transfer always specified report 2 length
>>> |
>>> |     The Windows HIDAPI is very strict when it comes to packet lengths:
>>> |     The API always expect the length specified by the device and rejects
>>> |     the transfer otherwise. The report size is specified by the HID
>>> |     Report Descriptor. The i.MX 6 SoC reports a report 2 size of 1024.
>>> |     Currently, imx_usb reads the length from the device specific config
>>> |     files and stores it in max_transfer. This change uses max_transfer
>>> |     for report 2 when using the HID API.
>>> |
>>> |     The boot ROM handles too long transfers just fine. Also NXP's
>>> |     sb_loader transfers always complete reports. The change has been
>>> |     successfully tested on Linux and Windows.
>>> |
>>> |     Given that the device reports the size, it is probably also "the
>>> |     right thing to do"...
>>> |
>>> |     With that change, we might end up transferring up to 1023 bytes
>>> |     more than actually necessary. Make sure that DoIRomDownload does
>>> |     not print an error message and returns successfully even if too
>>> |     many bytes have been transferred (this now typically happens at the
>>> |     end of every file).
>>> |
>>> |     In write_dcd use a signed length variable to keep track of remaining
>>> |     bytes. Continue transfer loop only if more than 0 bytes are left to
>>> |     transfer. Also drop the m_ prefix, m_ is usually used for member
>>> |     variables but this is a local variable.
>>> |
>>> |     Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>>>
>>> [1] https://github.com/boundarydevices/imx_usb_loader/commit/561f0377db5e1a36f5ab5e17f97d774f9f1e22ab
>>>
>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>>> ---
>>>  scripts/imx/imx-usb-loader.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
>>> index 676f077c25..ece4603b2b 100644
>>> --- a/scripts/imx/imx-usb-loader.c
>>> +++ b/scripts/imx/imx-usb-loader.c
>>> @@ -484,12 +484,16 @@ static int transfer(int report, void *p, unsigned cnt, int *last_trans)
>>>  		err = libusb_bulk_transfer(usb_dev_handle,
>>>  					   (report < 3) ? 1 : 2 + EP_IN, p, cnt, last_trans, 1000);
>>>  	} else {
>>> -		unsigned char tmp[1028];
>>> +		unsigned char tmp[1028] = { 0 };
>>>  
>>>  		tmp[0] = (unsigned char)report;
>>>  
>>>  		if (report < 3) {
>>>  			memcpy(&tmp[1], p, cnt);
>>> +
>>> +			if (report == 2)
>>> +				cnt = mach_id->max_transfer;
>>
>> Did you verify that this can't result in an out-of-bounds read?
> 
> How should that be possible? The tmp-array is always 1028-byte and we
> already do limit cnt to max_transfer if cnt is larger than max_transfer.

In the case that cnt < mach_id->max_transfer, we increase cnt to
mach_id->max_transfer and pass that to libusb. Are we guaranteed
that libusb won't read out of bounds, i.e. buf + cnt - 1 is always
inside tmp?

> 
>> I'd have expected either an adjustment to tmp's size or a description
>> why a buffer overread can't happen in the commit message.
> 
> Sorry but can you elaborate where a buffer overrread can happen?

You groked the code and wrote the patch, so I hoped you could tell me
"no, buffer overflow can't happen because ...".

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 
>>> +
>>>  			if (mach_id->hid_endpoint) {
>>>  				int trans;
>>>  				err = libusb_interrupt_transfer(usb_dev_handle,
>>> @@ -739,7 +743,7 @@ static int send_buf(void *buf, unsigned len)
>>>  	while (1) {
>>>  		int now = get_min(cnt, mach_id->max_transfer);
>>>  
>>> -		if (!now)
>>> +		if (now <= 0)
>>>  			break;
>>>  
>>>  		err = transfer(2, p, now, &now);
>>
>> -- 
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |