lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAO9qdTHrbG-aWetpM_e7zHUhrwPD=7uCHPbWSMoorgnwjKEOmA@mail.gmail.com>
Date: Mon, 16 Sep 2024 21:44:28 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Oliver Neukum <oneukum@...e.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, colin.i.king@...il.com, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: use mutex_lock in iowarrior_read()

Oliver Neukum <oneukum@...e.com> wrote:
>
>
>
> On 16.09.24 06:15, Greg KH wrote:
> > On Mon, Sep 16, 2024 at 01:06:29PM +0900, Jeongjun Park wrote:
> >> Currently, iowarrior_read() does not provide any protection for the
> >> iowarrior structure, so the iowarrior structure is vulnerable to data-race.
> >>
> >> Therefore, I think it is appropriate to protect the structure using
> >> mutex_lock in iowarrior_read().
> >>
> >> Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.")
> >> Signed-off-by: Jeongjun Park <aha310510@...il.com>
> >> ---
> >>   drivers/usb/misc/iowarrior.c | 42 +++++++++++++++++++++++++++---------
> >>   1 file changed, 32 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
> >> index 6d28467ce352..7f3d37b395c3 100644
> >> --- a/drivers/usb/misc/iowarrior.c
> >> +++ b/drivers/usb/misc/iowarrior.c
> >> @@ -277,28 +277,41 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
> >>      struct iowarrior *dev;
> >>      int read_idx;
> >>      int offset;
> >> +    int retval = 0;
> >>
> >>      dev = file->private_data;
> >>
> >> +    if (!dev) {
> >
> > How can this happen?  How was this tested?
>
> It cannot happen.
>
> [..]
> >> +    mutex_lock(&dev->mutex);
> >
> > Please use the guard() form here, it makes the change much simpler and
> > easier to review and maintain.
>
> That would break the O_NONBLOCK case.
>
> Looking at the code it indeed looks like iowarrior_read() can race
> with itself. Strictly speaking it always could happen if a task used
> fork() after open(). The driver tries to restrict its usage to one
> thread, but I doubt that the logic is functional.
>
> It seems to me the correct fix is something like this:

Well, I don't know why it's necessary to modify it like this.
I think it would be more appropriate to patch it to make it
more maintainable by using guard() as Greg suggested.


>
>  From 1627bc3a8e9aae60bdfc85430db2a44283e71a68 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@...e.com>
> Date: Thu, 12 Sep 2024 12:47:33 +0200
> Subject: [PATCH] iowarrior: fix read racing against itself case
>
> In a multithreaded application iowarrior_read() can race against itself.
> It needs to take the mutex.
>
> Signed-off-by: Oliver Neukum <oneukum@...e.com>
> ---
>   drivers/usb/misc/iowarrior.c | 33 ++++++++++++++++++++++++++-------
>   1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
> index 6d28467ce352..3b49d6c7b569 100644
> --- a/drivers/usb/misc/iowarrior.c
> +++ b/drivers/usb/misc/iowarrior.c
> @@ -277,6 +277,8 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
>         struct iowarrior *dev;
>         int read_idx;
>         int offset;
> +       int result;
> +       bool nonblock = file->f_flags & O_NONBLOCK;
>
>         dev = file->private_data;
>
> @@ -292,14 +294,25 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
>             && (count != (dev->report_size + 1)))
>                 return -EINVAL;
>
> +       if (nonblock) {
> +               result = mutex_trylock(&dev->mutex);
> +               if (!result)
> +                       return -EAGAIN;
> +               result = 0;
> +       } else {
> +               result = mutex_lock_interruptible(&dev->mutex);
> +               if (result < 0)
> +                       return -EINTR;
> +       }
>         /* 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;
> -                       else {
> +                       if (nonblock) {
> +                               result = -EAGAIN;
> +                               goto out;
> +                       } else {
>                                 //next line will return when there is either new data, or the device is unplugged
>                                 int r = wait_event_interruptible(dev->read_wait,
>                                                                  (!dev->present
> @@ -309,14 +322,17 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
>                                                                   -1));
>                                 if (r) {
>                                         //we were interrupted by a signal
> -                                       return -ERESTART;
> +                                       result = -ERESTART;
> +                                       goto out;
>                                 }
>                                 if (!dev->present) {
>                                         //The device was unplugged
> -                                       return -ENODEV;
> +                                       result = -ENODEV;
> +                                       goto out;
>                                 }
>                                 if (read_idx == -1) {
>                                         // Can this happen ???
> +                                       mutex_unlock(&dev->mutex);
>                                         return 0;
>                                 }
>                         }
> @@ -324,13 +340,16 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
>
>                 offset = read_idx * (dev->report_size + 1);
>                 if (copy_to_user(buffer, dev->read_queue + offset, count)) {
> -                       return -EFAULT;
> +                       result = -EFAULT;
> +                       goto out;
>                 }
>         } while (atomic_read(&dev->overflow_flag));
>
>         read_idx = ++read_idx == MAX_INTERRUPT_BUFFER ? 0 : read_idx;
>         atomic_set(&dev->read_idx, read_idx);
> -       return count;
> +out:
> +       mutex_unlock(&dev->mutex);
> +       return result < 0 ? result : count;
>   }
>
>   /*
> --
> 2.45.2
>
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ