linux-kernel - Re: [PATCH v2 2/2] iio: light: as73211: fix channel handling in only-color triggered buffer

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]

Message-ID: <3614353.dWV9SEqChM@n9w6sw14>
Date: Wed, 4 Dec 2024 17:20:47 +0100
From: Christian Eggers <ceggers@...i.de>
To: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
	Antoni Pokusinski <apokusinski01@...il.com>, Francesco Dolcini
	<francesco@...cini.it>, João Paulo Gonçalves
	<jpaulo.silvagoncalves@...il.com>, Javier Carrasco
	<javier.carrasco.cruz@...il.com>
CC: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	<linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	João Paulo Gonçalves <joao.goncalves@...adex.com>,
	Francesco Dolcini <francesco.dolcini@...adex.com>, Javier Carrasco
	<javier.carrasco.cruz@...il.com>, <stable@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: light: as73211: fix channel handling in only-color
 triggered buffer

On Wednesday, 4 December 2024, 00:55:32 CET, Javier Carrasco wrote:
> The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not
> set (optimized path for color channel readings), and it must be shifted
> instead of leaving an empty channel for the temperature when it is off.
> 
> Once the channel index is fixed, the uninitialized channel must be set
> to zero to avoid pushing uninitialized data.
> 
> Cc: stable@...r.kernel.org
> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
> ---
>  drivers/iio/light/as73211.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> index be0068081ebb..2d45dfeda406 100644
> --- a/drivers/iio/light/as73211.c
> +++ b/drivers/iio/light/as73211.c
> @@ -672,9 +672,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>  
>  		/* AS73211 starts reading at address 2 */
>  		ret = i2c_master_recv(data->client,
> -				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
> +				(char *)&scan.chan[0], 3 * sizeof(scan.chan[0]));
>  		if (ret < 0)
>  			goto done;
> +
> +		/* Avoid pushing uninitialized data */
> +		scan.chan[3] = 0;
>  	}
>  
>  	if (data_result) {
> @@ -682,9 +685,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>  		 * Saturate all channels (in case of overflows). Temperature channel
>  		 * is not affected by overflows.
>  		 */
> -		scan.chan[1] = cpu_to_le16(U16_MAX);
> -		scan.chan[2] = cpu_to_le16(U16_MAX);
> -		scan.chan[3] = cpu_to_le16(U16_MAX);
> +		if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {
> +			scan.chan[1] = cpu_to_le16(U16_MAX);
> +			scan.chan[2] = cpu_to_le16(U16_MAX);
> +			scan.chan[3] = cpu_to_le16(U16_MAX);
> +		} else {
> +			scan.chan[0] = cpu_to_le16(U16_MAX);
> +			scan.chan[1] = cpu_to_le16(U16_MAX);
> +			scan.chan[2] = cpu_to_le16(U16_MAX);
> +		}
>  	}
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
> 
> 

With this change, having only X, Y and Z in the scan_mask (without the
temperature channel) works fine.

But it looks that there is still another problem if a single color channel
(e.g. X) is omitted from the scan mask (which probably wouldn't make much
sense in practice).  If I am right, the layout of scan.chan[] is also wrong for
that case, so e.g. if omitting X, the application will get the X values where
it expects the temperature value (which isn't read from the hardware at all).

Does it make sense to write a follow-up patch for this? I fear that taking all
possible combinations into account could make the driver more complicated than
necessary.  Or is there a good example how to handle such combinations?


Tested-by: Christian Eggers <ceggers@...i.de>