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

[Marco Felsch <m.felsch@pengutronix.de>]

On 23-10-18, Ahmad Fatoum wrote:
> 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?

At the moment it is guaranteed that mach_id->max_transfer is always
smaller than 1028. libusb always start to read at &tmp[0] so the last
read may transfer useless bytes (0) but we are always within the tmp
buffer. So no out-of-bound reads.

> >> 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 ...".

Please see above.

Regards,
  Marco

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