diff mbox series

usb: gadget: printer: fix races against disable

Message ID 20240620114039.5767-1-oneukum@suse.com (mailing list archive)
State Accepted
Commit e587a7633dfee8987a999cf253f7c52a8e09276c
Headers show
Series usb: gadget: printer: fix races against disable | expand

Commit Message

Oliver Neukum June 20, 2024, 11:40 a.m. UTC
printer_read() and printer_write() guard against the race
against disable() by checking the dev->interface flag,
which in turn is guarded by a spinlock.
These functions, however, drop the lock on multiple occasions.
This means that the test has to be redone after reacquiring
the lock and before doing IO.

Add the tests.

This also addresses CVE-2024-25741

Fixes: 7f2ca14d2f9b9 ("usb: gadget: function: printer: Interface is disabled and returns error")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/gadget/function/f_printer.c | 39 ++++++++++++++++++-------
 1 file changed, 29 insertions(+), 10 deletions(-)

Comments

Greg Kroah-Hartman June 20, 2024, 12:28 p.m. UTC | #1
On Thu, Jun 20, 2024 at 01:40:26PM +0200, Oliver Neukum wrote:
> printer_read() and printer_write() guard against the race
> against disable() by checking the dev->interface flag,
> which in turn is guarded by a spinlock.
> These functions, however, drop the lock on multiple occasions.
> This means that the test has to be redone after reacquiring
> the lock and before doing IO.
> 
> Add the tests.
> 
> This also addresses CVE-2024-25741

What?  Why is MITRE assigning CVEs for the kernel now?  Who asked for
this and who assigned this?  Do I need to go poke someone with a big
stick?

thanks,

greg k-h
Oliver Neukum June 20, 2024, 1:11 p.m. UTC | #2
On 20.06.24 14:28, Greg KH wrote:

> What?  Why is MITRE assigning CVEs for the kernel now?  Who asked for
> this and who assigned this?  Do I need to go poke someone with a big
> stick?

I don't know about that. I just get the bug reports and this one
said that the issue is a CVE.

	Regards
		Oliver
Greg Kroah-Hartman June 20, 2024, 1:34 p.m. UTC | #3
On Thu, Jun 20, 2024 at 02:28:07PM +0200, Greg KH wrote:
> On Thu, Jun 20, 2024 at 01:40:26PM +0200, Oliver Neukum wrote:
> > printer_read() and printer_write() guard against the race
> > against disable() by checking the dev->interface flag,
> > which in turn is guarded by a spinlock.
> > These functions, however, drop the lock on multiple occasions.
> > This means that the test has to be redone after reacquiring
> > the lock and before doing IO.
> > 
> > Add the tests.
> > 
> > This also addresses CVE-2024-25741
> 
> What?  Why is MITRE assigning CVEs for the kernel now?  Who asked for
> this and who assigned this?  Do I need to go poke someone with a big
> stick?

Turns out it was allocated 1 day before kernel.org became the CNA, so
false alarm :)
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 4c0b7c2970f1..44e20c6c36d3 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -450,11 +450,8 @@  printer_read(struct file *fd, char __user *buf, size_t len, loff_t *ptr)
 	mutex_lock(&dev->lock_printer_io);
 	spin_lock_irqsave(&dev->lock, flags);
 
-	if (dev->interface < 0) {
-		spin_unlock_irqrestore(&dev->lock, flags);
-		mutex_unlock(&dev->lock_printer_io);
-		return -ENODEV;
-	}
+	if (dev->interface < 0)
+		goto out_disabled;
 
 	/* We will use this flag later to check if a printer reset happened
 	 * after we turn interrupts back on.
@@ -462,6 +459,9 @@  printer_read(struct file *fd, char __user *buf, size_t len, loff_t *ptr)
 	dev->reset_printer = 0;
 
 	setup_rx_reqs(dev);
+	/* this dropped the lock - need to retest */
+	if (dev->interface < 0)
+		goto out_disabled;
 
 	bytes_copied = 0;
 	current_rx_req = dev->current_rx_req;
@@ -495,6 +495,8 @@  printer_read(struct file *fd, char __user *buf, size_t len, loff_t *ptr)
 		wait_event_interruptible(dev->rx_wait,
 				(likely(!list_empty(&dev->rx_buffers))));
 		spin_lock_irqsave(&dev->lock, flags);
+		if (dev->interface < 0)
+			goto out_disabled;
 	}
 
 	/* We have data to return then copy it to the caller's buffer.*/
@@ -538,6 +540,9 @@  printer_read(struct file *fd, char __user *buf, size_t len, loff_t *ptr)
 			return -EAGAIN;
 		}
 
+		if (dev->interface < 0)
+			goto out_disabled;
+
 		/* If we not returning all the data left in this RX request
 		 * buffer then adjust the amount of data left in the buffer.
 		 * Othewise if we are done with this RX request buffer then
@@ -567,6 +572,11 @@  printer_read(struct file *fd, char __user *buf, size_t len, loff_t *ptr)
 		return bytes_copied;
 	else
 		return -EAGAIN;
+
+out_disabled:
+	spin_unlock_irqrestore(&dev->lock, flags);
+	mutex_unlock(&dev->lock_printer_io);
+	return -ENODEV;
 }
 
 static ssize_t
@@ -587,11 +597,8 @@  printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 	mutex_lock(&dev->lock_printer_io);
 	spin_lock_irqsave(&dev->lock, flags);
 
-	if (dev->interface < 0) {
-		spin_unlock_irqrestore(&dev->lock, flags);
-		mutex_unlock(&dev->lock_printer_io);
-		return -ENODEV;
-	}
+	if (dev->interface < 0)
+		goto out_disabled;
 
 	/* Check if a printer reset happens while we have interrupts on */
 	dev->reset_printer = 0;
@@ -614,6 +621,8 @@  printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		wait_event_interruptible(dev->tx_wait,
 				(likely(!list_empty(&dev->tx_reqs))));
 		spin_lock_irqsave(&dev->lock, flags);
+		if (dev->interface < 0)
+			goto out_disabled;
 	}
 
 	while (likely(!list_empty(&dev->tx_reqs)) && len) {
@@ -663,6 +672,9 @@  printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 			return -EAGAIN;
 		}
 
+		if (dev->interface < 0)
+			goto out_disabled;
+
 		list_add(&req->list, &dev->tx_reqs_active);
 
 		/* here, we unlock, and only unlock, to avoid deadlock. */
@@ -675,6 +687,8 @@  printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 			mutex_unlock(&dev->lock_printer_io);
 			return -EAGAIN;
 		}
+		if (dev->interface < 0)
+			goto out_disabled;
 	}
 
 	spin_unlock_irqrestore(&dev->lock, flags);
@@ -686,6 +700,11 @@  printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		return bytes_copied;
 	else
 		return -EAGAIN;
+
+out_disabled:
+	spin_unlock_irqrestore(&dev->lock, flags);
+	mutex_unlock(&dev->lock_printer_io);
+	return -ENODEV;
 }
 
 static int