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

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

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.

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

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