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@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.
>
Got it. Thanks, Jonathan!
regards,
dan carpenter
