Re: [bug report] iio: pressure: bmp280: drop sensor_data array

[Dan Carpenter <dan.carpenter@linaro.org>]

Let me add Kees to the CC list because he'll want to know this as well.

On Wed, May 07, 2025 at 08:33:07AM -0500, David Lechner wrote:
> On 5/7/25 2:41 AM, Dan Carpenter wrote:
> > On Wed, May 07, 2025 at 07:35:27AM +0100, Jonathan Cameron wrote:
> >>
> >>
> >> On 6 May 2025 19:35:08 BST, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >>> On Tue, May 06, 2025 at 09:25:00AM -0500, David Lechner wrote:
> >>>> On 5/6/25 7:32 AM, Dan Carpenter wrote:
> >>>>> Hello David Lechner,
> >>>>>
> >>>>> Commit 4e6c3c4801a6 ("iio: pressure: bmp280: drop sensor_data array")
> >>>>> from Apr 22, 2025 (linux-next), leads to the following Smatch static
> >>>>> checker warning:
> >>>>>
> >>>>> 	drivers/iio/pressure/bmp280-core.c:1280 bme280_trigger_handler()
> >>>>> 	warn: check that 'buffer' doesn't leak information (struct has a hole after 'comp_humidity')
> >>>>>
> >>>>> drivers/iio/pressure/bmp280-core.c
> >>>>>     1225 static irqreturn_t bme280_trigger_handler(int irq, void *p)
> >>>>>     1226 {
> >>>>>     1227         struct iio_poll_func *pf = p;
> >>>>>     1228         struct iio_dev *indio_dev = pf->indio_dev;
> >>>>>     1229         struct bmp280_data *data = iio_priv(indio_dev);
> >>>>>     1230         u32 adc_temp, adc_press, adc_humidity;
> >>>>>     1231         s32 t_fine;
> >>>>>     1232         struct {
> >>>>>     1233                 u32 comp_press;
> >>>>>     1234                 s32 comp_temp;
> >>>>>     1235                 u32 comp_humidity;
> >>>>>     1236                 aligned_s64 timestamp;
> >>>>>
> >>>>> There is a 4 byte hole between comp_humidity and timestamp.
> >>>>
> >>>> Yes, this was the intention of this patch.
> >>>>
> >>>>>
> >>>>>     1237         } buffer;
> >>>>>     1238         int ret;
> >>>>>     1239 
> >>>>
> >>>> ...
> >>>>
> >>>>>     1279 
> >>>>> --> 1280         iio_push_to_buffers_with_ts(indio_dev, &buffer, sizeof(buffer),
> >>>>>                                                         ^^^^^^^^^^^^^^^^^^^^^^^
> >>>>> So I believe it leads to an information leaks here.
> >>>>
> >>>> Aha, so we should e.g. do memset() to fill the hole first.
> >>>>
> >>>
> >>> That works, or you could initialize it with = { }.
> >>
> >> I tried to track this down the other day.
> >> What are compilers guaranteeing around
> >> that vs { 0 }  and holes?  The c spec has only recently standardised on { }.
> >>
> >> I'd love to stop using memset for these.
> > 
> > The rule in the C standard is that if the initializer sets every struct
> > member then it will NOT zero out struct holes.  But if there are any
> > unset struct members then the holes are zeroed.  So = { } will always
> > work.  You really have to try hard to invent a scenario where = { 0 }
> > won't fill the struct holes (a one member struct with a weird alignment).
> > 
> > regards,
> > dan carpenter
> > 
> 
> I was curious about this too and came across a blog post [1] that claims that
> with clang compiler and certain optimization levels, { } and { 0 } still aren't
> good enough, which is why I went with the memset().
> 
> [1]: https://interrupt.memfault.com/blog/c-struct-padding-initialization

Huh...

"Strategy 2, explicitly setting each struct member".  This isn't
"supposed" to initialize struct holes according to the C current
standard.

It's discouraging that = { 0 } and { } don't work.  -O1 is not supported
in the kernel so that's not an emergency.  I don't know about -Os?

regards,
dan carpenter