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

From: Greg KH
Date: Mon Sep 16 2024 - 00:15:35 EST


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@xxxxxxxxx>
> ---
> 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?

And you didn't mention this in your changelog, why?

> + retval = -ENODEV;
> + goto exit;
> + }

What prevents dev from becoming invalid after it is checked here?

> +
> + mutex_lock(&dev->mutex);

Please use the guard() form here, it makes the change much simpler and
easier to review and maintain.

thanks,

greg k-h