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.

Cc [email protected]

Fixes 946b960d13c1 ("USB: add driver for iowarrior devices.")

Signed-off-by Jeongjun Park <[email protected]>

---
v1 -> v2: Added cc tag and change log

 drivers/usb/misc/iowarrior.c | 46 ++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 10 deletions(-)

drivers/usb/misc/iowarrior.c +36 -11

index 6d28467ce352..a513766b4985 100644
@@ -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;
 }
 
 /*
--

Quoted 7 lines · Jeongjun Park

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.

Quoted 7 lines

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.

Cc [email protected]

Fixes 946b960d13c1 ("USB: add driver for iowarrior devices.")

Signed-off-by Jeongjun Park <[email protected]>

I think this patch should be moved to the usb-linus tree to be applied in the next rc version. iowarrior_read() is very vulnerable to a data-race because it reads a struct iowarrior without a mutex_lock. I think this almost certainly leads to a data-race, so I think this function should be moved to the usb-linus tree to be fixed as soon as possible.

I would appreciate it if you could review this.