Re: [PATCH v2] usb: serial: fix slab out-of-bounds read in interrupt URB callback

[Johan Hovold <johan@kernel.org>]

On Sun, Jul 26, 2026 at 12:27:51AM +0800, Jiale Yao wrote:
> The interrupt URB buffer is allocated in setup_port_interrupt_in() based
> on the endpoint's wMaxPacketSize:
> 
>     buffer_size = usb_endpoint_maxp(epd);
>     port->interrupt_in_buffer = kmalloc(buffer_size, GFP_KERNEL);
> 
> When a USB device declares wMaxPacketSize = 8 on its interrupt IN
> endpoint, the buffer is allocated from kmalloc-8 cache (exactly 8 bytes).
> 
> If the device sends a short packet (actual_length < wMaxPacketSize),
> the URB completes with status == 0 and the callback proceeds to read:
> 
>     data[sizeof(struct usb_ctrlrequest)]
> 
> which evaluates to data[8], accessing 1 byte beyond the allocated 8-byte
> buffer. This results in a slab out-of-bounds read.
> 
> Fix this by splitting the bounds check into two: first verify that the

As there is no current check I changed this to "by adding the missing
bounds check".

> actual length is large enough to contain the struct usb_ctrlrequest
> header before accessing req_pkt->bRequestType and req_pkt->bRequest,
> and then verify that there is an additional byte for the modem signal
> state before reading data[sizeof(struct usb_ctrlrequest)] inside the
> conditional.  Use sizeof(*req_pkt) instead of sizeof(struct
> usb_ctrlrequest) for consistency.
 
> Changes in v2:
> - Split the bounds check into two separate checks: one for the
>   usb_ctrlrequest header (before the conditional) and one for the
>   modem signal state (inside the conditional), as requested by
>   Johan Hovold and Oliver Neukum.
> - Use sizeof(*req_pkt) instead of sizeof(struct usb_ctrlrequest),
>   as suggested by Greg Kroah-Hartman.
> - Link to v1: https://lore.kernel.org/all/20260712170012.3503601-1-yaojiale02@163.com/T/#u

The changelog should go below the --- line so that it does not end up
in the git logs.

> Assisted-by: Claude:deepseek-v4-pro
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/usb/serial/option.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> index 4c4009b8a46d..caef872a3c40 100644
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> @@ -2672,12 +2672,25 @@ static void option_instat_callback(struct urb *urb)
>  			dev_dbg(dev, "%s: NULL req_pkt\n", __func__);
>  			return;
>  		}
> +
> +		if (urb->actual_length < sizeof(*req_pkt)) {
> +			dev_dbg(dev, "%s: short packet: %u bytes\n",
> +				__func__, urb->actual_length);
> +			return;

Since we stop the reading from the endpoint this should be logged as
an error.

> +		}
> +
>  		if ((req_pkt->bRequestType == 0xA1) &&
>  				(req_pkt->bRequest == 0x20)) {
>  			int old_dcd_state;
> +			if (urb->actual_length < sizeof(*req_pkt) + 1) {
> +				dev_dbg(dev, "%s: short interrupt transfer: %u bytes\n",
> +					__func__, urb->actual_length);
> +				return;

Same here.

> +			}
> +
>  			unsigned char signals = *((unsigned char *)
>  					urb->transfer_buffer +
> -					sizeof(struct usb_ctrlrequest));
> +					sizeof(*req_pkt));

And signals now need to be declared and initialised separately to keep
the declarations at the top of the block.

>  
>  			dev_dbg(dev, "%s: signal x%x\n", __func__, signals);

I fixed up the above when applying this time (and added the missing
driver name to Subject). The end result is here:

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=885d802f544ca7bfa8f3984d94233cce715bb6b3

Johan