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

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Sep 16, 2024 at 01:43:22PM +0900, Jeongjun Park wrote:
> Greg KH <gregkh@linuxfoundation.org> 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?
> >
> > 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.

But if you don't have a report, and don't have this device, how can you
test this to make sure?

> There is no reason why this function should not use mutex_lock,
> so I think adding a lock is appropriate.

Fair enough, but do it properly please.

> > > +             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?

What happens if the private_data pointer becomes "stale" right after
checking it is not NULL?  You need to explain how it is safe, if it is,
to do this.

Actually, what ever sets this to NULL?  I think this check isn't needed
at all from looking at the code (hint, think about the lifetime of the
file pointer...)

> > > +     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?

Unless you have the hardware to test this, I would not worry about doing
conversions like this.  I think I have this device somewhere around in
my "big box of USB devices", but testing any driver changes for it will
take a while before I can find it.

Actually, in looking at the code further, I think the lock is not taken
on purpose, so if you want to change this, you will have to document why
it is now really needed and what will happen if it is not.

thanks,

greg k-h