Re: [PATCH 10/11] iio: light: as73211: fix information leak in triggered buffer

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

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