linux-kernel - Re: [PATCH 02/11] iio: adc: ti-ads1119: fix information leak in 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: <59a4b096-101b-419d-8a19-1063d759b4e2@gmail.com>
Date: Tue, 26 Nov 2024 10:46:37 +0100
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Francesco Dolcini <francesco@...cini.it>
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Antoni Pokusinski <apokusinski01@...il.com>,
 João Paulo Gonçalves
 <jpaulo.silvagoncalves@...il.com>, Gregor Boirie <gregor.boirie@...rot.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 João Paulo Gonçalves <joao.goncalves@...adex.com>,
 Francesco Dolcini <francesco.dolcini@...adex.com>, stable@...r.kernel.org
Subject: Re: [PATCH 02/11] iio: adc: ti-ads1119: fix information leak in
 triggered buffer

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@...r.kernel.org
>> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver")
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
>> ---
>>  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@...adex.com>
> 
> 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