Re: [PATCH] usb: use mutex_lock in iowarrior_read()

[Oliver Neukum <oneukum@suse.com>]

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@gmail.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:

 From 1627bc3a8e9aae60bdfc85430db2a44283e71a68 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.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@suse.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