Re: [Outreachy kernel] [PATCH] Staging: iio: trigger: Use devm functions

[Alison Schofield <amsfield22@gmail.com>]

On Mon, Sep 19, 2016 at 10:06:08PM +0200, Lars-Peter Clausen wrote:
> On 09/19/2016 09:29 PM, Jonathan Cameron wrote:
> > On 19/09/16 10:21, Julia Lawall wrote:
> >>
> >>
> >> On Mon, 19 Sep 2016, Bhumika Goyal wrote:
> >>
> >>> On Mon, Sep 19, 2016 at 11:14 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> >>>> On Mon, Sep 19, 2016 at 02:46:50AM +0530, Bhumika Goyal wrote:
> >>>>> Use managed resource functions devm_iio_trigger_alloc and
> >>>>> devm_request_irq instead of iio_trigger_alloc and request_irq.
> >>>>> Remove corresponding calls to free_irq in the probe and remove
> >>>>> functions. Drop the now unnecessary goto labels and replace various
> >>>>> gotos with direct returns.
> >>>>> request_irq replacement done using coccinelle.
> >>>>
> >>>> Hi Bhumika,
> >>>> Thanks for the patch.  This seems like it's worthy of 2 separate
> >>>> patches.
> >>>>
> >>>> Also, I'm a bit confused with why I don't see an iio_trigger_free
> >>>> that is going away, when you switch to the devm_iio_trigger_alloc
> >>>> functions, but I realize there isn't one there to remove. hmm???
> >>>>
> >>>> alisons
> >>>>
> >>>
> >>> Hey Alison,
> >>> Ok, I will separate this in two patches. I will do
> >>> devm_iio_trigger_alloc in patch 1 and devm_request_irq in patch 2 as
> >>> devm_request_irq requires all previous memory allocations to be devms.
> >>> Regarding iio_trigger_free I didn't find any in either of the two
> >>> functions(probe and remove).
> >>> Thanks for the input.
> >>
> >> I think that normally people devmify everything at once.
> > I would as well.  It is 'kind of' one change...
> >>
> >> I dimly recollect that iio_trigger_unregister does the free.  Check
> >> whether this is the case, and whether it is devm safe.
> > Given it's referenced on the next line I certainly hope it doesn't ;)
> > There are quite a few reasons this one is in staging!
> 
> For some reason this driver uses iio_trigger_put() instead of
> iio_trigger_free(). Which is wrong, since iio_trigger_put() does an
> additional module_put() which would release a reference we've never gained.
> So the first fix should be to replace iio_trigger_put() with
> iio_trigger_free() (which is a patch for stable) and then switch to the devm
> version and remove the iio_trigger_free() (which is a patch for next).
> 
> This does not seem to be the only driver that gets this wrong,
> iio-trig-sysfs and iio-trig-interrupt are also affected. These would make a
> good target for further patches.
> 
> Bonus points if you can present a proof-of-concept that exploits this bug to
> run arbitrary code with kernel privileges.
> 
> There seems to be another bug regarding trigger reference counting in
> iio_trigger_write_current(). The reference to the trigger should be acquired
> in iio_trigger_find_by_name() while holding mutex. Otherwise we risk that
> the trigger is already freed before we grab the reference later in
> iio_trigger_write_current(). That's another opportunity for sending a good
> patch and also some opportunity for bonus points.
> 

Hi Bhumika,
I have interest in working this issue.  Let me know if it's ok for
me to run with.  
Thanks!
alisons