Commit 44feafba authored by Jeongjun Park's avatar Jeongjun Park Committed by Greg Kroah-Hartman
Browse files

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.

Fixes: 946b960d ("USB: add driver for iowarrior devices.")
Signed-off-by: default avatarJeongjun Park <aha310510@gmail.com>
Link: https://lore.kernel.org/r/20240919103403.3986-1-aha310510@gmail.com


Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 67c6150c

drivers/usb/misc/iowarrior.c

+36 −10
Original line number Diff line number Diff line
@@ -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;

}



/*