| Message ID | 20200525170628.503283-7-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO: 2nd set of timestamp alignment fixes. | expand |
On Mon, May 25, 2020 at 7:09 PM Jonathan Cameron <jic23@kernel.org> wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > One of a class of bugs pointed out by Lars in a recent review. > iio_push_to_buffers_with_timestamp assumes the buffer used is aligned > to the size of the timestamp (8 bytes). This is not guaranteed in > this driver which uses an array of smaller elements on the stack. > As Lars also noted this anti pattern can involve a leak of data to > userspace and that indeed can happen here. We close both issues by > moving to a suitable structure in the iio_priv() data. > > This data is allocated with kzalloc so no data can leak appart from > previous readings. > > Fixes: 7c94a8b2ee8cf ("iio: magn: add a driver for AK8974") > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Linus Walleij <linus.walleij@linaro.org> Whoa, good catch! Definitely my mindless coding. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, 26 May 2020 11:24:40 +0200 Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, May 25, 2020 at 7:09 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > One of a class of bugs pointed out by Lars in a recent review. > > iio_push_to_buffers_with_timestamp assumes the buffer used is aligned > > to the size of the timestamp (8 bytes). This is not guaranteed in > > this driver which uses an array of smaller elements on the stack. > > As Lars also noted this anti pattern can involve a leak of data to > > userspace and that indeed can happen here. We close both issues by > > moving to a suitable structure in the iio_priv() data. > > > > This data is allocated with kzalloc so no data can leak appart from > > previous readings. > > > > Fixes: 7c94a8b2ee8cf ("iio: magn: add a driver for AK8974") > > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Whoa, good catch! Definitely my mindless coding. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I've tweaked this slightly fro v2 to add an __aligned(8) to the ts. This is driven by the need for some cases to be careful on x86_32 where the s64 might be 4 byte aligned and the padding come out wrong. It doesn't actually matter in this case but I'd rather be explicit. Have kept the reviewed-by as not a material change on this one. Thanks, Jonathan > > Yours, > Linus Walleij
diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c index 810fdfd37c88..899795add957 100644 --- a/drivers/iio/magnetometer/ak8974.c +++ b/drivers/iio/magnetometer/ak8974.c @@ -192,6 +192,11 @@ struct ak8974 { bool drdy_irq; struct completion drdy_complete; bool drdy_active_low; + /* Ensure timestamp is naturally aligned */ + struct { + __le16 channels[3]; + s64 ts; + } scan; }; static const char ak8974_reg_avdd[] = "avdd"; @@ -657,7 +662,6 @@ static void ak8974_fill_buffer(struct iio_dev *indio_dev) { struct ak8974 *ak8974 = iio_priv(indio_dev); int ret; - __le16 hw_values[8]; /* Three axes + 64bit padding */ pm_runtime_get_sync(&ak8974->i2c->dev); mutex_lock(&ak8974->lock); @@ -667,13 +671,13 @@ static void ak8974_fill_buffer(struct iio_dev *indio_dev) dev_err(&ak8974->i2c->dev, "error triggering measure\n"); goto out_unlock; } - ret = ak8974_getresult(ak8974, hw_values); + ret = ak8974_getresult(ak8974, ak8974->scan.channels); if (ret) { dev_err(&ak8974->i2c->dev, "error getting measures\n"); goto out_unlock; } - iio_push_to_buffers_with_timestamp(indio_dev, hw_values, + iio_push_to_buffers_with_timestamp(indio_dev, &ak8974->scan, iio_get_time_ns(indio_dev)); out_unlock: