linux-kernel - Re: [BUG] iio: light: opt3001: possible deadlock in opt3001_read_raw() and opt3001

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]

Date:   Mon, 7 Feb 2022 20:39:10 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Jia-Ju Bai <baijiaju1990@...il.com>
Cc:     lars@...afoo.de, valek@...cz, gwendal@...omium.org,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [BUG] iio: light: opt3001: possible deadlock in
 opt3001_read_raw() and opt3001_irq()

On Mon, 7 Feb 2022 23:41:49 +0800
Jia-Ju Bai <baijiaju1990@...il.com> wrote:

> Hello,
> 
> My static analysis tool reports a possible deadlock in the opt3001 
> driver in Linux 5.16:
> 
> opt3001_read_raw()
>    mutex_lock(&opt->lock); --> Line 399 (Lock A)
>    opt3001_get_lux()
>      wait_event_timeout(opt->result_ready_queue, ...) --> Line 276 (Wait X)
>    mutex_lock(&opt->lock); --> Line 412 (Unlock A)
> 
> opt3001_irq()
>    mutex_lock(&opt->lock); --> Line 693 (Lock A)
>    mutex_unlock(&opt->lock); --> Line 730 (Unlock A)
>    wake_up(&opt->result_ready_queue); --> Line 733 (Wake X)
> 
> When opt3001_read_raw() is executed, "Wait X" is performed by holding 
> "Lock A". If opt3001_irq() is executed at this time, "Wake X" cannot be 
> performed to wake up "Wait X" in opt3001_read_raw(), because "Lock A" 
> has been already hold by opt3001_read_raw(), causing a possible deadlock.
> I find that "Wait X" is performed with a timeout, to relieve the 
> possible deadlock; but I think this timeout can cause inefficient execution.
Hi Jia-Ju Bai,

There is a quirk in here thatyou haven't mentioned.  The "magic"
opt->ok_to_ignore_lock.

So there are two cases:
1) No irq in use.  In that case the opt3001_irq() will never run and we
   just sleep + check a status flag. That's the best we can do without
   an interrupt.
2) irq in use, the ok_to_ignore_lock = true statement occurs and
   in the opt3001_irq() the locks are never taken hence no deadlock.

It is a very odd bit of code though so I'd be surprised if a static
analyser hadn't highlighted it as a possible deadlock!

Now, I'm not immediately sure why the driver is done like this as opposed
to a more complete wait_for_completion() in read_raw(), complete() in the irq
handler and do the actual read of the data back in read_raw().
It's probably related to the other interrupt sources that we need to
differentiate from in the interrupt handler.

The lock definition is missing an documentation of exactly what it's scope
is which definitely doesn't help us understand this unusual structure.

Thanks,

Jonathan

> 
> I am not quite sure whether this possible problem is real and how to fix 
> it if it is real.
> Any feedback would be appreciated, thanks :)
> 
> 
> Best wishes,
> Jia-Ju Bai