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

["Javier Carrasco" <javier.carrasco.cruz@gmail.com>]

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