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

[Oliver Neukum]

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@xxxxxxxxxxxxxxx
> Reported-by: Jay Vadayath <jkrshnmenon@xxxxxxxxx>
> Reported-by: Lukas Dresel <lukas@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jay Vadayath <jkrshnmenon@xxxxxxxxx>

Nacked-by: Oliver Neukum <oneukum@xxxxxxxx>

> +
> +		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