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