Re: [PATCH] usbtmc: Fix slab-out-of-bounds access in usbtmc_interrupt

[Dave Penkler]

On Fri, Mar 14, 2025 at 12:19:07AM +0300, Vasiliy Kovalev wrote:
> Prevent buffer overflow in usbtmc_interrupt by ensuring interrupt IN
> endpoint packets are at least 2 bytes (1 byte ID + 1 byte data), as
> required by the driver. Without this, short packets could lead to
> out-of-bounds reads of iin_buffer.
> 
> - In usbtmc_probe(), reject devices with wMaxPacketSize < 2 bytes to
>   ensure the allocated iin_buffer is large enough.
> - In usbtmc_interrupt(), skip processing and log a warning if
>   actual_length < 2 bytes, avoiding invalid memory access.
> 
> This fixes a KASAN slab-out-of-bounds error:
> 
> BUG: KASAN: slab-out-of-bounds in usbtmc_interrupt (drivers/usb/class/usbtmc.c:2297)
> Read of size 1 at addr ffff88800a41b2e1 by task kworker/0:1/11
> CPU: 0 UID: 0 PID: 11 Comm: kworker/0:1 Not tainted 6.14.0-un-def-alt0.rc6.kasan #1
> Call Trace:
>  <IRQ>
>  dump_stack_lvl (lib/dump_stack.c:122)
>  print_report (mm/kasan/report.c:521)
>  kasan_report (mm/kasan/report.c:636)
>  usbtmc_interrupt (drivers/usb/class/usbtmc.c:2297)
>  __usb_hcd_giveback_urb (drivers/usb/core/hcd.c:1650)
>  usb_hcd_giveback_urb (drivers/usb/core/hcd.c:1735)
>  dummy_timer (drivers/usb/gadget/udc/dummy_hcd.c:1995)
>  __hrtimer_run_queues (kernel/time/hrtimer.c:1865)
>  hrtimer_run_softirq (kernel/time/hrtimer.c:1884)
>  handle_softirqs (kernel/softirq.c:561)
>  __irq_exit_rcu (kernel/softirq.c:662)
>  irq_exit_rcu (kernel/softirq.c:680)
>  sysvec_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:1049)
>  </IRQ>
> 
> Found by Linux Verification Center (linuxtesting.org) with
> 'USB Gadget Tests'[1]:
> 
> $ make usbtmc
> $ sudo src/usbtmc/usbtmc --invalid_ep_int_len
> 
> [1] Link: https://github.com/kovalev0/usb-gadget-tests
> Fixes: dbf3e7f654c0 ("Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Vasiliy Kovalev <kovalev@xxxxxxxxxxxx>
> ---
>  drivers/usb/class/usbtmc.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index 34e46ef308abf..8a3ba90f32455 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -47,6 +47,9 @@
>   */
>  #define USBTMC_MAX_READS_TO_CLEAR_BULK_IN	100
>  
> +/* Minimum packet size for interrupt IN endpoint */
> +#define USBTMC_MIN_INT_IN_PACKET_SIZE 2	// 1 byte ID + 1 byte data
> +
>  static const struct usb_device_id usbtmc_devices[] = {
>  	{ USB_INTERFACE_INFO(USB_CLASS_APP_SPEC, 3, 0), },
>  	{ USB_INTERFACE_INFO(USB_CLASS_APP_SPEC, 3, 1), },
> @@ -2291,6 +2294,14 @@ static void usbtmc_interrupt(struct urb *urb)
>  
>  	switch (status) {
>  	case 0: /* SUCCESS */
> +		/* check for valid length */
> +		if (urb->actual_length < USBTMC_MIN_INT_IN_PACKET_SIZE) {
> +			dev_warn(dev, "short interrupt packet: %d bytes, min %d required\n",
> +				 urb->actual_length,
> +				 USBTMC_MIN_INT_IN_PACKET_SIZE);
> +			goto exit;
> +		}
> +
>  		/* check for valid STB notification */
>  		if (data->iin_buffer[0] > 0x81) {
>  			data->bNotify1 = data->iin_buffer[0];
> @@ -2426,6 +2437,15 @@ static int usbtmc_probe(struct usb_interface *intf,
>  		dev_err(&intf->dev, "can't read capabilities\n");
>  
>  	if (data->iin_ep_present) {
> +		if (data->iin_wMaxPacketSize < USBTMC_MIN_INT_IN_PACKET_SIZE) {
> +			dev_err(&intf->dev,
> +				"Int in endpoint wMaxPacketSize too small: %d, minimum %d required\n",
> +				data->iin_wMaxPacketSize,
> +				USBTMC_MIN_INT_IN_PACKET_SIZE);
> +			retcode = -EINVAL;
> +			goto error_register;
> +		}
> +
>  		/* allocate int urb */
>  		data->iin_urb = usb_alloc_urb(0, GFP_KERNEL);
>  		if (!data->iin_urb) {
> -- 
> 2.42.2
>
Tested-by: Dave Penkler <dpenkler@xxxxxxxxx>