linux-kernel - Re: [PATCH] usb: use mutex_lock in iowarrior

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO9qdTHPA6cUWc+T8pcO8_tUpJ5PZ4UgmyP6oA+R5bEH8nX5pQ@mail.gmail.com>
Date: Mon, 16 Sep 2024 13:43:22 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: colin.i.king@...il.com, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: use mutex_lock in iowarrior_read()

Greg KH <gregkh@...uxfoundation.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@...il.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.

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