linux-kernel - Re: [PATCH v2] iio: proximity: as3935: fix use-after-free on device remove

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: <CAGngYiVgEDTQxynjPDMO_ZApAX6uyvUL+iqyK-QHnhDz=E4eCw@mail.gmail.com>
Date:   Fri, 8 Mar 2019 15:42:04 -0500
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     Tomasz Duszynski <tduszyns@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Jonathan Cameron <jonathan.cameron@...wei.com>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Matt Ranostay <matt.ranostay@...sulko.com>
Subject: Re: [PATCH v2] iio: proximity: as3935: fix use-after-free on device remove

On Fri, Mar 8, 2019 at 3:30 PM Tomasz Duszynski <tduszyns@...il.com> wrote:
>
> > @@ -368,7 +376,6 @@ static int as3935_probe(struct spi_device *spi)
> >
> >       spi_set_drvdata(spi, indio_dev);
> >       mutex_init(&st->lock);
> > -     INIT_DELAYED_WORK(&st->work, as3935_event_work);
>
> Any specific reason for moving this elsewhere?

Yes. On the remove path, cancel_delayed_work_sync() should execute after
free_irq(), but before triggered_buffer_cleanup(). So the devm_add_action()
must run right before devm_request_irq(). I figured it would make sense to
group the devm_add_action() and INIT_WORK() together, as they are
related. This also makes it easier to understand the probe/remove order
when reading the code.

> >
> >       ret = of_property_read_u32(np,
> >                       "ams,tuning-capacitor-pf", &st->tune_cap);
> > @@ -414,22 +421,27 @@ static int as3935_probe(struct spi_device *spi)
> >       iio_trigger_set_drvdata(trig, indio_dev);
> >       trig->ops = &iio_interrupt_trigger_ops;
> >
> > -     ret = iio_trigger_register(trig);
> > +     ret = devm_iio_trigger_register(&spi->dev, trig);
> >       if (ret) {
> >               dev_err(&spi->dev, "failed to register trigger\n");
> >               return ret;
> >       }
> >
> > -     ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> > -             &as3935_trigger_handler, NULL);
> > +     ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > +             iio_pollfunc_store_time, as3935_trigger_handler, NULL);
>
> You can fix arguments alignment while you are at it.
>

What type of alignment would you prefer? This?

        ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
                                              iio_pollfunc_store_time,
                                              as3935_trigger_handler, NULL);