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

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

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?

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.

> +
>  			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 |