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