usb: usbtmc: Allocate enough space for interrupt-IN buffer

[PATCH] usb: usbtmc: Allocate enough space for interrupt-IN buffer

Posted by Heitor Alves de Siqueira 1 month, 3 weeks ago

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 ensuring the driver will allocate at least enough space to fit
the headers for Interrupt-IN packets (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
Signed-off-by: Heitor Alves de Siqueira <halves@igalia.com>
---
 drivers/usb/class/usbtmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index bd9347804dec..22efa74008f8 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -2452,7 +2452,7 @@ static int usbtmc_probe(struct usb_interface *intf,
 		kref_get(&data->kref);
 
 		/* allocate buffer for interrupt in */
-		data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
+		data->iin_buffer = kmalloc(max(2, data->iin_wMaxPacketSize),
 					GFP_KERNEL);
 		if (!data->iin_buffer) {
 			retcode = -ENOMEM;

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

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

Re: [PATCH] usb: usbtmc: Allocate enough space for interrupt-IN buffer

Posted by Michal Pecio 1 month, 3 weeks ago

On Wed, 22 Apr 2026 19:22:09 -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 ensuring the driver will allocate at least enough space to fit
> the headers for Interrupt-IN packets (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
> Signed-off-by: Heitor Alves de Siqueira <halves@igalia.com>
> ---
>  drivers/usb/class/usbtmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index bd9347804dec..22efa74008f8 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -2452,7 +2452,7 @@ static int usbtmc_probe(struct usb_interface *intf,
>  		kref_get(&data->kref);
>  
>  		/* allocate buffer for interrupt in */
> -		data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
> +		data->iin_buffer = kmalloc(max(2, data->iin_wMaxPacketSize),
>  					GFP_KERNEL);

This changes OOB read into uninitialized memory read, which syzbot
may complain about again next week.

More reliable fix would be to reject such devices if they are illegal,
or make the driver not read beyond wMaxPacketSize if they are legal.

Note that the driver never worked for them properly anyway and returned
data from unititialized memory or page faulted.

>  		if (!data->iin_buffer) {
>  			retcode = -ENOMEM;
> 
> ---
> base-commit: 70c8a7ec6715b5fb14e501731b5b9210a16684f7
> change-id: 20260422-usbtmc-iin-size-f1aaf04a6c4c
> 
> Best regards,
> --  
> Heitor Alves de Siqueira <halves@igalia.com>
>

Re: [PATCH] usb: usbtmc: Allocate enough space for interrupt-IN buffer

Posted by Heitor Alves de Siqueira 1 month, 3 weeks ago

On Thu Apr 23, 2026 at 2:33 AM -03, Michal Pecio wrote:
> On Wed, 22 Apr 2026 19:22:09 -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 ensuring the driver will allocate at least enough space to fit
>> the headers for Interrupt-IN packets (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
>> Signed-off-by: Heitor Alves de Siqueira <halves@igalia.com>
>> ---
>>  drivers/usb/class/usbtmc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
>> index bd9347804dec..22efa74008f8 100644
>> --- a/drivers/usb/class/usbtmc.c
>> +++ b/drivers/usb/class/usbtmc.c
>> @@ -2452,7 +2452,7 @@ static int usbtmc_probe(struct usb_interface *intf,
>>  		kref_get(&data->kref);
>>  
>>  		/* allocate buffer for interrupt in */
>> -		data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
>> +		data->iin_buffer = kmalloc(max(2, data->iin_wMaxPacketSize),
>>  					GFP_KERNEL);
>
> This changes OOB read into uninitialized memory read, which syzbot
> may complain about again next week.
>
> More reliable fix would be to reject such devices if they are illegal,
> or make the driver not read beyond wMaxPacketSize if they are legal.
>

That's a great point. The USBTMC spec doesn't explicitly mention that
small sizes of vMaxPacketSize are illegal, but indeed it doesn't make
sense to have a device that can't transfer at least the mandatory
headers. I'll see if I can send a follow-up for this soon, thanks!

Best regards,
Heitor

Re: [PATCH] usb: usbtmc: Allocate enough space for interrupt-IN buffer

Posted by Greg Kroah-Hartman 1 month, 3 weeks ago

On Thu, Apr 23, 2026 at 07:33:07AM +0200, Michal Pecio wrote:
> On Wed, 22 Apr 2026 19:22:09 -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 ensuring the driver will allocate at least enough space to fit
> > the headers for Interrupt-IN packets (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
> > Signed-off-by: Heitor Alves de Siqueira <halves@igalia.com>
> > ---
> >  drivers/usb/class/usbtmc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> > index bd9347804dec..22efa74008f8 100644
> > --- a/drivers/usb/class/usbtmc.c
> > +++ b/drivers/usb/class/usbtmc.c
> > @@ -2452,7 +2452,7 @@ static int usbtmc_probe(struct usb_interface *intf,
> >  		kref_get(&data->kref);
> >  
> >  		/* allocate buffer for interrupt in */
> > -		data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
> > +		data->iin_buffer = kmalloc(max(2, data->iin_wMaxPacketSize),
> >  					GFP_KERNEL);
> 
> This changes OOB read into uninitialized memory read, which syzbot
> may complain about again next week.
> 
> More reliable fix would be to reject such devices if they are illegal,

That's the best thing to do.

thanks,

greg k-h