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

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

[Dan Carpenter]

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?

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

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

[Jonathan Cameron]

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
> 

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

[Dan Carpenter]

On Tue, Feb 04, 2025 at 07:33:04PM +0000, Jonathan Cameron wrote:
> 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.
> 

Got it.  Thanks, Jonathan!

regards,
dan carpenter