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

[David Lechner <dlechner@baylibre.com>]

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