On Tue, 4 Feb 2025 14:18:36 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
>
