Re: [PATCH 02/11] iio: adc: ti-ads1119: fix information leak in triggered buffer

[Javier Carrasco]

On 26/11/2024 09:59, Francesco Dolcini wrote:
> On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote:
>> The 'scan' local struct is used to push data to user space from a
>> triggered buffer, but it has a hole between the sample (unsigned int)
>> and the timestamp. This hole is never initialized.
>>
>> Initialize the struct to zero before using it to avoid pushing
>> uninitialized information to userspace.
>>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver")
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
>> ---
>>  drivers/iio/adc/ti-ads1119.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
>> index e9d9d4d46d38..2615a275acb3 100644
>> --- a/drivers/iio/adc/ti-ads1119.c
>> +++ b/drivers/iio/adc/ti-ads1119.c
>> @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private)
>>  	unsigned int index;
>>  	int ret;
>>  
>> +	memset(&scan, 0, sizeof(scan));
> 
> Did you consider adding a reserved field after sample and just
> initializing that one to zero?
> 
> It seems a trivial optimization not adding much value, but I thought about
> it, so I'd like to be sure you considered it.
> 
> In any case, the change is fine.
> 
> Reviewed-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> 
> Thanks,
> Francesco
> 

Hi Francesco, thanks for your review.

In this particular case where unsigned int is used for the sample, the
padding would _in theory_ depend on the architecture. The size of the
unsigned int is usually 4 bytes, but the standard only specifies that it
must be able to contain values in the [0, 65535] range i.e. 2 bytes.
That is indeed theory, and I don't know if there is a real case where a
new version of Linux is able to run on an architecture that uses 2 bytes
for an int. I guess there is not, but better safe than sorry.

We could be more specific with u32 for the sample and then add the
reserved field, but I would still prefer a memset() for this small
struct. Adding and initializing a reserved field looks a bit artificial
to me, especially for such marginal gains.

Moreover, the common practice (at least in IIO)is a plain memset() to
initialize struct holes, and such common patterns are easier to maintain :)

Best regards,
Javier Carrasco