Re: [PATCH 02/11] iio: adc: ti-ads1119: fix information leak in triggered buffer
From: Javier Carrasco
Date: Tue Nov 26 2024 - 05:01:26 EST
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