linux-kernel - Re: [PATCH v4] 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] [day] [month] [year] [list]
Message-ID: <20241215141226.31c81048@jic23-huawei>
Date: Sun, 15 Dec 2024 14:12:26 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Christian Eggers
 <ceggers@...i.de>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH v4] iio: light: as73211: fix channel handling in
 only-color triggered buffer

On Sat, 14 Dec 2024 23:55:50 +0100
Javier Carrasco <javier.carrasco.cruz@...il.com> 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.
> 
> Add available_scan_masks for all channels and only-color channels to let
> the IIO core demux and repack the enabled channels.
> 
> Cc: stable@...r.kernel.org
> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> Tested-by: Christian Eggers <ceggers@...i.de>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> ---
> This issue was found after attempting to make the same mistake for
> a driver I maintain, which was fortunately spotted by Jonathan [1].
> 
> Keeping old sensor values if the channel configuration changes is known
> and not considered an issue, which is also mentioned in [1], so it has
> not been addressed by this series. That keeps most of the drivers out
> of the way because they store the scan element in iio private data,
> which is kzalloc() allocated.
> 
> This series only addresses cases where uninitialized i.e. unknown data
> is pushed to the userspace, either due to holes in structs or
> uninitialized struct members/array elements.
> 
> While analyzing involved functions, I found and fixed some triviality
> (wrong function name) in the documentation of iio_dev_opaque.
> 
> Link: https://lore.kernel.org/linux-iio/20241123151634.303aa860@jic23-huawei/ [1]
> ---
> Changes in v4:
> - Fix as73211_scan_masks[] (first MASK_COLOR, then MASK_ALL, no comma
>   after 0 i.e. the last element).
> - Link to v3: https://lore.kernel.org/r/20241212-iio_memset_scan_holes-v3-1-7f496b6f7222@gmail.com
> 
> Changes in v3:
> - as73211.c: add available_scan_masks for all channels and only-color
>   channels to let the IIO core demux and repack the enabled channels.
> - Link to v2: https://lore.kernel.org/r/20241204-iio_memset_scan_holes-v2-0-3f941592a76d@gmail.com
> 
> Changes in v2:
> - as73211.c: shift channels if no temperature is available and
>   initialize chan[3] to zero.
> - Link to v1: https://lore.kernel.org/r/20241125-iio_memset_scan_holes-v1-0-0cb6e98d895c@gmail.com
> ---
>  drivers/iio/light/as73211.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> index be0068081ebb..11fbdcdd26d6 100644
> --- a/drivers/iio/light/as73211.c
> +++ b/drivers/iio/light/as73211.c
> @@ -177,6 +177,12 @@ struct as73211_data {
>  	BIT(AS73211_SCAN_INDEX_TEMP) | \
>  	AS73211_SCAN_MASK_COLOR)
>  
> +static const unsigned long as73211_scan_masks[] = {
> +	AS73211_SCAN_MASK_COLOR,
> +	AS73211_SCAN_MASK_ALL,
> +	0
> +};
> +
>  static const struct iio_chan_spec as73211_channels[] = {
>  	{
>  		.type = IIO_TEMP,
> @@ -672,9 +678,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 +691,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));
> @@ -758,6 +773,7 @@ static int as73211_probe(struct i2c_client *client)
>  	indio_dev->channels = data->spec_dev->channels;
>  	indio_dev->num_channels = data->spec_dev->num_channels;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->available_scan_masks = as73211_scan_masks;
>  
>  	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR);
>  	if (ret < 0)
> 
> ---
> base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
> change-id: 20241123-iio_memset_scan_holes-a673833ef932
> 
> Best regards,