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@xxxxxxxxxxx>
> | 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@xxxxxxxxxxx>
>
> [1] https://github.com/boundarydevices/imx_usb_loader/commit/561f0377db5e1a36f5ab5e17f97d774f9f1e22ab
>
> Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> ---
> 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 |
