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

[Kees Cook <kees@kernel.org>]

On Fri, May 09, 2025 at 08:49:30AM +0300, Dan Carpenter wrote:
> 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?

tl;dr: Use { }.

I didn't think -Os was supported either?

The stackinit KUnit tests will do all these checks, FWIW. It's a little
difficult to decode the results, but this is consistent with the
findings. Looking at a similar case to above, the "small_hole" struct
test:

struct test_small_hole {
        size_t one;
        char two;
        /* 3 byte padding hole here. */
        int three;
        unsigned long four;
};

Tested with various initializers:

#define INIT_STRUCT_none(var_type)      /**/
#define INIT_STRUCT_zero(var_type)      = { }
#define INIT_STRUCT_old_zero(var_type)  = { 0 }

#define __static_partial                { .two = 0, }
#define __static_all                    { .one = 0, \
                                          .two = 0, \
                                          .three = 0, \
                                          .four = 0, \
                                        }
#define __dynamic_partial               { .two = arg->two, }
#define __dynamic_all                   { .one = arg->one, \
                                          .two = arg->two, \
                                          .three = arg->three, \
                                          .four = arg->four, \
                                        }
#define __runtime_partial               var.two = 0
#define __runtime_all                   var.one = 0; \
                                        var.two = 0; \
                                        var.three = 0; \
                                        var.four = 0


We can see a default build (-O2), "PASSED" means all zero, and "SKIPPED"
means "not all zero, but we expected that given the Kconfig involved":

(Running "./tools/testing/kunit/kunit.py run stackinit")

[09:44:47] [PASSED] test_small_hole_zero
[09:44:47] [PASSED] test_small_hole_old_zero
[09:44:47] [PASSED] test_small_hole_dynamic_partial
[09:44:47] [PASSED] test_small_hole_assigned_dynamic_partial
[09:44:47] [PASSED] test_small_hole_static_partial
[09:44:47] [PASSED] test_small_hole_assigned_static_partial

These are "= { }", "= { 0 }", and partial member initialization.

[09:44:47] [SKIPPED] test_small_hole_static_all
[09:44:47] [SKIPPED] test_small_hole_assigned_static_all
[09:44:47] [SKIPPED] test_small_hole_dynamic_all
[09:44:47] [SKIPPED] test_small_hole_assigned_dynamic_all

These are the full member initialization, which, yes, leaves the padding
uninitialized. :(

[09:44:47] [SKIPPED] test_small_hole_runtime_partial
[09:44:47] [SKIPPED] test_small_hole_runtime_all

These set members from runtime values instead of static values, and
padding is left uninitialized.

[09:44:47] [SKIPPED] test_small_hole_none

No init, obviously left uninitialized

[09:44:47] [PASSED] test_small_hole_assigned_copy

This is a full object copy (from a source with initialized padding),
so the result is also initialized.

Building with CONFIG_INIT_STACK_ALL_ZERO=y changes all this, of course.
In that case, everything passes:

./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y stackinit


-- 
Kees Cook