Re: [PATCH v2] usb: usbtmc: reject invalid interrupt endpoints

["Heitor Alves de Siqueira" <halves@igalia.com>]

On Thu Apr 23, 2026 at 7:28 PM -03, Michal Pecio wrote:
> On Thu, 23 Apr 2026 15:04:38 -0300, Heitor Alves de Siqueira wrote:
>> The USBTMC driver allocates the Interrupt-IN buffer according to the
>> wMaxPacketSize value obtained from the USB endpoint. If a USB device
>> advertises a small enough wMaxPacketSize (e.g. a malfunctioning device
>> or an endpoint constructed by syzbot), the buffer will not have enough
>> space for the mandatory headers and will trigger an out-of-bounds read.
>> 
>> Fix by rejecting devices advertising interrupt endpoints that don't fit
>> at least the mandatory headers (bNotify1 and bNotify2).
>> 
>> Fixes: dbf3e7f654c0 ("Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.")
>> Reported-by: syzbot+abbfd103085885cf16a2@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=abbfd103085885cf16a2
>> Cc: stable@kernel.org
>> Suggested-by: Michal Pecio <michal.pecio@gmail.com>
>> Signed-off-by: Heitor Alves de Siqueira <halves@igalia.com>
>> ---
>> Changes in v2:
>> - Instead of ensuring buffer size, reject devices that advertise illegal/invalid interrupt endpoints
>> - Link to v1: https://patch.msgid.link/20260422-usbtmc-iin-size-v1-1-5dc44b4389aa@igalia.com
>
> On second thought, this may be not enough. A wMaxPacketSize == 2 device
> can still send only 1 byte (or even 0) and cause unititialized read.
> Better check if the URB completed with expected actual_length before
> trying to parse the message.

You're right, although I think these are two separate issues. There
are indeed no checks for actual_length in usbtmc_interrupt(), and I'd
be happy to include those in a v3 (or a separate patch) if you agree
with this approach. For these cases though, I wonder if we can just
ignore the URB, as actual_length<2 would imply that either the headers
are missing, or the payload length is 0.

> And by the way, an interrupt transfer may span multiple intervals and
> exceed wMaxPacketSize, USBTMC spec alludes to it. Theoretically, even
> wMaxPacketSize == 1 seems possible, though it would be crazy and likely
> no such HW exists because nobody complains that it doesn't work.

Yes, but aren't such cases already handled as long as we validate the
bNotify headers? USBTMC interrupt payloads must include at least two
bytes for bNotify1 and bNotify2, so URBs that don't fit those should be
considered invalid, right? Even if the payload is split between multiple
transfers, these headers should be present in individual URBs. Checking
if actual_length fits both bNotify headers should be sufficient, as
data buffers will have enough space for at least wMaxPacketSize, and the
overflow case is already handled in usbtmc_interrupt().

Please let me know what you think, and I can submit a v3 with checks for
wMaxPacketSize in usbtmc_probe() as well as for actual_length in
usbtmc_interrupt().

Best regards,
Heitor