* [PATCH v2] usb: using mutex lock and supporting O_NONBLOCK flag in iowarrior_read()
@ 2024-09-19 10:34 Jeongjun Park
2024-10-16 14:52 ` Jeongjun Park
0 siblings, 1 reply; 3+ messages in thread
From: Jeongjun Park @ 2024-09-19 10:34 UTC (permalink / raw)
To: gregkh, oneukum
Cc: colin.i.king, linux-usb, linux-kernel, stable, 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.
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@vger.kernel.org
Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
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;
}
/*
--
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] usb: using mutex lock and supporting O_NONBLOCK flag in iowarrior_read()
2024-09-19 10:34 [PATCH v2] usb: using mutex lock and supporting O_NONBLOCK flag in iowarrior_read() Jeongjun Park
@ 2024-10-16 14:52 ` Jeongjun Park
2024-10-16 15:34 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Jeongjun Park @ 2024-10-16 14:52 UTC (permalink / raw)
To: gregkh, oneukum; +Cc: colin.i.king, linux-usb, linux-kernel, stable
Jeongjun Park <aha310510@gmail.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@vger.kernel.org
> Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.")
> Signed-off-by: Jeongjun Park <aha310510@gmail.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;
> }
>
> /*
> --
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] usb: using mutex lock and supporting O_NONBLOCK flag in iowarrior_read()
2024-10-16 14:52 ` Jeongjun Park
@ 2024-10-16 15:34 ` Greg KH
0 siblings, 0 replies; 3+ messages in thread
From: Greg KH @ 2024-10-16 15:34 UTC (permalink / raw)
To: Jeongjun Park; +Cc: oneukum, colin.i.king, linux-usb, linux-kernel, stable
On Wed, Oct 16, 2024 at 11:52:47PM +0900, Jeongjun Park wrote:
> Jeongjun Park <aha310510@gmail.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@vger.kernel.org
> > Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.")
> > Signed-off-by: Jeongjun Park <aha310510@gmail.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.
Do you have this hardware to test this with? What type of data race
will happen for a normal user of it? What systems that have this
hardware allow untrusted users to operate this hardware?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-16 15:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-19 10:34 [PATCH v2] usb: using mutex lock and supporting O_NONBLOCK flag in iowarrior_read() Jeongjun Park
2024-10-16 14:52 ` Jeongjun Park
2024-10-16 15:34 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox