linux-kernel - Re: A potential data race in drivers/iio/adc/berlin2-adc.ko

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] [day] [month] [year] [list]

Message-ID: <20210320145353.5a82a34d@jic23-huawei>
Date:   Sat, 20 Mar 2021 14:53:53 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Lars-Peter Clausen <lars@...afoo.de>
Cc:     Pavel Andrianov <andrianov@...ras.ru>,
        ldv-project@...uxtesting.org,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: A potential data race in drivers/iio/adc/berlin2-adc.ko

On Thu, 18 Mar 2021 09:47:29 +0100
Lars-Peter Clausen <lars@...afoo.de> wrote:

> On 3/18/21 9:27 AM, Lars-Peter Clausen wrote:
> > On 3/18/21 9:07 AM, Pavel Andrianov wrote:  
> >> Hi,
> >>
> >> berlin2_adc_probe [1] registers two interrupt handlers: 
> >> berlin2_adc_irq [2]
> >> and berlin2_adc_tsen_irq [3]. The interrupt handlers operate with the 
> >> same data, for example, modify
> >> priv->data with different masks:
> >>
> >> priv->data &= BERLIN2_SM_ADC_MASK;
> >> and
> >> priv->data &= BERLIN2_SM_TSEN_MASK;
> >>
> >> If the two interrupt handlers are executed simultaneously, a 
> >> potential data race takes place. So, the question is if the situation 
> >> is possible. For example, in the case of the handlers are executed on 
> >> different CPU cores.

If we assume there is a race here, the reading into priv->data from
two different registers on the line above the masking is more of any issue.

> >>
> >> Best regards,
> >> Pavel
> >>
> >> [1] 
> >> https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L283 
> >>
> >> [2] 
> >> https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L239 
> >>
> >> [3] 
> >> https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L259
> >>  
> > Looking at the code there are two functions. berlin2_adc_tsen_read() 
> > and berlin2_adc_read(). These two function are take the same mutex and 
> > can not run concurrently. At the beginning of the protected section 
> > the corresponding interrupt for that function is enabled and at the 
> > end disabled. So at least if the hardware works correctly those two 
> > interrupts will never fire at the same time.
> >
> > Now, if the hardware misbehaves the two interrupts could still fire at 
> > the same time.
> >
> > - Lars
> >  
> Actually thinking a bit more about this the interrupt could still fire 
> after it has been disabled since there is no synchronization between the 
> disable and the interrupt handler. And the handler might be queued on 
> one CPU, while the disable is running on another CPU.
> 
Superficially it looks like splitting the priv->data and related priv->data_available
into versions for the normal ADC and the touch screen ADC paths should solve
this at the trivial cost of a couple of elements in that structure.
Possibly also need to deal with the wait_queue but I think that's fine as is.
(haven't thought about it that much!)

Jonathan