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

[Michal Pecio <michal.pecio@gmail.com>]

On Tue, 28 Apr 2026 16:55:58 -0300, Heitor Alves de Siqueira wrote:
> 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.

I'm completely unfamiliar with this class, but my understanding of
USBTMC spec is that bNotify1 is mandatory while bNotify2 may have
any length, likely including zero, though it's a bit imprecise.

The driver only supports notifications defined in the separate USB488
spec and for these, bNotify2 should be one byte. It also warns on
every unrecognized notification.

I think a minimal fix which mostly preserves existing behavior would
be adding "urb->actual_length == 2" as a requirement for all USB488
notifications. Then any truncated message will be ignored and logged.

wMaxPacketSize is a separate issue indeed and it seems that a USB488
device could legally set it to 1, though it would be crazy. Your v1
patch would probably make such devices work, if anyone cares.

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

Not entirely sure what you mean, but obviously a notification which
doesn't even have bNotify1 is bogus, as is one where bNotify2 is
shorter than requiredy by particular value of bNotify1 and protocol.

> 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().

Not sure what you mean, and I'm not sure if a long multi-packet
vendor notification must have bNotify1 header in every packet.
I think it doesn't, note that transfer != transaction. A device
actually sending such messages could be painful to handle.
But that's yet another separate issue, if such HW even exists.

Regards,
Michal