linux-kernel - Re: [PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod

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:   Sun, 19 Mar 2023 15:32:03 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Patrik Dahlström <risca@...akolonin.se>
Cc:     lars@...afoo.de, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, hns@...delico.com
Subject: Re: [PATCH] iio: adc: palmas_gpadc: fix NULL dereference on rmmod

On Sat, 18 Mar 2023 20:22:53 +0100
Patrik Dahlström <risca@...akolonin.se> wrote:

> On Sat, Mar 18, 2023 at 04:30:33PM +0000, Jonathan Cameron wrote:
> > On Mon, 13 Mar 2023 21:50:29 +0100
> > Patrik Dahlström <risca@...akolonin.se> wrote:
> >   
> > > Calling dev_to_iio_dev() on a platform device pointer is undefined and
> > > will make adc NULL.
> > > 
> > > Signed-off-by: Patrik Dahlström <risca@...akolonin.se>  
> > 
> > Hi Patrik,
> > 
> > Looks good so applied to the fixes-togreg branch of iio.git.
> > 
> > Whilst we are here, this would be a trivial driver to take fully device
> > managed.  The only slightly messy bit is that it would need
> > a devm_add_action_or_reset() + custom callback to handle the
> > device_wakeup_enable().
> > 
> > On the off chance you can test it I'll send a patch in a few mins.
> > Note that will depend on this one going up stream first and that
> > I haven't done more than build test it.  
> I got the patch and it looks good, but it will take a few days before I
> have the time to test it.
> 
> I have some more patches coming for this driver to configure the adc
> thresholds from userspace, employing the iio channel event subsystem, but
> they need a bit more work. In particular, to ensure backwards compatibility
> with the adc_wakeupX_data platform data. However, I don't see this platform
> data being used by anyone.
> How important is it to retain support for adc_wakeupX_data?

It's a somewhat unusual feature, so I doubt it was implemented without someone
needing it.  However as you observe there is no upstream user.

As it is causing you problems, I'd just rip out the palmas_adc_platform_data
completely and see if anyone objects.  You can do that as a standalone patch
prior to posting your events stuff if you like.  Or hopefully
H. Nikolaus Schaller might be able to give us some background on why
that feature is there but not used.

> > 
> > Thanks,
> > 
> > Jonathan  
> 
> Thank you for going the extra mile :)

No problem.  I jumped on the opportunity to get it tested - takes way longer
than writing a little patch like that ;)

Jonathan

> > 
> >   
> > > ---
> > >  drivers/iio/adc/palmas_gpadc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
> > > index 61e80bf3d05e..6db6f3bc768a 100644
> > > --- a/drivers/iio/adc/palmas_gpadc.c
> > > +++ b/drivers/iio/adc/palmas_gpadc.c
> > > @@ -638,7 +638,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> > >  
> > >  static int palmas_gpadc_remove(struct platform_device *pdev)
> > >  {
> > > -	struct iio_dev *indio_dev = dev_to_iio_dev(&pdev->dev);
> > > +	struct iio_dev *indio_dev = dev_get_drvdata(&pdev->dev);
> > >  	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > >  
> > >  	if (adc->wakeup1_enable || adc->wakeup2_enable)  
> >