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

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

[Heitor Alves de Siqueira]

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
---
 drivers/usb/class/usbtmc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index bd9347804dec..d851c1d76d5b 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -2431,6 +2431,10 @@ static int usbtmc_probe(struct usb_interface *intf,
 		data->iin_ep_present = 1;
 		data->iin_ep = int_in->bEndpointAddress;
 		data->iin_wMaxPacketSize = usb_endpoint_maxp(int_in);
+		if (data->iin_wMaxPacketSize < 2) {
+			retcode = -EINVAL;
+			goto err_put;
+		}
 		data->iin_interval = int_in->bInterval;
 		dev_dbg(&intf->dev, "Found Int in endpoint at %u\n",
 				data->iin_ep);

---
base-commit: 70c8a7ec6715b5fb14e501731b5b9210a16684f7
change-id: 20260422-usbtmc-iin-size-f1aaf04a6c4c

Best regards,
--  
Heitor Alves de Siqueira <halves@igalia.com>

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

[Michal Pecio]

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.

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.

Regards,
Michal

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

[Heitor Alves de Siqueira]

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

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

[Michal Pecio]

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