Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@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?
There is no separate reproduction code or bug report. However, all other
functions in iowarrior use mutex_lock to protect the iowarrior structure.
Only iowarrior_read does not use mutex_lock, which could potentially cause
bugs.
There is no reason why this function should not use mutex_lock,
so I think adding a lock is appropriate.
>
> > + retval = -ENODEV;
> > + goto exit;
> > + }
>
> What prevents dev from becoming invalid after it is checked here?
I'm not sure what this means. Can you explain it in more detail?
>
> > +
> > + mutex_lock(&dev->mutex);
>
> Please use the guard() form here, it makes the change much simpler and
> easier to review and maintain.
I didn't know such a convenient function existed. It certainly seems like
it would make maintenance easier, but it also seems like it would be a
good idea to consistently replace all mutex_locks in iowarrior.c with guard().
What do you think?
Regards,
Jeongjun Park
>
> thanks,
>
> greg k-h
