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
