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

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

[Jiale Yao]

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 adding a bounds check before accessing data[8], ensuring that
the actual length is sufficient to contain a full USB control request
header.

Signed-off-by: Jiale Yao <yaojiale02@163.com>
---
 drivers/usb/serial/option.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index 4c4009b8a46d..4aae54e1eacf 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -2672,6 +2672,13 @@ static void option_instat_callback(struct urb *urb)
 			dev_dbg(dev, "%s: NULL req_pkt\n", __func__);
 			return;
 		}
+
+		if (urb->actual_length < sizeof(struct usb_ctrlrequest) + 1) {
+			dev_dbg(dev, "%s: short interrupt transfer: %d bytes\n",
+				__func__, urb->actual_length);
+			return;
+		}
+
 		if ((req_pkt->bRequestType == 0xA1) &&
 				(req_pkt->bRequest == 0x20)) {
 			int old_dcd_state;
-- 
2.34.1

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

[Greg KH]

On Mon, Jul 13, 2026 at 01:00:12AM +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 adding a bounds check before accessing data[8], ensuring that
> the actual length is sufficient to contain a full USB control request
> header.
> 
> Signed-off-by: Jiale Yao <yaojiale02@163.com>
> ---
>  drivers/usb/serial/option.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
> index 4c4009b8a46d..4aae54e1eacf 100644
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> @@ -2672,6 +2672,13 @@ static void option_instat_callback(struct urb *urb)
>  			dev_dbg(dev, "%s: NULL req_pkt\n", __func__);
>  			return;
>  		}
> +
> +		if (urb->actual_length < sizeof(struct usb_ctrlrequest) + 1) {
> +			dev_dbg(dev, "%s: short interrupt transfer: %d bytes\n",
> +				__func__, urb->actual_length);
> +			return;
> +		}

Close, shouldn't this be rewritten to be:

               if (urb->actual_length < sizeof(*req_pkt) + 1) {
                       dev_dbg(dev, "%s: short packet %u\n", __func__,
                               urb->actual_length);

As that's the structure you need to care about instead?

Also, nit, dev_dbg() already has the __func__ name in it, no need to
duplicate it again (yes, the other ones in this file do that, so it is
the style here...)

thanks,

greg k-h

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

[Johan Hovold]

On Mon, Jul 13, 2026 at 08:30:38AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 13, 2026 at 01:00:12AM +0800, Jiale Yao wrote:

> > @@ -2672,6 +2672,13 @@ static void option_instat_callback(struct urb *urb)
> >  			dev_dbg(dev, "%s: NULL req_pkt\n", __func__);
> >  			return;
> >  		}
> > +
> > +		if (urb->actual_length < sizeof(struct usb_ctrlrequest) + 1) {
> > +			dev_dbg(dev, "%s: short interrupt transfer: %d bytes\n",
> > +				__func__, urb->actual_length);
> > +			return;
> > +		}
> 
> Close, shouldn't this be rewritten to be:
> 
>                if (urb->actual_length < sizeof(*req_pkt) + 1) {
>                        dev_dbg(dev, "%s: short packet %u\n", __func__,
>                                urb->actual_length);
> 
> As that's the structure you need to care about instead?

The load uses sizeof(struct usb_ctrlrequest) currently so I think it's
fine to use that in the length check as well.

But looking at this again, the check should be split in two: one for the
struct usb_ctrlrequest header and one for the modem signal state (inside
the conditional).

Johan

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

[Oliver Neukum]

On 13.07.26 12:02, Johan Hovold wrote:

> But looking at this again, the check should be split in two: one for the
> struct usb_ctrlrequest header and one for the modem signal state (inside
> the conditional).

Not just should be. It must be.
Doing the same check for both cases means that any of the, presumably
allowed, messages the driver does not care about will terminate communication.

	Regards
		Oliver

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

[Johan Hovold]

On Mon, Jul 13, 2026 at 12:44:17PM +0200, Oliver Neukum wrote:
> On 13.07.26 12:02, Johan Hovold wrote:
> 
> > But looking at this again, the check should be split in two: one for the
> > struct usb_ctrlrequest header and one for the modem signal state (inside
> > the conditional).
> 
> Not just should be. It must be.

Yeah, I meant "should" in the natural language sense of the word (not
"SHOULD" as in rfc2119).

> Doing the same check for both cases means that any of the, presumably
> allowed, messages the driver does not care about will terminate communication.

Indeed.

Johan

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

[Johan Hovold]

On Mon, Jul 13, 2026 at 01:00:12AM +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 adding a bounds check before accessing data[8], ensuring that
> the actual length is sufficient to contain a full USB control request
> header.
> 
> Signed-off-by: Jiale Yao <yaojiale02@163.com>

This looks correct, but how was this issue found and how was the patch
created?

If you used an LLM you need to document this in the commit message, see:

	Documentation/process/generated-content.rst
	Documentation/process/coding-assistants.rst

Johan