usb: using mutex lock and supporting O_NONBLOCK flag in iowarrior_read()

iowarrior_read() uses the iowarrior dev structure, but does not use any
lock on the structure. This can cause various bugs including data-races,
so it is more appropriate to use a mutex lock to safely protect the
iowarrior dev structure. When using a mutex lock, you should split the
branch to prevent blocking when the O_NONBLOCK flag is set.

In addition, it is unnecessary to check for NULL on the iowarrior dev
structure obtained by reading file->private_data. Therefore, it is
better to remove the check.

Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
Link: https://lore.kernel.org/r/20240919103403.3986-1-aha310510@gmail.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index 6d28467ce35227684e0281a39284eba87582195a..a513766b4985d097c2718451a1bea453e88339e9 100644 (file)
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -277,28 +277,45 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
        struct iowarrior *dev;
        int read_idx;
        int offset;
+       int retval;
 
        dev = file->private_data;
 
+       if (file->f_flags & O_NONBLOCK) {
+               retval = mutex_trylock(&dev->mutex);
+               if (!retval)
+                       return -EAGAIN;
+       } else {
+               retval = mutex_lock_interruptible(&dev->mutex);
+               if (retval)
+                       return -ERESTARTSYS;
+       }
+
        /* verify that the device wasn't unplugged */
-       if (!dev || !dev->present)
-               return -ENODEV;
+       if (!dev->present) {
+               retval = -ENODEV;
+               goto exit;
+       }
 
        dev_dbg(&dev->interface->dev, "minor %d, count = %zd\n",
                dev->minor, count);
 
        /* read count must be packet size (+ time stamp) */
        if ((count != dev->report_size)
-           && (count != (dev->report_size + 1)))
-               return -EINVAL;
+           && (count != (dev->report_size + 1))) {
+               retval = -EINVAL;
+               goto exit;
+       }
 
        /* repeat until no buffer overrun in callback handler occur */
        do {
                atomic_set(&dev->overflow_flag, 0);
                if ((read_idx = read_index(dev)) == -1) {
                        /* queue empty */
-                       if (file->f_flags & O_NONBLOCK)
-                               return -EAGAIN;
+                       if (file->f_flags & O_NONBLOCK) {
+                               retval = -EAGAIN;
+                               goto exit;
+                       }
                        else {
                                //next line will return when there is either new data, or the device is unplugged
                                int r = wait_event_interruptible(dev->read_wait,
@@ -309,28 +326,37 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
                                                                  -1));
                                if (r) {
                                        //we were interrupted by a signal
-                                       return -ERESTART;
+                                       retval = -ERESTART;
+                                       goto exit;
                                }
                                if (!dev->present) {
                                        //The device was unplugged
-                                       return -ENODEV;
+                                       retval = -ENODEV;
+                                       goto exit;
                                }
                                if (read_idx == -1) {
                                        // Can this happen ???
-                                       return 0;
+                                       retval = 0;
+                                       goto exit;
                                }
                        }
                }
 
                offset = read_idx * (dev->report_size + 1);
                if (copy_to_user(buffer, dev->read_queue + offset, count)) {
-                       return -EFAULT;
+                       retval = -EFAULT;
+                       goto exit;
                }
        } while (atomic_read(&dev->overflow_flag));
 
        read_idx = ++read_idx == MAX_INTERRUPT_BUFFER ? 0 : read_idx;
        atomic_set(&dev->read_idx, read_idx);
+       mutex_unlock(&dev->mutex);
        return count;
+
+exit:
+       mutex_unlock(&dev->mutex);
+       return retval;
 }
 
 /*