[PATCH 00/11] iio: fix information leaks in triggered buffers

[PATCH 00/11] iio: fix information leaks in triggered buffers

[Javier Carrasco]

This issue was found after attempting to make the same mistake for
a driver I maintain, which was fortunately spotted by Jonathan [1].

Keeping old sensor values if the channel configuration changes is known
and not considered an issue, which is also mentioned in [1], so it has
not been addressed by this series. That keeps most of the drivers out
of the way because they store the scan element in iio private data,
which is kzalloc() allocated.

This series only addresses cases where uninitialized i.e. unknown data
is pushed to the userspace, either due to holes in structs or
uninitialized struct members/array elements.

While analyzing involved functions, I found and fixed some triviality
(wrong function name) in the documentation of iio_dev_opaque.

Link: https://lore.kernel.org/linux-iio/20241123151634.303aa860@jic23-huawei/ [1]

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (11):
      iio: temperature: tmp006: fix information leak in triggered buffer
      iio: adc: ti-ads1119: fix information leak in triggered buffer
      iio: pressure: zpa2326: fix information leak in triggered buffer
      iio: adc: rockchip_saradc: fix information leak in triggered buffer
      iio: imu: kmx61: fix information leak in triggered buffer
      iio: light: vcnl4035: fix information leak in triggered buffer
      iio: light: bh1745: fix information leak in triggered buffer
      iio: adc: ti-ads8688: fix information leak in triggered buffer
      iio: dummy: iio_simply_dummy_buffer: fix information leak in triggered buffer
      iio: light: as73211: fix information leak in triggered buffer
      iio: core: fix doc reference to iio_push_to_buffers_with_ts_unaligned

 drivers/iio/adc/rockchip_saradc.c           | 2 ++
 drivers/iio/adc/ti-ads1119.c                | 2 ++
 drivers/iio/adc/ti-ads8688.c                | 2 +-
 drivers/iio/dummy/iio_simple_dummy_buffer.c | 2 +-
 drivers/iio/imu/kmx61.c                     | 2 +-
 drivers/iio/light/as73211.c                 | 3 +++
 drivers/iio/light/bh1745.c                  | 2 ++
 drivers/iio/light/vcnl4035.c                | 2 +-
 drivers/iio/pressure/zpa2326.c              | 2 ++
 drivers/iio/temperature/tmp006.c            | 2 ++
 include/linux/iio/iio-opaque.h              | 2 +-
 11 files changed, 18 insertions(+), 5 deletions(-)
---
base-commit: ab376e4d674037f45d5758c1dc391bd4e11c5dc4
change-id: 20241123-iio_memset_scan_holes-a673833ef932

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 01/11] iio: temperature: tmp006: fix information leak in triggered buffer

[Javier Carrasco]

The 'scan' local struct is used to push data to user space from a
triggered buffer, but it has a hole between the two 16-bit data channels
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@vger.kernel.org
Fixes: 91f75ccf9f03 ("iio: temperature: tmp006: add triggered buffer support")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/temperature/tmp006.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
index 0c844137d7aa..02b27f471baa 100644
--- a/drivers/iio/temperature/tmp006.c
+++ b/drivers/iio/temperature/tmp006.c
@@ -252,6 +252,8 @@ static irqreturn_t tmp006_trigger_handler(int irq, void *p)
 	} scan;
 	s32 ret;
 
+	memset(&scan, 0, sizeof(scan));
+
 	ret = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT);
 	if (ret < 0)
 		goto err;

-- 
2.43.0

Re: [PATCH 01/11] iio: temperature: tmp006: fix information leak in triggered buffer

[Javier Carrasco]

On 25/11/2024 22:16, 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 two 16-bit data channels
> 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@vger.kernel.org
> Fixes: 91f75ccf9f03 ("iio: temperature: tmp006: add triggered buffer support")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/iio/temperature/tmp006.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
> index 0c844137d7aa..02b27f471baa 100644
> --- a/drivers/iio/temperature/tmp006.c
> +++ b/drivers/iio/temperature/tmp006.c
> @@ -252,6 +252,8 @@ static irqreturn_t tmp006_trigger_handler(int irq, void *p)
>  	} scan;
>  	s32 ret;
>  
> +	memset(&scan, 0, sizeof(scan));
> +
>  	ret = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT);
>  	if (ret < 0)
>  		goto err;
> 

@Jonathan, this patch requires 91f75ccf9f03 ("iio: temperature: tmp006:
add triggered buffer support"), which is in the mainline kernel, but not
accessible from iio/fixes-to-greg.

Is there any branch in IIO where the fixes and the new features are put
together? I would like to rebase my series to automatically get rid of
the applied patches, but iio/fixes-to-greg (where the patches were
applied) does not have the feature this patch fixes. Of course I can
manually drop the applied patches, but that is error-prone.

This is not the first time I face this inconvenience, and I suppose
there is a cleaner way that I might be missing, or maybe that branch I
am looking for already exists.

Thanks and best regards,
Javier Carrasco

Re: [PATCH 01/11] iio: temperature: tmp006: fix information leak in triggered buffer

[Jonathan Cameron]

On Mon, 2 Dec 2024 20:28:12 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> On 25/11/2024 22:16, 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 two 16-bit data channels
> > 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@vger.kernel.org
> > Fixes: 91f75ccf9f03 ("iio: temperature: tmp006: add triggered buffer support")
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > ---
> >  drivers/iio/temperature/tmp006.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
> > index 0c844137d7aa..02b27f471baa 100644
> > --- a/drivers/iio/temperature/tmp006.c
> > +++ b/drivers/iio/temperature/tmp006.c
> > @@ -252,6 +252,8 @@ static irqreturn_t tmp006_trigger_handler(int irq, void *p)
> >  	} scan;
> >  	s32 ret;
> >  
> > +	memset(&scan, 0, sizeof(scan));
> > +
> >  	ret = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT);
> >  	if (ret < 0)
> >  		goto err;
> >   
> 
> @Jonathan, this patch requires 91f75ccf9f03 ("iio: temperature: tmp006:
> add triggered buffer support"), which is in the mainline kernel, but not
> accessible from iio/fixes-to-greg.
> 

Yeah. That happens briefly around merge windows.  In this particular
case to just after rc1 as there were some tree wide refactors that
needed merging.  Sometimes it takes me a few days to find the time to
rebase.  Doing anything mid merge window is a challenge at best.

> Is there any branch in IIO where the fixes and the new features are put
> together? I would like to rebase my series to automatically get rid of
> the applied patches, but iio/fixes-to-greg (where the patches were
> applied) does not have the feature this patch fixes. Of course I can
> manually drop the applied patches, but that is error-prone.
No. I don't push out such a tree, though I often do test merges.

You could use linux-next for your automation as that normally contains
both the fixes-togreg and togreg branches. Mind you that doesn't right
now because of the merge issue mentioned above,

Jonathan

> 
> This is not the first time I face this inconvenience, and I suppose
> there is a cleaner way that I might be missing, or maybe that branch I
> am looking for already exists.
> 
> Thanks and best regards,
> Javier Carrasco

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

[Javier Carrasco]

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@vger.kernel.org
Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.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));
+
 	if (!iio_trigger_using_own(indio_dev)) {
 		index = find_first_bit(indio_dev->active_scan_mask,
 				       iio_get_masklength(indio_dev));

-- 
2.43.0

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

[Francesco Dolcini]

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

Thanks,
Francesco

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

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

[Jonathan Cameron]

On Tue, 26 Nov 2024 10:46:37 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.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@vger.kernel.org
> >> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver")
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.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@toradex.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.
> 
> 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

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

[Javier Carrasco]

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

> >
> > 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

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

[Javier Carrasco]

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

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

[Jonathan Cameron]

On Wed, 27 Nov 2024 01:30:36 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> 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@gmail.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@vger.kernel.org
> >>>>> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver")
> >>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.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@toradex.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.
> 
Separate patch would be great!

Thanks

Jonathan

> >>>
> >>> 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  
> >   
> 

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

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:10 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> 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@vger.kernel.org
> Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied.
> ---
>  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));
> +
>  	if (!iio_trigger_using_own(indio_dev)) {
>  		index = find_first_bit(indio_dev->active_scan_mask,
>  				       iio_get_masklength(indio_dev));
> 

[PATCH 03/11] iio: pressure: zpa2326: fix information leak in triggered buffer

[Javier Carrasco]

The 'sample' local struct is used to push data to user space from a
triggered buffer, but it has a hole between the temperature and the
timestamp (u32 pressure, u16 temperature, GAP, u64 timestamp).
This hole is never initialized.

Initialize the struct to zero before using it to avoid pushing
uninitialized information to userspace.

Cc: stable@vger.kernel.org
Fixes: 03b262f2bbf4 ("iio:pressure: initial zpa2326 barometer support")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/pressure/zpa2326.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/pressure/zpa2326.c b/drivers/iio/pressure/zpa2326.c
index 950f8dee2b26..b4c6c7c47256 100644
--- a/drivers/iio/pressure/zpa2326.c
+++ b/drivers/iio/pressure/zpa2326.c
@@ -586,6 +586,8 @@ static int zpa2326_fill_sample_buffer(struct iio_dev               *indio_dev,
 	}   sample;
 	int err;
 
+	memset(&sample, 0, sizeof(sample));
+
 	if (test_bit(0, indio_dev->active_scan_mask)) {
 		/* Get current pressure from hardware FIFO. */
 		err = zpa2326_dequeue_pressure(indio_dev, &sample.pressure);

-- 
2.43.0

Re: [PATCH 03/11] iio: pressure: zpa2326: fix information leak in triggered buffer

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:11 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The 'sample' local struct is used to push data to user space from a
> triggered buffer, but it has a hole between the temperature and the
> timestamp (u32 pressure, u16 temperature, GAP, u64 timestamp).
> This hole is never initialized.
> 
> Initialize the struct to zero before using it to avoid pushing
> uninitialized information to userspace.
> 
> Cc: stable@vger.kernel.org
> Fixes: 03b262f2bbf4 ("iio:pressure: initial zpa2326 barometer support")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied.
> ---
>  drivers/iio/pressure/zpa2326.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/pressure/zpa2326.c b/drivers/iio/pressure/zpa2326.c
> index 950f8dee2b26..b4c6c7c47256 100644
> --- a/drivers/iio/pressure/zpa2326.c
> +++ b/drivers/iio/pressure/zpa2326.c
> @@ -586,6 +586,8 @@ static int zpa2326_fill_sample_buffer(struct iio_dev               *indio_dev,
>  	}   sample;
>  	int err;
>  
> +	memset(&sample, 0, sizeof(sample));
> +
>  	if (test_bit(0, indio_dev->active_scan_mask)) {
>  		/* Get current pressure from hardware FIFO. */
>  		err = zpa2326_dequeue_pressure(indio_dev, &sample.pressure);
> 

[PATCH 04/11] iio: adc: rockchip_saradc: fix information leak in triggered buffer

[Javier Carrasco]

The 'data' local struct is used to push data to user space from a
triggered buffer, but it does not set values for inactive channels, as
it only uses iio_for_each_active_channel() to assign new values.

Initialize the struct to zero before using it to avoid pushing
uninitialized information to userspace.

Cc: stable@vger.kernel.org
Fixes: 4e130dc7b413 ("iio: adc: rockchip_saradc: Add support iio buffers")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/adc/rockchip_saradc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index 240cfa391674..dfd47a6e1f4a 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -368,6 +368,8 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
 	int ret;
 	int i, j = 0;
 
+	memset(&data, 0, sizeof(data));
+
 	mutex_lock(&info->lock);
 
 	iio_for_each_active_channel(i_dev, i) {

-- 
2.43.0

Re: [PATCH 04/11] iio: adc: rockchip_saradc: fix information leak in triggered buffer

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:12 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The 'data' local struct is used to push data to user space from a
> triggered buffer, but it does not set values for inactive channels, as
> it only uses iio_for_each_active_channel() to assign new values.
> 
> Initialize the struct to zero before using it to avoid pushing
> uninitialized information to userspace.
> 
> Cc: stable@vger.kernel.org
> Fixes: 4e130dc7b413 ("iio: adc: rockchip_saradc: Add support iio buffers")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied.
> ---
>  drivers/iio/adc/rockchip_saradc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 240cfa391674..dfd47a6e1f4a 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -368,6 +368,8 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p)
>  	int ret;
>  	int i, j = 0;
>  
> +	memset(&data, 0, sizeof(data));
> +
>  	mutex_lock(&info->lock);
>  
>  	iio_for_each_active_channel(i_dev, i) {
> 

[PATCH 05/11] iio: imu: kmx61: fix information leak in triggered buffer

[Javier Carrasco]

The 'buffer' local array is used to push data to user space from a
triggered buffer, but it does not set values for inactive channels, as
it only uses iio_for_each_active_channel() to assign new values.

Initialize the array to zero before using it to avoid pushing
uninitialized information to userspace.

Cc: stable@vger.kernel.org
Fixes: c3a23ecc0901 ("iio: imu: kmx61: Add support for data ready triggers")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/imu/kmx61.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
index 324c38764656..e19c5d3137c6 100644
--- a/drivers/iio/imu/kmx61.c
+++ b/drivers/iio/imu/kmx61.c
@@ -1193,7 +1193,7 @@ static irqreturn_t kmx61_trigger_handler(int irq, void *p)
 	struct kmx61_data *data = kmx61_get_data(indio_dev);
 	int bit, ret, i = 0;
 	u8 base;
-	s16 buffer[8];
+	s16 buffer[8] = { };
 
 	if (indio_dev == data->acc_indio_dev)
 		base = KMX61_ACC_XOUT_L;

-- 
2.43.0

Re: [PATCH 05/11] iio: imu: kmx61: fix information leak in triggered buffer

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:13 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The 'buffer' local array is used to push data to user space from a
> triggered buffer, but it does not set values for inactive channels, as
> it only uses iio_for_each_active_channel() to assign new values.
> 
> Initialize the array to zero before using it to avoid pushing
> uninitialized information to userspace.
> 
> Cc: stable@vger.kernel.org
> Fixes: c3a23ecc0901 ("iio: imu: kmx61: Add support for data ready triggers")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied.
> ---
>  drivers/iio/imu/kmx61.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> index 324c38764656..e19c5d3137c6 100644
> --- a/drivers/iio/imu/kmx61.c
> +++ b/drivers/iio/imu/kmx61.c
> @@ -1193,7 +1193,7 @@ static irqreturn_t kmx61_trigger_handler(int irq, void *p)
>  	struct kmx61_data *data = kmx61_get_data(indio_dev);
>  	int bit, ret, i = 0;
>  	u8 base;
> -	s16 buffer[8];
> +	s16 buffer[8] = { };
>  
>  	if (indio_dev == data->acc_indio_dev)
>  		base = KMX61_ACC_XOUT_L;
> 

[PATCH 06/11] iio: light: vcnl4035: fix information leak in triggered buffer

[Javier Carrasco]

The 'buffer' local array is used to push data to userspace from a
triggered buffer, but it does not set an initial value for the single
data element, which is an u16 aligned to 8 bytes. That leaves at least
4 bytes uninitialized even after writing an integer value with
regmap_read().

Initialize the array to zero before using it to avoid pushing
uninitialized information to userspace.

Cc: stable@vger.kernel.org
Fixes: ec90b52c07c0 ("iio: light: vcnl4035: Fix buffer alignment in iio_push_to_buffers_with_timestamp()")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/vcnl4035.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c
index 337a1332c2c6..67c94be02018 100644
--- a/drivers/iio/light/vcnl4035.c
+++ b/drivers/iio/light/vcnl4035.c
@@ -105,7 +105,7 @@ static irqreturn_t vcnl4035_trigger_consumer_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct vcnl4035_data *data = iio_priv(indio_dev);
 	/* Ensure naturally aligned timestamp */
-	u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)]  __aligned(8);
+	u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)]  __aligned(8) = { };
 	int ret;
 
 	ret = regmap_read(data->regmap, VCNL4035_ALS_DATA, (int *)buffer);

-- 
2.43.0

Re: [PATCH 06/11] iio: light: vcnl4035: fix information leak in triggered buffer

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:14 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The 'buffer' local array is used to push data to userspace from a
> triggered buffer, but it does not set an initial value for the single
> data element, which is an u16 aligned to 8 bytes. That leaves at least
> 4 bytes uninitialized even after writing an integer value with
> regmap_read().
> 
> Initialize the array to zero before using it to avoid pushing
> uninitialized information to userspace.
> 
> Cc: stable@vger.kernel.org
> Fixes: ec90b52c07c0 ("iio: light: vcnl4035: Fix buffer alignment in iio_push_to_buffers_with_timestamp()")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied.
> ---
>  drivers/iio/light/vcnl4035.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c
> index 337a1332c2c6..67c94be02018 100644
> --- a/drivers/iio/light/vcnl4035.c
> +++ b/drivers/iio/light/vcnl4035.c
> @@ -105,7 +105,7 @@ static irqreturn_t vcnl4035_trigger_consumer_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct vcnl4035_data *data = iio_priv(indio_dev);
>  	/* Ensure naturally aligned timestamp */
> -	u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)]  __aligned(8);
> +	u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)]  __aligned(8) = { };
>  	int ret;
>  
>  	ret = regmap_read(data->regmap, VCNL4035_ALS_DATA, (int *)buffer);
> 

[PATCH 07/11] iio: light: bh1745: fix information leak in triggered buffer

[Javier Carrasco]

The 'scan' local struct is used to push data to user space from a
triggered buffer, but it does not set values for inactive channels, as
it only uses iio_for_each_active_channel() to assign new values.

Initialize the struct to zero before using it to avoid pushing
uninitialized information to userspace.

Cc: stable@vger.kernel.org
Fixes: eab35358aae7 ("iio: light: ROHM BH1745 colour sensor")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/bh1745.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c
index 23e9f16090cc..2ffc839c7501 100644
--- a/drivers/iio/light/bh1745.c
+++ b/drivers/iio/light/bh1745.c
@@ -746,6 +746,8 @@ static irqreturn_t bh1745_trigger_handler(int interrupt, void *p)
 	int i;
 	int j = 0;
 
+	memset(&scan, 0, sizeof(scan));
+
 	iio_for_each_active_channel(indio_dev, i) {
 		ret = regmap_bulk_read(data->regmap, BH1745_RED_LSB + 2 * i,
 				       &value, 2);

-- 
2.43.0

Re: [PATCH 07/11] iio: light: bh1745: fix information leak in triggered buffer

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:15 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The 'scan' local struct is used to push data to user space from a
> triggered buffer, but it does not set values for inactive channels, as
> it only uses iio_for_each_active_channel() to assign new values.
> 
> Initialize the struct to zero before using it to avoid pushing
> uninitialized information to userspace.
> 
> Cc: stable@vger.kernel.org
> Fixes: eab35358aae7 ("iio: light: ROHM BH1745 colour sensor")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied.
> ---
>  drivers/iio/light/bh1745.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c
> index 23e9f16090cc..2ffc839c7501 100644
> --- a/drivers/iio/light/bh1745.c
> +++ b/drivers/iio/light/bh1745.c
> @@ -746,6 +746,8 @@ static irqreturn_t bh1745_trigger_handler(int interrupt, void *p)
>  	int i;
>  	int j = 0;
>  
> +	memset(&scan, 0, sizeof(scan));
> +
>  	iio_for_each_active_channel(indio_dev, i) {
>  		ret = regmap_bulk_read(data->regmap, BH1745_RED_LSB + 2 * i,
>  				       &value, 2);
> 

[PATCH 08/11] iio: adc: ti-ads8688: fix information leak in triggered buffer

[Javier Carrasco]

The 'buffer' local array is used to push data to user space from a
triggered buffer, but it does not set values for inactive channels, as
it only uses iio_for_each_active_channel() to assign new values.

Initialize the array to zero before using it to avoid pushing
uninitialized information to userspace.

Cc: stable@vger.kernel.org
Fixes: 61fa5dfa5f52 ("iio: adc: ti-ads8688: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/adc/ti-ads8688.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c
index 9b1814f1965a..a31658b760a4 100644
--- a/drivers/iio/adc/ti-ads8688.c
+++ b/drivers/iio/adc/ti-ads8688.c
@@ -381,7 +381,7 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	/* Ensure naturally aligned timestamp */
-	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8);
+	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8) = { };
 	int i, j = 0;
 
 	iio_for_each_active_channel(indio_dev, i) {

-- 
2.43.0

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

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:16 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The 'buffer' local array is used to push data to user space from a
> triggered buffer, but it does not set values for inactive channels, as
> it only uses iio_for_each_active_channel() to assign new values.
> 
> Initialize the array to zero before using it to avoid pushing
> uninitialized information to userspace.
> 
> Cc: stable@vger.kernel.org
> Fixes: 61fa5dfa5f52 ("iio: adc: ti-ads8688: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied.
> ---
>  drivers/iio/adc/ti-ads8688.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c
> index 9b1814f1965a..a31658b760a4 100644
> --- a/drivers/iio/adc/ti-ads8688.c
> +++ b/drivers/iio/adc/ti-ads8688.c
> @@ -381,7 +381,7 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	/* Ensure naturally aligned timestamp */
> -	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8);
> +	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8) = { };
>  	int i, j = 0;
>  
>  	iio_for_each_active_channel(indio_dev, i) {
> 

[PATCH 09/11] iio: dummy: iio_simply_dummy_buffer: fix information leak in triggered buffer

[Javier Carrasco]

The 'data' array is allocated via kmalloc() and it is used to push data
to user space from a triggered buffer, but it does not set values for
inactive channels, as it only uses iio_for_each_active_channel()
to assign new values.

Use kzalloc for the memory allocation to avoid pushing uninitialized
information to userspace.

Cc: stable@vger.kernel.org
Fixes: 415f79244757 ("iio: Move IIO Dummy Driver out of staging")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/dummy/iio_simple_dummy_buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c
index 4ca3f1aaff99..288880346707 100644
--- a/drivers/iio/dummy/iio_simple_dummy_buffer.c
+++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c
@@ -48,7 +48,7 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p)
 	int i = 0, j;
 	u16 *data;
 
-	data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
+	data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
 	if (!data)
 		goto done;
 

-- 
2.43.0

Re: [PATCH 09/11] iio: dummy: iio_simply_dummy_buffer: fix information leak in triggered buffer

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:17 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The 'data' array is allocated via kmalloc() and it is used to push data
> to user space from a triggered buffer, but it does not set values for
> inactive channels, as it only uses iio_for_each_active_channel()
> to assign new values.
> 
> Use kzalloc for the memory allocation to avoid pushing uninitialized
> information to userspace.
> 
> Cc: stable@vger.kernel.org
> Fixes: 415f79244757 ("iio: Move IIO Dummy Driver out of staging")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Oops. Got it wrong even in the example driver.

Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan

> ---
>  drivers/iio/dummy/iio_simple_dummy_buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c
> index 4ca3f1aaff99..288880346707 100644
> --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c
> +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c
> @@ -48,7 +48,7 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p)
>  	int i = 0, j;
>  	u16 *data;
>  
> -	data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> +	data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
>  	if (!data)
>  		goto done;
>  
> 

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

[Javier Carrasco]

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>
---
 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) {

-- 
2.43.0

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

[Jonathan Cameron]

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.

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) {
> 

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

[Javier Carrasco]

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.
> 
> 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.

Thanks and best regards,
Javier Carrasco

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

[Christian Eggers]

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
> 
> 

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

[Javier Carrasco]

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

[PATCH 11/11] iio: core: fix doc reference to iio_push_to_buffers_with_ts_unaligned

[Javier Carrasco]

Use the right name of the function, which is defined in
drivers/iio/industrialio-buffer.c

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 include/linux/iio/iio-opaque.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index a89e7e43e441..4247497f3f8b 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -28,7 +28,7 @@
  * @groupcounter:		index of next attribute group
  * @legacy_scan_el_group:	attribute group for legacy scan elements attribute group
  * @legacy_buffer_group:	attribute group for legacy buffer attributes group
- * @bounce_buffer:		for devices that call iio_push_to_buffers_with_timestamp_unaligned()
+ * @bounce_buffer:		for devices that call iio_push_to_buffers_with_ts_unaligned()
  * @bounce_buffer_size:		size of currently allocate bounce buffer
  * @scan_index_timestamp:	cache of the index to the timestamp
  * @clock_id:			timestamping clock posix identifier

-- 
2.43.0

Re: [PATCH 11/11] iio: core: fix doc reference to iio_push_to_buffers_with_ts_unaligned

[Jonathan Cameron]

On Mon, 25 Nov 2024 22:16:19 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> Use the right name of the function, which is defined in
> drivers/iio/industrialio-buffer.c
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Applied this one via the slow path.

> ---
>  include/linux/iio/iio-opaque.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> index a89e7e43e441..4247497f3f8b 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -28,7 +28,7 @@
>   * @groupcounter:		index of next attribute group
>   * @legacy_scan_el_group:	attribute group for legacy scan elements attribute group
>   * @legacy_buffer_group:	attribute group for legacy buffer attributes group
> - * @bounce_buffer:		for devices that call iio_push_to_buffers_with_timestamp_unaligned()
> + * @bounce_buffer:		for devices that call iio_push_to_buffers_with_ts_unaligned()
>   * @bounce_buffer_size:		size of currently allocate bounce buffer
>   * @scan_index_timestamp:	cache of the index to the timestamp
>   * @clock_id:			timestamping clock posix identifier
>