[07/25] iio:magnetometer:ak8975 Fix alignment and data leak issues.
diff mbox series

Message ID 20200525170628.503283-8-jic23@kernel.org
State New
Headers show
Series
  • IIO: 2nd set of timestamp alignment fixes.
Related show

Commit Message

Jonathan Cameron May 25, 2020, 5:06 p.m. UTC
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 apart from
previous readings.

Fixes: bc11ca4a0b84 ("iio:magnetometer:ak8975: triggered buffer support")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Gregor Boirie <gregor.boirie@parrot.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/magnetometer/ak8975.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Andy Shevchenko May 26, 2020, 9:24 a.m. UTC | #1
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 :-)
Jonathan Cameron May 26, 2020, 4:50 p.m. UTC | #2
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

Patch
diff mbox series

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;