| Message ID | 20200525170628.503283-8-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 06:06:10PM +0100, Jonathan Cameron 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. I'm not sure I understood the issue in full. s16 members are aligned on 2 bytes at least. Unaligned access easily to fix by moving from (int64_t *)... = ...; to put_unaligned(); in iio_push_to_buffers_with_timestamp() once for all. On stack the driver allocates proper amount of data with padding. > This data is allocated with kzalloc so no data can leak apart from > previous readings. > > Fixes: bc11ca4a0b84 ("iio:magnetometer:ak8975: triggered buffer support") Unfortunately breaks as per other patch review :-)
On Tue, 26 May 2020 12:24:15 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, May 25, 2020 at 06:06:10PM +0100, Jonathan Cameron 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. > > I'm not sure I understood the issue in full. > > s16 members are aligned on 2 bytes at least. Unaligned access easily to fix by > moving from (int64_t *)... = ...; to put_unaligned(); in > iio_push_to_buffers_with_timestamp() once for all. The problem is that consumers of the buffer also need to know that it's potentially unaligned. So ultimately all consumers need to then do a get_unaligned for the timestamp. Note in some of these cases they would need to do a get_unaligned for the channels as well. So I think we are better off fixing all these up and ensuring predictability. Moving to the heap here was just to avoid having to memset the thing each time rather than the alignment issue. > > On stack the driver allocates proper amount of data with padding. > > > This data is allocated with kzalloc so no data can leak apart from > > previous readings. > > > > Fixes: bc11ca4a0b84 ("iio:magnetometer:ak8975: triggered buffer support") > > Unfortunately breaks as per other patch review :-) > Actually I think this one is coincidentally fine as we have 6 bytes of channels, but see that other thread for why. + we'd still want to change the code here to make that explicit. Jonathan
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c index 3c881541ae72..673ac70d014d 100644 --- a/drivers/iio/magnetometer/ak8975.c +++ b/drivers/iio/magnetometer/ak8975.c @@ -365,6 +365,12 @@ struct ak8975_data { struct iio_mount_matrix orientation; struct regulator *vdd; struct regulator *vid; + + /* Ensure natural alignment of timestamp */ + struct { + s16 channels[3]; + s64 ts; + } scan; }; /* Enable attached power regulator if any. */ @@ -787,7 +793,6 @@ static void ak8975_fill_buffer(struct iio_dev *indio_dev) const struct i2c_client *client = data->client; const struct ak_def *def = data->def; int ret; - s16 buff[8]; /* 3 x 16 bits axis values + 1 aligned 64 bits timestamp */ __le16 fval[3]; mutex_lock(&data->lock); @@ -810,11 +815,14 @@ static void ak8975_fill_buffer(struct iio_dev *indio_dev) mutex_unlock(&data->lock); /* Clamp to valid range. */ - buff[0] = clamp_t(s16, le16_to_cpu(fval[0]), -def->range, def->range); - buff[1] = clamp_t(s16, le16_to_cpu(fval[1]), -def->range, def->range); - buff[2] = clamp_t(s16, le16_to_cpu(fval[2]), -def->range, def->range); - - iio_push_to_buffers_with_timestamp(indio_dev, buff, + data->scan.channels[0] = + clamp_t(s16, le16_to_cpu(fval[0]), -def->range, def->range); + data->scan.channels[1] = + clamp_t(s16, le16_to_cpu(fval[1]), -def->range, def->range); + data->scan.channels[2] = + clamp_t(s16, le16_to_cpu(fval[2]), -def->range, def->range); + + iio_push_to_buffers_with_timestamp(indio_dev, &data->scan, iio_get_time_ns(indio_dev)); return;