Re: [bug report] iio: magnetometer: add Allegro MicroSystems ALS31300 3-D Linear Hall Effect driver

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 4 Feb 2025 14:18:36 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> Hello Neil Armstrong,
> 
> Commit 3c9b6fd74188 ("iio: magnetometer: add Allegro MicroSystems
> ALS31300 3-D Linear Hall Effect driver") from Oct 30, 2024
> (linux-next), leads to the following Smatch static checker warning:
> 
> 	drivers/iio/magnetometer/als31300.c:248 als31300_trigger_handler()
> 	warn: check that 'scan.timestamp' doesn't leak information
> 
> drivers/iio/magnetometer/als31300.c
>     226 static irqreturn_t als31300_trigger_handler(int irq, void *p)
>     227 {
>     228         struct iio_poll_func *pf = p;
>     229         struct iio_dev *indio_dev = pf->indio_dev;
>     230         struct als31300_data *data = iio_priv(indio_dev);
>     231         struct {
>     232                 u16 temperature;
>     233                 s16 channels[3];
>     234                 aligned_s64 timestamp;
>     235         } scan;
>     236         s16 x, y, z;
>     237         int ret;
>     238         u16 t;
>     239 
>     240         ret = als31300_get_measure(data, &t, &x, &y, &z);
>     241         if (ret)
>     242                 goto trigger_out;
>     243 
>     244         scan.temperature = t;
>     245         scan.channels[0] = x;
>     246         scan.channels[1] = y;
>     247         scan.channels[2] = z;
> --> 248         iio_push_to_buffers_with_timestamp(indio_dev, &scan,  
>     249                                            pf->timestamp);
> 
> So I guess we had some CVEs recently with regards to
> iio_push_to_buffers_with_timestamp() so this was added as a "must be
> initialized" thing.  The "aligned_s64 timestamp" struct member is
> sometimes initialized in iio_push_to_buffers_with_timestamp() but not
> always.  So this seems like a valid static checker warning?
Hi Dan,

It's a false positive. When it's not initialized it is also never
used.  No code beyond that iio_push_to_buffers_with_timestamp() can
assume there is even data there. In the common case of it being a
kfifo the elements aren't big enough to store the timestamp if
it's not enabled. So it never gets to userspace.

The other bugs were around holes in the structure.  Those can
get to userspace.  On my todo list is a patch to add
a size parameter to that function so we can verify the passed
buffer is big enough, but that won't change how the data is used.

Jonathan

> 
> These are simple enoug to fix if we add a "scan = {};" initializer
> but the IIO function is slightly new to me so I thought I would consult.
> There was also one in trigger_handler().
> 
> drivers/media/pci/mgb4/mgb4_trigger.c:99 trigger_handler()
> warn: check that 'scan' doesn't leak information (struct has a hole after 'data')
> 
>     250 
>     251 trigger_out:
>     252         iio_trigger_notify_done(indio_dev->trig);
>     253 
>     254         return IRQ_HANDLED;
>     255 }
> 
> regards,
> dan carpenter
>