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: <2024091659-showing-bulge-196b@gregkh>
Date: Mon, 16 Sep 2024 08:50:09 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Jeongjun Park <aha310510@...il.com>
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()

On Mon, Sep 16, 2024 at 01:43:22PM +0900, Jeongjun Park wrote:
> 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.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ