Re: [PATCH] USB: serial: cypress_m8: validate interrupt packet headers

[Johan Hovold]

On Fri, May 22, 2026 at 12:11:17AM +0800, Zhang Cen wrote:
> cypress_read_int_callback() parses the interrupt-in buffer according to
> the selected Cypress packet format. Format 1 has a two-byte status/count
> header and format 2 has a one-byte combined status/count header. The
> usb-serial core sizes the interrupt-in buffer from the endpoint
> descriptor's wMaxPacketSize, and successful interrupt transfers can
> complete short when URB_SHORT_NOT_OK is not set.
> 
> Check that both the allocated URB buffer and completed packet contain the
> selected header before reading it. Malformed short reports are ignored and
> the interrupt URB is resubmitted through the existing retry path,
> preventing out-of-bounds and stale header-byte reads.

Good catch.

> KASAN report as below:
> KASAN slab-out-of-bounds in cypress_read_int_callback+0x240/0x7f0
> RIP: 0010:kasan_check_range+0x67/0x1b0
> Read of size 1
> Call trace:
>   dump_stack_lvl+0x66/0xa0 (?:?)
>   print_report+0xce/0x630 (?:?)
>   cypress_read_int_callback() (drivers/usb/serial/cypress_m8.c:1009)
>   srso_alias_return_thunk+0x5/0xfbef5 (?:?)
>   __virt_addr_valid+0x188/0x320 (?:?)
>   kasan_report+0xe0/0x110 (?:?)
>   __usb_hcd_giveback_urb+0x103/0x1d0 (?:?)
>   __usb_hcd_giveback_urb+0xf3/0x1d0 (?:?)
>   __usb_hcd_giveback_urb+0x112/0x1d0 (?:?)
>   dummy_timer+0xaaa/0x19a0 (?:?)

You can trim call traces like this one from here (dummy_timer).

>   mark_held_locks+0x40/0x70 (?:?)
>   _raw_spin_unlock_irqrestore+0x44/0x60 (?:?)
>   lockdep_hardirqs_on_prepare+0xb7/0x1a0 (?:?)
>   __hrtimer_run_queues+0x102/0x510 (?:?)
>   hrtimer_run_softirq+0xd0/0x130 (?:?)
>   handle_softirqs+0x155/0x650 (?:?)
>   __irq_exit_rcu+0xc4/0x160 (?:?)
>   irq_exit_rcu+0xe/0x20 (?:?)
>   sysvec_apic_timer_interrupt+0x6c/0x80 (?:?)
>   asm_sysvec_apic_timer_interrupt+0x1a/0x20 (?:?)
> 
> Fixes: 3416eaa1f8f8 ("USB: cypress_m8: Packet format is separate from characteristic size")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Zhang Cen <rollkingzzc@xxxxxxxxx>
> ---
>  drivers/usb/serial/cypress_m8.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c
> index afff1a0f4298b..50c6abc69e756 100644
> --- a/drivers/usb/serial/cypress_m8.c
> +++ b/drivers/usb/serial/cypress_m8.c
> @@ -1016,6 +1016,7 @@ static void cypress_read_int_callback(struct urb *urb)
>  	unsigned long flags;
>  	char tty_flag = TTY_NORMAL;
>  	int bytes = 0;
> +	int header_size = 0;
>  	int result;
>  	int i = 0;
>  	int status = urb->status;
> @@ -1060,18 +1061,32 @@ static void cypress_read_int_callback(struct urb *urb)
>  	default:
>  	case packet_format_1:
>  		/* This is for the CY7C64013... */
> +		header_size = 2;
> +		if (result < header_size ||
> +		    urb->transfer_buffer_length < header_size)

As with you previous patch, there is no need to check the
transfer_buffer_length (here or below).

> +			break;
>  		priv->current_status = data[0] & 0xF8;
>  		bytes = data[1] + 2;
>  		i = 2;

The variable i already holds the header size, so even if it's not
very well named you should reuse that one after moving the assignment.

>  		break;
>  	case packet_format_2:
>  		/* This is for the CY7C63743... */
> +		header_size = 1;
> +		if (result < header_size ||
> +		    urb->transfer_buffer_length < header_size)
> +			break;
>  		priv->current_status = data[0] & 0xF8;
>  		bytes = (data[0] & 0x07) + 1;
>  		i = 1;
>  		break;
>  	}
>  	spin_unlock_irqrestore(&priv->lock, flags);
> +	if (result < header_size || urb->transfer_buffer_length < header_size) {
> +		dev_dbg(dev,
> +			"%s - short packet header - received %d bytes but buffer has %d bytes\n",
> +			__func__, result, urb->transfer_buffer_length);

Just say "short packet received" and possibly include result.

> +		goto continue_read;
> +	}
>  	if (result < bytes) {
>  		dev_dbg(dev,
>  			"%s - wrong packet size - received %d bytes but packet said %d bytes\n",

Johan