On Sun, 17 May 2020 20:57:26 +0200
Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 5/17/20 7:29 PM, jic23@xxxxxxxxxx wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 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 a 16 byte u8 array 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 with alignment
> > ensured by use of an explicit c structure. This data is allocated
> > with kzalloc so no data can leak appart from previous readings.
> >
> > Fixes: c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")
> > Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Cc: Peter Meerwald <pmeerw@xxxxxxxxxx>
> > ---
> > drivers/iio/accel/mma8452.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> > index 00e100fc845a..704867ffda7a 100644
> > --- a/drivers/iio/accel/mma8452.c
> > +++ b/drivers/iio/accel/mma8452.c
> > @@ -110,6 +110,12 @@ struct mma8452_data {
> > int sleep_val;
> > struct regulator *vdd_reg;
> > struct regulator *vddio_reg;
> > +
> > + /* Ensure correct alignment of time stamp when present */
> > + struct {
> > + __be16 channels[3];
> > + s64 ts;
> > + } buffer;
>
>
> I feel we should have a macro for this.
>
> DECLARE_IIO_BUFFER_WITH_TIMESTAMP(buffer, __be16, 3);
>
> The name is maybe a bit too long.
It runs into the issue we had with the afe4403 and other devices
that are happy to do very different numbers of enabled channels.
Such a macro would imply that the timestamp will always be written
at a fixed location, which isn't true. So far, I've made sure
we didn't do anything like this unless the number of channels was
small enough there was only one possible location for the timestamp
(now we insist that at least one channel is enabled to start a buffer).
Maybe we just deal with that with some suitable documentation though.
>
> And potentially also DECLARE_IIO_BUFFER_WITH_TIMESTAMP_ON_STACK() which
> initializes it to zero.
The thing there is we should only need to initialize it to zero if there
are holes. If we always write data up to the 8 byte boundary
it won't matter if the timestamp is disabled or not, I don't think we
will get a data leak. We can't even do some magic in the call to
identify if there are potential holes because we can't tell if all
the channels will be written or not...
I'm not totally convinced hiding what is going on behind a macro
is a good idea. Sometimes bashing people over the head with the
fact there are some non obvious requirements is a good idea.
Not sure...
J
>
> > };
> >
> > /**
> > @@ -1091,14 +1097,13 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)
> > struct iio_poll_func *pf = p;
> > struct iio_dev *indio_dev = pf->indio_dev;
> > struct mma8452_data *data = iio_priv(indio_dev);
> > - u8 buffer[16]; /* 3 16-bit channels + padding + ts */
> > int ret;
> >
> > - ret = mma8452_read(data, (__be16 *)buffer);
> > + ret = mma8452_read(data, data->buffer.channels);
> > if (ret < 0)
> > goto done;
> >
> > - iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> > iio_get_time_ns(indio_dev));
> >
> > done:
>
>
