linux-kernel - Re: [PATCH v2] iio: Fix uninitialized variable

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHhBtNr6rycOKjXvAR6nK9ZKF8Ze=wvki2yetCZDNz-LHA+FvQ@mail.gmail.com>
Date: Fri, 11 Oct 2024 23:01:25 +0800
From: Yo-Jung Lin <0xff07@...il.com>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc: linux-kernel-mentees@...ts.linuxfoundation.org, ricardo@...liere.net, 
	skhan@...uxfoundation.org, Jonathan Cameron <jic23@...nel.org>, 
	Lars-Peter Clausen <lars@...afoo.de>, Nathan Chancellor <nathan@...nel.org>, 
	Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>, 
	Justin Stitt <justinstitt@...gle.com>, Vasileios Amoiridis <vassilisamir@...il.com>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Angel Iglesias <ang.iglesiasg@...il.com>, Adam Rizkalla <ajarizzo@...il.com>, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH v2] iio: Fix uninitialized variable

Hi Javier,

On Fri, Oct 11, 2024 at 8:31 PM Javier Carrasco
<javier.carrasco.cruz@...il.com> wrote:
>
> On 11/10/2024 13:52, Yo-Jung (Leo) Lin wrote:
> > clang found that the "offset" in bmp580_trigger_handler doesn't get
> > initialized before access. Add proper initialization to this variable.
> >
> > Signed-off-by: Yo-Jung (Leo) Lin <0xff07@...il.com>
> > ---
> > Change in v2:
> > - Make value initialization immediate before its first use.
> > - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/
> >
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f4df222ed0c3..682329f81886 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
> >               goto out;
> >       }
> >
> > +     offset = 0;
> > +
> >       /* Pressure calculations */
> >       memcpy(&data->sensor_data[offset], &data->buf[3], 3);
> >
>
> That was a quick reply. I would recommend you to wait a little bit while
> the first version is under discussion.

It was my bad not waiting for enough feedback to finalize another
patch. I’ll definitely do better next time.

I feel that initializing it as sizeof(s32) in the beginning and not
using it until the second memcpy() only widens the gap between value
initialization and its first access, which doesn’t address
Shevchenko’s concern.

>
> I still see the offset thing a bit weird. data->sensor_data uses an
> offset to avoid hard-coded numbers, but for data->buf we do exactly
> that, in the very same lines.
>
> Setting offset to 0 to access the first element i.e. no offset required,
> and then adding the actual offset sizeof(s32), which could even be a
> const if the first access was to sensor_data[0], looks to verbose.

While that is also true, it’ll take a more general refactoring to make
it driver-wise consistent. Maybe that refactoring should be on top of
a fix, instead of being part of a fix.

>
> These things are of course not critical, and the proposed fix is
> definitely ok, but I am missing some consistency here.

Best,
Leo