linux-kernel - Re: [PATCH 10/15] iio: sx9310: Simplify error return handling

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]

Date:   Tue, 28 Jul 2020 14:32:28 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Daniel Campello <campello@...omium.org>
Cc:     LKML <devicetree@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Douglas Anderson <dianders@...omium.org>,
        Enrico Granata <egranata@...omium.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 10/15] iio: sx9310: Simplify error return handling

Quoting Daniel Campello (2020-07-28 14:23:29)
> On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@...omium.org> wrote:
> >
> > Quoting Daniel Campello (2020-07-28 08:12:53)
> > > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data)
> > >  static int sx9310_read_proximity(struct sx9310_data *data,
> > >                                  const struct iio_chan_spec *chan, int *val)
> > >  {
> > > -       int ret = 0;
> > > +       int ret;
> > >         __be16 rawval;
> > >
> > >         mutex_lock(&data->mutex);
> > >
> > >         ret = sx9310_get_read_channel(data, chan->channel);
> > > -       if (ret < 0)
> > > +       if (ret)
> > >                 goto out;
> > >
> > >         if (data->client->irq) {
> > > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data,
> > >
> > >         mutex_lock(&data->mutex);
> > >
> > > -       if (ret < 0)
> > > +       if (ret)
> > >                 goto out_disable_irq;
> >
> > Why is this condition checked after grabbing the mutex? Shouldn't it be
> > checked before grabbing the mutex? Or is that supposed to be a
> > mutex_unlock()?
> We acquire the lock before jumping to out_disable_irq which is before
> a mutex_unlock()

Does this function need to hold the mutex lock around get/put_read_channel?
It drops the lock while waiting and then regrabs it which seems to
imply that another reader could come in and try to get the channel again
during the wait. So put another way, it may be simpler to shorten the
lock area and then bail out of this function to a place where the lock
isn't held already on the return path.