linux-kernel - Re: [PATCH 1/5] gnss: sirf: write data to gnss only when the gnss device is open

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:   Wed, 5 Dec 2018 21:14:42 +0100
From:   Andreas Kemnade <andreas@...nade.info>
To:     Johan Hovold <johan@...nel.org>
Cc:     robh+dt@...nel.org, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>
Subject: Re: [PATCH 1/5] gnss: sirf: write data to gnss only when the gnss
 device is open

On Wed, 5 Dec 2018 15:47:39 +0100
Johan Hovold <johan@...nel.org> wrote:

> On Sun, Nov 18, 2018 at 10:57:57PM +0100, Andreas Kemnade wrote:
> > The api forbids writing data there otherwise. Prepare for the
> > serdev_open()/close() being a part of runtime pm.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@...nade.info>
> > ---
> >  drivers/gnss/sirf.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)  
>  
> > @@ -73,6 +76,7 @@ static void sirf_close(struct gnss_device *gdev)
> >  	serdev_device_close(serdev);
> >  
> >  	pm_runtime_put(&serdev->dev);
> > +	data->opened = false;
> >  }
> >  
> >  static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
> > @@ -105,7 +109,17 @@ static int sirf_receive_buf(struct serdev_device *serdev,
> >  	struct sirf_data *data = serdev_device_get_drvdata(serdev);
> >  	struct gnss_device *gdev = data->gdev;
> >  
> > -	return gnss_insert_raw(gdev, buf, count);
> > +	/*
> > +	 * we might come here everytime when runtime is resumed
> > +	 * and data is received. Two cases are possible
> > +	 * 1. device is opened during initialisation
> > +	 * 2. kernel is compiled without runtime pm
> > +	 *    and device is opened all the time
> > +	 */
> > +	if (data->opened)
> > +		return gnss_insert_raw(gdev, buf, count);  
> 
> This can race with sirf_close() when you move serdev handling out of
> sirf_open()/close(). Not sure how best to handle that yet.
> 
Ok, first lets check whether we have a common idea of the problem before
coming to a solution. So race condition here can happen if the serdev
is still opened after the pm_runtime_put in sirf_close() which might
happen if runtime is not suspended after that. I missed that case.
Then if (data->opened) 
is checked, data->opened set to false
then gnss_insert_raw() is executed.
There is data inserted into the fifo which will be read by the next one
opening the gnss device. And there might be trouble at deregistering
time.

And now if we simply add locks:
For sirf_receive_buf(), it somehow feels dangerous if we call
gnss_insert_raw() with a lock held but I have not analyzed it
thoroughly. For sirf_close(), we could simply
put a lock around data->opened = false;
Am I missing something?

I will check if anything changes when I move the serdev_open()/close()
calls a bit. In the end me end to have the serdev open whenever the
userspace wants data or we want to change the state of the gnss chip
without wakeup.
Maybe add a special driver state for that situation where the power
state of the gps chip is changed without having it opened. If we check
that at the beginning of that action, we might have luck.

Regards,
Andreas

Content of type "application/pgp-signature" skipped