Skip to contentSkip to conversation

USB: ldusb: fix ring-buffer locking

PATCH New Johan Hovold 1 participant 3 messages Oct 22, 2019

Subsystems (2)
Stable branch USB
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

cover USB: ldusb: fix ring-buffer locking

Here's a fix for ring-buffer locking issue that was previously posted as an RFC (1/2), and a new related format specifier clean up.

Johan

Changes since RFC
- drop the head barrier bits which were not needed and update the commit message (1/2)
- clean up the format specifiers (2/2, new)

Johan Hovold (2):
USB: ldusb: fix ring-buffer locking
USB: ldusb: use unsigned size format specifiers

 drivers/usb/misc/ldusb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
Signature · 1 line
2.23.0
author
#1 lore raw

PATCH 1/2 USB: ldusb: fix ring-buffer locking

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

The lock did not cover the update of the tail index once the entry had been processed, something which could lead to memory corruption on weakly ordered architectures or due to compiler optimisations.

Specifically, a completion handler running on another CPU might observe the incremented tail index and update the entry before ld_usb_read() is done with it.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/misc/ldusb.c +2 -2
index 15b5f06fb0b3..c3e764909fd0 100644
@@ -495,11 +495,11 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
 		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);
Signature · 1 line
2.23.0
author
#2 lore raw

PATCH 2/2 USB: ldusb: use unsigned size format specifiers

In reply to Johan Hovold

A recent info-leak bug manifested itself along with warning about a negative buffer overflow:

	ldusb 1-1:0.28: Read buffer overflow, -131383859965943 bytes dropped

when it was really a rather large positive one.

A sanity check that prevents this has now been put in place, but let's fix up the size format specifiers, which should all be unsigned.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/misc/ldusb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/misc/ldusb.c +4 -3
index c3e764909fd0..dd1ea25e42b1 100644
@@ -487,7 +487,7 @@ static ssize_t ld_usb_read(struct file *file, char __user *buffer, size_t count,
 	}
 	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",
+		dev_warn(&dev->intf->dev, "Read buffer overflow, %zu bytes dropped\n",
 			 *actual_buffer-bytes_to_read);
 
 	/* copy one interrupt_in_buffer from ring_buffer into userspace */

@@ -562,8 +562,9 @@ static ssize_t ld_usb_write(struct file *file, const char __user *buffer,
 	/* write the data into interrupt_out_buffer from userspace */
 	bytes_to_write = min(count, write_buffer_size*dev->interrupt_out_endpoint_size);
 	if (bytes_to_write < count)
-		dev_warn(&dev->intf->dev, "Write buffer overflow, %zd bytes dropped\n", count-bytes_to_write);
-	dev_dbg(&dev->intf->dev, "%s: count = %zd, bytes_to_write = %zd\n",
+		dev_warn(&dev->intf->dev, "Write buffer overflow, %zu bytes dropped\n",
+			count - bytes_to_write);
+	dev_dbg(&dev->intf->dev, "%s: count = %zu, bytes_to_write = %zu\n",
 		__func__, count, bytes_to_write);
 
 	if (copy_from_user(dev->interrupt_out_buffer, buffer, bytes_to_write)) {
Signature · 1 line
2.23.0