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 <sta...@vger.kernel.org> # 2.6.13
> Reported-by: syzbot+6fe95b826644f7f12...@syzkaller.appspotmail.com
> Signed-off-by: Johan Hovold <jo...@kernel.org>
> ---
> 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
