Message-ID: <CAO9qdTHgSwtaVfwzUYgSNX_3Yx=hmyYQnUb-OpP6k2u_gRZVGg@mail.gmail.com>
Date: Wed, 16 Oct 2024 23:52:47 +0900
From: Jeongjun Park <aha310510@...il.com>
To: gregkh@...uxfoundation.org, oneukum@...e.com
Cc: colin.i.king@...il.com, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] usb: using mutex lock and supporting O_NONBLOCK flag
 in iowarrior_read()

Jeongjun Park <aha310510@...il.com> wrote:
>
> 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: stable@...r.kernel.org
> Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.")
> Signed-off-by: Jeongjun Park <aha310510@...il.com>

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.

Regards,

Jeongjun Park

> ---
> v1 -> v2: Added cc tag and change log
>
>  drivers/usb/misc/iowarrior.c | 46 ++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
> index 6d28467ce352..a513766b4985 100644
> --- 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;
>  }
>
>  /*
> --

Powered by blists - more mailing lists