On Wed, 27 Nov 2024 01:30:36 +0100
Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> 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@xxxxxxxxx> 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@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.
> >> 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
> >
>
