Re: [Linux Kernel Bug][usb/f_printer] WARNING in usb_ep_queue

[Oliver Neukum]

On 02.02.24 19:59, Chenyuan Yang wrote:
> Dear Linux Developers for F_printer,
>
> We encountered "WARNING in usb_ep_queue" when testing the f_printer driver with
> Syzkaller and our generated specifications.

Hi,

it is clear what happens, but at least to me it is not clear why we allow
a write() before we enable the endpoint. Anyway, does this fix the issue?

	Regards
		Oliver
From 0008697b8ce373a0378058b60d1f1498c3821330 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@xxxxxxxx>
Date: Wed, 13 Mar 2024 16:15:56 +0100
Subject: [PATCH] usb: f_printer: sanity check in write

User space can trigger a write() before the endpoint needed
for that is enabled. Check for that.

Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
---
 drivers/usb/gadget/function/f_printer.c | 37 ++++++++++++++++---------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index 076dd4c1be96..1e266ba697e8 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -577,6 +577,7 @@ printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 	size_t			bytes_copied = 0;
 	struct usb_request	*req;
 	int			value;
+	int err = -ENODEV;
 
 	DBG(dev, "printer_write trying to send %d bytes\n", (int)len);
 
@@ -586,11 +587,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 error_spin;
 
 	/* Check if a printer reset happens while we have interrupts on */
 	dev->reset_printer = 0;
@@ -605,8 +603,8 @@ printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		 * a NON-Blocking call or not.
 		 */
 		if (fd->f_flags & (O_NONBLOCK|O_NDELAY)) {
-			mutex_unlock(&dev->lock_printer_io);
-			return -EAGAIN;
+			err = -EAGAIN;
+			goto error_mutex;
 		}
 
 		/* Sleep until a write buffer is available */
@@ -657,9 +655,17 @@ printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		/* We've disconnected or reset so free the req and buffer */
 		if (dev->reset_printer) {
 			list_add(&req->list, &dev->tx_reqs);
-			spin_unlock_irqrestore(&dev->lock, flags);
-			mutex_unlock(&dev->lock_printer_io);
-			return -EAGAIN;
+			err = -EAGAIN;
+			goto error_spin;
+		}
+
+		/*
+		 * We cannot guarantee user space is using the API nicely
+		 * This check needs to be duplicated
+		 */
+		if (!dev->in_ep->enabled && dev->in_ep->address) {
+			err = -ESHUTDOWN;
+			goto error_spin;
 		}
 
 		list_add(&req->list, &dev->tx_reqs_active);
@@ -670,9 +676,8 @@ printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		spin_lock(&dev->lock);
 		if (value) {
 			list_move(&req->list, &dev->tx_reqs);
-			spin_unlock_irqrestore(&dev->lock, flags);
-			mutex_unlock(&dev->lock_printer_io);
-			return -EAGAIN;
+			err = -EAGAIN;
+			goto error_spin;
 		}
 	}
 
@@ -685,6 +690,12 @@ printer_write(struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		return bytes_copied;
 	else
 		return -EAGAIN;
+
+error_spin:
+	spin_unlock_irqrestore(&dev->lock, flags);
+error_mutex:
+	mutex_unlock(&dev->lock_printer_io);
+	return err;
 }
 
 static int
-- 
2.44.0