Re: [PATCH 1/2] USB: ldusb: fix read info leaks

[Johan Hovold]

On Fri, Oct 18, 2019 at 04:17:49PM +0200, Johan Hovold wrote:
> Fix broken read implementation, which could be used to trigger slab info
> leaks.
> 
> The driver failed to check if the custom ring buffer was still empty
> when waking up after having waited for more data. This would happen on
> every interrupt-in completion, even if no data had been added to the
> ring buffer (e.g. on disconnect events).
> 
> Due to missing sanity checks and uninitialised (kmalloced) ring-buffer
> entries, this meant that huge slab info leaks could easily be triggered.
> 
> Note that the empty-buffer check after wakeup is enough to fix the info
> leak on disconnect, but let's clear the buffer on allocation and add a
> sanity check to read() to prevent further leaks.
> 
> Fixes: 2824bd250f0b ("[PATCH] USB: add ldusb driver")
> Cc: stable <stable@xxxxxxxxxxxxxxx>     # 2.6.13
> Reported-by: syzbot+6fe95b826644f7f12b0b@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>
> ---
>  drivers/usb/misc/ldusb.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
> index 147c90c2a4e5..94780e14e95d 100644
> --- a/drivers/usb/misc/ldusb.c
> +++ b/drivers/usb/misc/ldusb.c
> @@ -464,7 +464,7 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
>  
>  	/* wait for data */
>  	spin_lock_irq(&dev->rbsl);
> -	if (dev->ring_head == dev->ring_tail) {
> +	while (dev->ring_head == dev->ring_tail) {
>  		dev->interrupt_in_done = 0;
>  		spin_unlock_irq(&dev->rbsl);
>  		if (file->f_flags & O_NONBLOCK) {
> @@ -474,12 +474,17 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
>  		retval = wait_event_interruptible(dev->read_wait, dev->interrupt_in_done);
>  		if (retval < 0)
>  			goto unlock_exit;
> -	} else {
> -		spin_unlock_irq(&dev->rbsl);
> +
> +		spin_lock_irq(&dev->rbsl);
>  	}
> +	spin_unlock_irq(&dev->rbsl);
>  
>  	/* actual_buffer contains actual_length + interrupt_in_buffer */
>  	actual_buffer = (size_t *)(dev->ring_buffer + dev->ring_tail * (sizeof(size_t)+dev->interrupt_in_endpoint_size));
> +	if (*actual_buffer > sizeof(size_t) + dev->interrupt_in_endpoint_size) {

Bah, this should have been just

	if (*actual_buffer > dev->interrupt_in_endpoint_size)

will send a v2.
	
> +		retval = -EIO;
> +		goto unlock_exit;
> +	}
>  	bytes_to_read = min(count, *actual_buffer);
>  	if (bytes_to_read < *actual_buffer)
>  		dev_warn(&dev->intf->dev, "Read buffer overflow, %zd bytes dropped\n",

Johan