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

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

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

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

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

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

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
>

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