The 'scan' local struct is used to push data to userspace from a
triggered buffer, but it leaves the first channel uninitialized if
AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel
readings.

Set the temperature channel to zero if only color channels are
relevant to avoid pushing uninitialized information to userspace.

Cc: stable@vger.kernel.org
Fixes: 403e5586b52e ("iio: light: as73211: New driver")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/as73211.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
index be0068081ebb..99679b686146 100644
--- a/drivers/iio/light/as73211.c
+++ b/drivers/iio/light/as73211.c
@@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
 				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
 		if (ret < 0)
 			goto done;
+
+		/* Avoid leaking uninitialized data */
+		scan.chan[0] = 0;
 	}
 
 	if (data_result) {

-- 
2.43.0

On Mon, 25 Nov 2024 22:16:18 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The 'scan' local struct is used to push data to userspace from a
> triggered buffer, but it leaves the first channel uninitialized if
> AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel
> readings.
> 
> Set the temperature channel to zero if only color channels are
> relevant to avoid pushing uninitialized information to userspace.
> 
> Cc: stable@vger.kernel.org
> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Huh.

If the temperature channel is turned off the data should shift. So should be read
into scan.chan[0] and [1] and [2], but not [3].

Not skipping [0] as here.

So this code path currently doesn't work as far as I can tell.

Jonathan

> ---
>  drivers/iio/light/as73211.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> index be0068081ebb..99679b686146 100644
> --- a/drivers/iio/light/as73211.c
> +++ b/drivers/iio/light/as73211.c
> @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>  				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
>  		if (ret < 0)
>  			goto done;
> +
> +		/* Avoid leaking uninitialized data */
> +		scan.chan[0] = 0;
>  	}
>  
>  	if (data_result) {
>

On 30/11/2024 21:49, Jonathan Cameron wrote:
> On Mon, 25 Nov 2024 22:16:18 +0100
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> 
>> The 'scan' local struct is used to push data to userspace from a
>> triggered buffer, but it leaves the first channel uninitialized if
>> AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel
>> readings.
>>
>> Set the temperature channel to zero if only color channels are
>> relevant to avoid pushing uninitialized information to userspace.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Huh.
> 
> If the temperature channel is turned off the data should shift. So should be read
> into scan.chan[0] and [1] and [2], but not [3].
> 
> Not skipping [0] as here.
> 
> So this code path currently doesn't work as far as I can tell.
> 
> Jonathan
> 
>> ---
>>  drivers/iio/light/as73211.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
>> index be0068081ebb..99679b686146 100644
>> --- a/drivers/iio/light/as73211.c
>> +++ b/drivers/iio/light/as73211.c
>> @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>>  				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
>>  		if (ret < 0)
>>  			goto done;
>> +
>> +		/* Avoid leaking uninitialized data */
>> +		scan.chan[0] = 0;
>>  	}
>>  
>>  	if (data_result) {
>>
> 

Adding the driver maintainer (should have been added from the beginning)
to the conversation.

@Christian, could you please confirm this?

Apparently, the optimization to read the color channels without
temperature is not right. I don't have access to the AS7331 at the
moment, but I remember that you could test my patches on your hardware
with an AS73211, so maybe you can confirm whether wrong data is
delivered or not in that case.

Thanks and best regards,
Javier Carrasco

Hi Jonathan, hi Javier,

On Monday, 2 December 2024, 16:38:50 CET, Javier Carrasco wrote:
> On 30/11/2024 21:49, Jonathan Cameron wrote:
> > On Mon, 25 Nov 2024 22:16:18 +0100
> > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> > 
> >> The 'scan' local struct is used to push data to userspace from a
> >> triggered buffer, but it leaves the first channel uninitialized if
> >> AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel
> >> readings.
> >>
> >> Set the temperature channel to zero if only color channels are
> >> relevant to avoid pushing uninitialized information to userspace.
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > Huh.
> > 
> > If the temperature channel is turned off the data should shift. So should be read
> > into scan.chan[0] and [1] and [2], but not [3].
> > 
> > Not skipping [0] as here.
> > 
> > So this code path currently doesn't work as far as I can tell.

I've just tested and you are right! In our application we never had the case that
we didn't read the temperature channel. If I don't enable scan_elements/in_temp_en,
I need to put the data into scan.chan[0..2] in order to get correct values in my
application. This also means that the "Optimization for reading only color channel"
(and the following saturation block) isn't correct at all, especially if reading only
one or two of the available channels.

> > 
> > Jonathan
> > 
> >> ---
> >>  drivers/iio/light/as73211.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
> >> index be0068081ebb..99679b686146 100644
> >> --- a/drivers/iio/light/as73211.c
> >> +++ b/drivers/iio/light/as73211.c
> >> @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
> >>  				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
> >>  		if (ret < 0)
> >>  			goto done;
> >> +
> >> +		/* Avoid leaking uninitialized data */
> >> +		scan.chan[0] = 0;
> >>  	}
> >>  
> >>  	if (data_result) {
> >>
> > 
> 
> Adding the driver maintainer (should have been added from the beginning)
> to the conversation.
> 
> @Christian, could you please confirm this?
> 
> Apparently, the optimization to read the color channels without
> temperature is not right. I don't have access to the AS7331 at the
> moment, but I remember that you could test my patches on your hardware
> with an AS73211, so maybe you can confirm whether wrong data is
> delivered or not in that case.

Yes, the delivered data is wrong (as already stated above).

@Javier: If you like to rework this, I can test your patches (I have still
access to the hardware).  Otherwise I can also try to fix this on my own.

> 
> Thanks and best regards,
> Javier Carrasco

Thanks for reporting this!
Christian
> 
>

On 02/12/2024 19:00, Christian Eggers wrote:
> Hi Jonathan, hi Javier,
> 
> On Monday, 2 December 2024, 16:38:50 CET, Javier Carrasco wrote:
>> On 30/11/2024 21:49, Jonathan Cameron wrote:
>>> On Mon, 25 Nov 2024 22:16:18 +0100
>>> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>>>
>>>> The 'scan' local struct is used to push data to userspace from a
>>>> triggered buffer, but it leaves the first channel uninitialized if
>>>> AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel
>>>> readings.
>>>>
>>>> Set the temperature channel to zero if only color channels are
>>>> relevant to avoid pushing uninitialized information to userspace.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: 403e5586b52e ("iio: light: as73211: New driver")
>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>> Huh.
>>>
>>> If the temperature channel is turned off the data should shift. So should be read
>>> into scan.chan[0] and [1] and [2], but not [3].
>>>
>>> Not skipping [0] as here.
>>>
>>> So this code path currently doesn't work as far as I can tell.
> 
> I've just tested and you are right! In our application we never had the case that
> we didn't read the temperature channel. If I don't enable scan_elements/in_temp_en,
> I need to put the data into scan.chan[0..2] in order to get correct values in my
> application. This also means that the "Optimization for reading only color channel"
> (and the following saturation block) isn't correct at all, especially if reading only
> one or two of the available channels.
> 
>>>
>>> Jonathan
>>>
>>>> ---
>>>>  drivers/iio/light/as73211.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
>>>> index be0068081ebb..99679b686146 100644
>>>> --- a/drivers/iio/light/as73211.c
>>>> +++ b/drivers/iio/light/as73211.c
>>>> @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
>>>>  				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
>>>>  		if (ret < 0)
>>>>  			goto done;
>>>> +
>>>> +		/* Avoid leaking uninitialized data */
>>>> +		scan.chan[0] = 0;
>>>>  	}
>>>>  
>>>>  	if (data_result) {
>>>>
>>>
>>
>> Adding the driver maintainer (should have been added from the beginning)
>> to the conversation.
>>
>> @Christian, could you please confirm this?
>>
>> Apparently, the optimization to read the color channels without
>> temperature is not right. I don't have access to the AS7331 at the
>> moment, but I remember that you could test my patches on your hardware
>> with an AS73211, so maybe you can confirm whether wrong data is
>> delivered or not in that case.
> 
> Yes, the delivered data is wrong (as already stated above).
> 
> @Javier: If you like to rework this, I can test your patches (I have still
> access to the hardware).  Otherwise I can also try to fix this on my own.
> 
>>
>> Thanks and best regards,
>> Javier Carrasco
> 
> Thanks for reporting this!
> Christian
>>
>>
>

Thanks for your prompt reply. I will rework it for v2, as the current
patch does not apply. For this path, scan.chan[0]..scan.chan[2] will be
read from the sensor, and scan.chan[3] will be set to zero.

Best regards,
Javier Carrasco