linux-kernel - Re: [PATCH] iio: light: al3010: Fix an error handling path in al3010

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: <20240914151532.1c9580e2@jic23-huawei>
Date: Sat, 14 Sep 2024 15:15:32 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Lars-Peter Clausen <lars@...afoo.de>, David Heidelberg <david@...t.cz>,
 Dmitry Osipenko <digetx@...il.com>, linux-kernel@...r.kernel.org,
 kernel-janitors@...r.kernel.org, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org
Subject: Re: [PATCH] iio: light: al3010: Fix an error handling path in
 al3010_probe()

On Tue, 10 Sep 2024 20:36:06 +0200
Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:

> If i2c_smbus_write_byte_data() fails in al3010_init(),
> al3010_set_pwr(false) is not called.
> 
> In order to avoid such a situation, move the devm_add_action_or_reset()
> witch calls al3010_set_pwr(false) right after a successful
> al3010_set_pwr(true).
> 
> Fixes: c36b5195ab70 ("iio: light: add Dyna-Image AL3010 driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Looks correct to me, but given the upshot of the bug is that in
a case where an bus access fails we don't power down (which requires a bus
access). It's unlikely to happen in practice and outcome is device
remains powered up when it shouldn't be which isn't too bad an outcome.

Hence I'll queue this up the slow way.
Applied to the testing branch of iio.git for 0-day to play with it before
I rebase on rc1 once available.

Thanks,

Jonathan

> ---
> Compile tested only.
> This patch is speculative, review with care
> ---
>  drivers/iio/light/al3010.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
> index 53569587ccb7..7cbb8b203300 100644
> --- a/drivers/iio/light/al3010.c
> +++ b/drivers/iio/light/al3010.c
> @@ -87,7 +87,12 @@ static int al3010_init(struct al3010_data *data)
>  	int ret;
>  
>  	ret = al3010_set_pwr(data->client, true);
> +	if (ret < 0)
> +		return ret;
>  
> +	ret = devm_add_action_or_reset(&data->client->dev,
> +				       al3010_set_pwr_off,
> +				       data);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -190,12 +195,6 @@ static int al3010_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> -	ret = devm_add_action_or_reset(&client->dev,
> -					al3010_set_pwr_off,
> -					data);
> -	if (ret < 0)
> -		return ret;
> -
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>