linux-kernel - Re: [PATCH] iio: light: opt3001: fix deadlock due to concurrent flag access

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250330182110.5373d1a0@jic23-huawei>
Date: Sun, 30 Mar 2025 18:21:10 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Felipe Balbi <balbi@...com>,
 Andreas Dannenberg <dannenberg@...com>, David Frey <dpfrey@...il.com>, Emil
 Gedenryd <emil.gedenryd@...s.com>, Alexander Koch <mail@...xanderkoch.net>,
 Jiri Valek - 2N <valek@...cz>, Hui Pu <Hui.Pu@...ealthcare.com>, Thomas
 Petazzoni <thomas.petazzoni@...tlin.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] iio: light: opt3001: fix deadlock due to concurrent
 flag access

On Fri, 21 Mar 2025 19:10:00 +0100
Luca Ceresoli <luca.ceresoli@...tlin.com> wrote:

> The threaded IRQ function in this driver is reading the flag twice: once to
> lock a mutex and once to unlock it. Even though the code setting the flag
> is designed to prevent it, there are subtle cases where the flag could be
> true at the mutex_lock stage and false at the mutex_unlock stage. This
> results in the mutex not being unlocked, resulting in a deadlock.

From a quick look I think that only occurs if the triggering write
reports an error but the device accepts it anyway (maybe a bus
error on the ack going back to the host).

I'm not a fan of this locking dance, but I'm also not keen on
touching it too much without test hardware. Let's go with your fix which
looks safe to me.

Applied.
> 
> Fix it by making the opt3001_irq() code generally more robust, reading the
> flag into a variable and using the variable value at both stages.
> 
> Fixes: 94a9b7b1809f ("iio: light: add support for TI's opt3001 light sensor")
> Cc: stable@...r.kernel.org
> Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
> ---
>  drivers/iio/light/opt3001.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 65b295877b41588d40234ca7681bfee291e937c2..393a3d2fbe1d7320a243d3b6720e98b90f17baca 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -788,8 +788,9 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
>  	int ret;
>  	bool wake_result_ready_queue = false;
>  	enum iio_chan_type chan_type = opt->chip_info->chan_type;
> +	bool ok_to_ignore_lock = opt->ok_to_ignore_lock;
>  
> -	if (!opt->ok_to_ignore_lock)
> +	if (!ok_to_ignore_lock)
>  		mutex_lock(&opt->lock);
>  
>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> @@ -826,7 +827,7 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
>  	}
>  
>  out:
> -	if (!opt->ok_to_ignore_lock)
> +	if (!ok_to_ignore_lock)
>  		mutex_unlock(&opt->lock);
>  
>  	if (wake_result_ready_queue)
> 
> ---
> base-commit: 250a4b882cf37d9719874253f055aad211f2c317
> change-id: 20250321-opt3001-irq-fix-f7eecd4e2e9c
> 
> Best regards,