| Message ID | 20191018141750.23756-2-johan@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: ldusb: fix ring-buffer bugs | expand |
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@vger.kernel.org> # 2.6.13 > Reported-by: syzbot+6fe95b826644f7f12b0b@syzkaller.appspotmail.com > Signed-off-by: Johan Hovold <johan@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
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) { + 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", @@ -690,10 +695,9 @@ static int ld_usb_probe(struct usb_interface *intf, const struct usb_device_id * dev_warn(&intf->dev, "Interrupt out endpoint not found (using control endpoint instead)\n"); dev->interrupt_in_endpoint_size = usb_endpoint_maxp(dev->interrupt_in_endpoint); - dev->ring_buffer = - kmalloc_array(ring_buffer_size, - sizeof(size_t) + dev->interrupt_in_endpoint_size, - GFP_KERNEL); + dev->ring_buffer = kcalloc(ring_buffer_size, + sizeof(size_t) + dev->interrupt_in_endpoint_size, + GFP_KERNEL); if (!dev->ring_buffer) goto error; dev->interrupt_in_buffer = kmalloc(dev->interrupt_in_endpoint_size, GFP_KERNEL);
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@vger.kernel.org> # 2.6.13 Reported-by: syzbot+6fe95b826644f7f12b0b@syzkaller.appspotmail.com Signed-off-by: Johan Hovold <johan@kernel.org> --- drivers/usb/misc/ldusb.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)