[PATCH 0/4] i.MX USB Loader Windows Fixes

[PATCH 0/4] i.MX USB Loader Windows Fixes

[Marco Felsch]

Hi all,

this small series enable our imx-usb-loader and barebox-pbl
implementation to get loaded via Windows.

Regards,
  Marco

Marco Felsch (4):
  scripts: common: fix read_file_2 for windows
  scripts: common: fix buffer freeing
  usb: gadget: fsl_udc: lower state_complete constraints
  imx-usb-loader: fix windows usage

 drivers/usb/gadget/udc/fsl_udc_pbl.c | 2 +-
 scripts/common.c                     | 5 +++--
 scripts/imx/imx-usb-loader.c         | 8 ++++++--
 3 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.41.0

[PATCH 1/4] scripts: common: fix read_file_2 for windows

[Marco Felsch]

From: Marco Felsch <marco.felsch@gmail.com>

We need to specify the O_BINARY flag for Windows else the followed
read() may detected a EOF condition.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 scripts/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/common.c b/scripts/common.c
index 88173bc977..0599cd41ff 100644
--- a/scripts/common.c
+++ b/scripts/common.c
@@ -11,6 +11,7 @@
 #include <stdarg.h>
 
 #include "common.h"
+#include "compiler.h"
 
 int read_file_2(const char *filename, size_t *size, void **outbuf, size_t max_size)
 {
@@ -22,7 +23,7 @@ int read_file_2(const char *filename, size_t *size, void **outbuf, size_t max_si
 	*size = 0;
 	*outbuf = NULL;
 
-	fd = open(filename, O_RDONLY);
+	fd = open(filename, O_RDONLY | O_BINARY);
 	if (fd < 0) {
 		fprintf(stderr, "Cannot open %s: %s\n", filename, strerror(errno));
 		return -errno;
-- 
2.41.0

[PATCH 2/4] scripts: common: fix buffer freeing

[Marco Felsch]

We pass the wrong address to free in case of an error occured in
preceding while-read loop if the loop is executed more than once. Fix
this by use the outbuf which stores the original malloc'ed address.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 scripts/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/common.c b/scripts/common.c
index 0599cd41ff..49c468a1ea 100644
--- a/scripts/common.c
+++ b/scripts/common.c
@@ -76,8 +76,8 @@ int read_file_2(const char *filename, size_t *size, void **outbuf, size_t max_si
 	ret = 0;
 	goto close;
 free:
+	free(*outbuf);
 	*outbuf = NULL;
-	free(buf);
 close:
 	close(fd);
 	return ret;
-- 
2.41.0

[PATCH 3/4] usb: gadget: fsl_udc: lower state_complete constraints

[Marco Felsch]

Signal state_complete if the host send more data than specified first.
This may become crucial with different host usb loader tools and
different host OSes.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/gadget/udc/fsl_udc_pbl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_pbl.c b/drivers/usb/gadget/udc/fsl_udc_pbl.c
index 6a4e0557fc..a5364decb1 100644
--- a/drivers/usb/gadget/udc/fsl_udc_pbl.c
+++ b/drivers/usb/gadget/udc/fsl_udc_pbl.c
@@ -136,7 +136,7 @@ static void dtd_complete_irq(struct usb_dr_device *dr)
 		actual += len - 1;
 		to_transfer -= len - 1;
 
-		if (to_transfer == 0)
+		if (to_transfer <= 0)
 			state = state_complete;
 	}
 
-- 
2.41.0

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

[Marco Felsch]

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;
+
 			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);
-- 
2.41.0

Re: [PATCH 0/4] i.MX USB Loader Windows Fixes

[Sascha Hauer]

On Tue, Oct 17, 2023 at 11:36:04PM +0200, Marco Felsch wrote:
> Hi all,
> 
> this small series enable our imx-usb-loader and barebox-pbl
> implementation to get loaded via Windows.
> 
> Regards,
>   Marco
> 
> Marco Felsch (4):
>   scripts: common: fix read_file_2 for windows
>   scripts: common: fix buffer freeing
>   usb: gadget: fsl_udc: lower state_complete constraints
>   imx-usb-loader: fix windows usage
> 
>  drivers/usb/gadget/udc/fsl_udc_pbl.c | 2 +-
>  scripts/common.c                     | 5 +++--
>  scripts/imx/imx-usb-loader.c         | 8 ++++++--
>  3 files changed, 10 insertions(+), 5 deletions(-)

Applied, thanks

Sascha

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

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

[Ahmad Fatoum]

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 |

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

[Marco Felsch]

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

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

[Ahmad Fatoum]

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 |

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

[Marco Felsch]

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