Re: [PATCH] USB: serial: sierra: fix slab out-of-bounds read in sierra_instat_callback

[Oliver Neukum <oneukum@suse.com>]

On 14.07.26 20:11, Jay Vadayath wrote:
> The interrupt-in URB buffer is allocated based on the endpoint's
> wMaxPacketSize. A device declaring wMaxPacketSize == 8 gets an 8-byte
> buffer from kmalloc-8. When such a device delivers a short packet,
> sierra_instat_callback() still dereferences transfer_buffer as struct
> usb_ctrlrequest and reads a further byte at data[sizeof(*req_pkt)], one
> byte past the end of the allocation.
> 
> Reject the URB when fewer than sizeof(struct usb_ctrlrequest) + 1 bytes
> were received.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Jay Vadayath <jkrshnmenon@gmail.com>
> Reported-by: Lukas Dresel <lukas@artiphishell.com>
> Signed-off-by: Jay Vadayath <jkrshnmenon@gmail.com>

Nacked-by: Oliver Neukum <oneukum@suse.com>

> +
> +		if (urb->actual_length < sizeof(struct usb_ctrlrequest) + 1) {
> +			dev_dbg(&port->dev, "%s: short interrupt transfer: %d bytes\n",
> +				__func__, urb->actual_length);
> +			return;
> +		}

I am sorry, but you cannot do this. Not here.

> +
>   		if ((req_pkt->bRequestType == 0xA1) &&
>   				(req_pkt->bRequest == 0x20)) {

This is the test you need to check how long the reply needs to be.

If we look at the full evaluation of the package from the driver we have:

                 }
                 if ((req_pkt->bRequestType == 0xA1) &&
                                 (req_pkt->bRequest == 0x20)) {
                         int old_dcd_state;
                         unsigned char signals = *((unsigned char *)
                                         urb->transfer_buffer +
                                         sizeof(struct usb_ctrlrequest));

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

                         old_dcd_state = portdata->dcd_state;
                         portdata->cts_state = 1;
                         portdata->dcd_state = ((signals & 0x01) ? 1 : 0);
                         portdata->dsr_state = ((signals & 0x02) ? 1 : 0);
                         portdata->ri_state = ((signals & 0x08) ? 1 : 0);

                         if (old_dcd_state && !portdata->dcd_state)
                                 tty_port_tty_hangup(&port->port, true);
                 } else {
                         dev_dbg(&port->dev, "%s: type %x req %x\n",
                                 __func__, req_pkt->bRequestType,
                                 req_pkt->bRequest);
                 }

In this branch:

                 if ((req_pkt->bRequestType == 0xA1) &&
                                 (req_pkt->bRequest == 0x20)) {

replies must be at least sizeof(struct usb_ctrlrequest) + 1 long.
But in the else branch a length of sizeof(struct usb_ctrlrequest) will do,
because they do not evaluate the signal.

In other words, you stop processing messages if a message the driver does not
care about, but is valid under the specification arrives. That breaks the driver.

	Regards
		Oliver