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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <98feceae-2146-478b-8296-d3a41401dbf9@gmail.com>
Date: Wed, 27 Nov 2024 01:30:36 +0100
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Francesco Dolcini <francesco@...cini.it>,
 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 23:00, Javier Carrasco wrote:
> On Tue Nov 26, 2024 at 7:52 PM CET, Jonathan Cameron wrote:
>> On Tue, 26 Nov 2024 10:46:37 +0100
>> Javier Carrasco <javier.carrasco.cruz@...il.com> wrote:
>>
>>> 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.
>> Using an unsigned int here is a bug as well as we should present consistent
>> formatted data whatever the architecture.
> 
> Would you prefer that in the same patch as they are related issues? I
> could switch to u32 in v2 along with anything else that might arise in
> the reviews of the rest of the series.
> If you prefer a separate patch, that's fine too.
> 

Although now that I am looking into it, and according to the datasheet
and defined scan_type, the right size should be s16.

>>>
>>> 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.
>> Issue with reserved fields is we would have to be very very careful to spot them
>> all.  A memset avoids that care being needed.
>>
>> Jonathan
>>
>>>
>>> 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
>