USB: ldusb: fix ring-buffer bugs

The custom ring-buffer implementation was merged without any locking whatsoever, but a spinlock was later added by commit 9d33efd9a791 ("USB: ldusb bugfix").

The lock did not cover the loads from the ring-buffer entry after determining the buffer was non-empty, nor the update of the tail index once the entry had been processed. The former could lead to stale data being returned, while the latter could lead to memory corruption on sufficiently weakly ordered architectures.

Quoted 9 lines · Johan Hovold

The custom ring-buffer implementation was merged without any locking whatsoever, but a spinlock was later added by commit 9d33efd9a791 ("USB: ldusb bugfix").

The lock did not cover the loads from the ring-buffer entry after determining the buffer was non-empty, nor the update of the tail index once the entry had been processed. The former could lead to stale data being returned, while the latter could lead to memory corruption on sufficiently weakly ordered architectures.

Ugh.

This almost looks sane, but what's the odds there is some other issue in here as well? Would it make sense to just convert the code to use the "standard" ring buffer code instead?

Quoted 11 lines · Greg Kroah-Hartman

Quoted 9 lines · Johan Hovold

The custom ring-buffer implementation was merged without any locking whatsoever, but a spinlock was later added by commit 9d33efd9a791 ("USB: ldusb bugfix").

The lock did not cover the loads from the ring-buffer entry after determining the buffer was non-empty, nor the update of the tail index once the entry had been processed. The former could lead to stale data being returned, while the latter could lead to memory corruption on sufficiently weakly ordered architectures.

Quoted 5 lines

Ugh.

This almost looks sane, but what's the odds there is some other issue in here as well? Would it make sense to just convert the code to use the "standard" ring buffer code instead?

Yeah, long term that may be the right thing to do, but I wanted a minimal fix addressing the issue at hand without having to reimplement the driver and fix all other (less-critical) issues in there...

For the ring-buffer corruption / info-leak issue, these two patches should be sufficient though.

Copying the ring-buffer entry to a temporary buffer while holding the lock might still be preferred to avoid having to deal with barrier subtleties. But unless someone speaks out against 2/2, I'd just go ahead and apply it.

The custom ring-buffer implementation was merged without any locking whatsoever, but a spinlock was later added by commit 9d33efd9a791 ("USB: ldusb bugfix").

The lock did not cover the loads from the ring-buffer entry after determining the buffer was non-empty, nor the update of the tail index once the entry had been processed. The former could lead to stale data being returned, while the latter could lead to memory corruption on sufficiently weakly ordered architectures.

Fixes 2824bd250f0b ("[PATCH] USB: add ldusb driver")

Fixes 9d33efd9a791 ("USB: ldusb bugfix")

Cc stable <[email protected]> # 2.6.13

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/ldusb.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/usb/misc/ldusb.c +12 -3

index 15b5f06fb0b3..6b5843b0071e 100644
@@ -477,11 +477,11 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
 
 		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 > dev->interrupt_in_endpoint_size) {
+		spin_unlock_irq(&dev->rbsl);
 		retval = -EIO;
 		goto unlock_exit;
 	}

@@ -489,17 +489,26 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
 	if (bytes_to_read < *actual_buffer)
 		dev_warn(&dev->intf->dev, "Read buffer overflow, %zd bytes dropped\n",
 			 *actual_buffer-bytes_to_read);
+	spin_unlock_irq(&dev->rbsl);
+
+	/*
+	 * Pairs with spin_unlock_irqrestore() in
+	 * ld_usb_interrupt_in_callback() and makes sure the ring-buffer entry
+	 * has been updated before copy_to_user().
+	 */
+	smp_rmb();
 
 	/* copy one interrupt_in_buffer from ring_buffer into userspace */
 	if (copy_to_user(buffer, actual_buffer+1, bytes_to_read)) {
 		retval = -EFAULT;
 		goto unlock_exit;
 	}
-	dev->ring_tail = (dev->ring_tail+1) % ring_buffer_size;
-
 	retval = bytes_to_read;
 
 	spin_lock_irq(&dev->rbsl);
+
+	dev->ring_tail = (dev->ring_tail + 1) % ring_buffer_size;
+
 	if (dev->buffer_overflow) {
 		dev->buffer_overflow = 0;
 		spin_unlock_irq(&dev->rbsl);

Quoted 9 lines · Johan Hovold

The custom ring-buffer implementation was merged without any locking whatsoever, but a spinlock was later added by commit 9d33efd9a791 ("USB: ldusb bugfix").

The lock did not cover the loads from the ring-buffer entry after determining the buffer was non-empty, nor the update of the tail index once the entry had been processed. The former could lead to stale data being returned, while the latter could lead to memory corruption on sufficiently weakly ordered architectures.

Let's see if I understand this correctly.

The completion routine stores a buffer-length value at the location actual_buffer points to, and it stores the buffer contents themselves in the immediately following bytes. All this happens while the dev->rbsl spinlock is held.

Later on the read routine loads a value from *actual_buffer while holding the spinlock, but drops the spinlock before copying the immediately following buffer contents to userspace.

Your question is whether the read routine needs to call smp_rmb() after dropping the spinlock and before doing copy_to_user(), right?

The answer is: No, smp_rmb() isn't needed. All the data stored while ld_usb_interrupt_in_callback() held the spinlock will be visible to ld_usb_read() while it holds the spinlock and afterward (assuming the critical section in ld_usb_read() runs after the critical section in ld_usb_interrupt_in_callback() -- but you know this is true because of the value you read from *actual_buffer).

Quoted 55 lines

Fixes 2824bd250f0b ("[PATCH] USB: add ldusb driver")

Fixes 9d33efd9a791 ("USB: ldusb bugfix")

Cc stable <[email protected]> # 2.6.13

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/misc/ldusb.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/usb/misc/ldusb.c +12 -3

index 15b5f06fb0b3..6b5843b0071e 100644
@@ -477,11 +477,11 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
 
 		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 > dev->interrupt_in_endpoint_size) {
+		spin_unlock_irq(&dev->rbsl);
 		retval = -EIO;
 		goto unlock_exit;
 	}

@@ -489,17 +489,26 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
 	if (bytes_to_read < *actual_buffer)
 		dev_warn(&dev->intf->dev, "Read buffer overflow, %zd bytes dropped\n",
 			 *actual_buffer-bytes_to_read);
+	spin_unlock_irq(&dev->rbsl);
+
+	/*
+	 * Pairs with spin_unlock_irqrestore() in
+	 * ld_usb_interrupt_in_callback() and makes sure the ring-buffer entry
+	 * has been updated before copy_to_user().
+	 */
+	smp_rmb();
 
 	/* copy one interrupt_in_buffer from ring_buffer into userspace */
 	if (copy_to_user(buffer, actual_buffer+1, bytes_to_read)) {
 		retval = -EFAULT;
 		goto unlock_exit;
 	}
-	dev->ring_tail = (dev->ring_tail+1) % ring_buffer_size;
-
 	retval = bytes_to_read;
 
 	spin_lock_irq(&dev->rbsl);
+
+	dev->ring_tail = (dev->ring_tail + 1) % ring_buffer_size;
+
 	if (dev->buffer_overflow) {
 		dev->buffer_overflow = 0;
 		spin_unlock_irq(&dev->rbsl);