On Tue, 26 May 2020 12:31:11 -0700
Matt Ranostay <matt.ranostay@xxxxxxxxxxxx> wrote:
> On Mon, May 25, 2020 at 10:09 AM Jonathan Cameron <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 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.
> >
>
> Acked-by: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
This is one of the cases that Andy pointed out will be inconsistent
on x86_32. That only requires 4 byte alignment for the s64 so here
we end up with the wrong amount of padding. I've fixed that up
in v2 with s64 ts __aligned(8);
I've assumed the ack still stands given this is a fairly obscure corner.
Thanks,
Jonathan
>
> > Fixes: 16bf793f86b2 ("iio: humidity: hdc100x: add triggered buffer support for HDC100X")
> > Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Cc: Alison Schofield <amsfield22@xxxxxxxxx>
> > Cc: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
> > ---
> > drivers/iio/humidity/hdc100x.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> > index 7ecd2ffa3132..fd825e281d4f 100644
> > --- a/drivers/iio/humidity/hdc100x.c
> > +++ b/drivers/iio/humidity/hdc100x.c
> > @@ -38,6 +38,11 @@ struct hdc100x_data {
> >
> > /* integration time of the sensor */
> > int adc_int_us[2];
> > + /* Ensure natural alignment of timestamp */
> > + struct {
> > + __be16 channels[2];
> > + s64 ts;
> > + } scan;
> > };
> >
> > /* integration time in us */
> > @@ -322,7 +327,6 @@ static irqreturn_t hdc100x_trigger_handler(int irq, void *p)
> > struct i2c_client *client = data->client;
> > int delay = data->adc_int_us[0] + data->adc_int_us[1];
> > int ret;
> > - s16 buf[8]; /* 2x s16 + padding + 8 byte timestamp */
> >
> > /* dual read starts at temp register */
> > mutex_lock(&data->lock);
> > @@ -333,13 +337,13 @@ static irqreturn_t hdc100x_trigger_handler(int irq, void *p)
> > }
> > usleep_range(delay, delay + 1000);
> >
> > - ret = i2c_master_recv(client, (u8 *)buf, 4);
> > + ret = i2c_master_recv(client, (u8 *)data->scan.channels, 4);
> > if (ret < 0) {
> > dev_err(&client->dev, "cannot read sensor data\n");
> > goto err;
> > }
> >
> > - iio_push_to_buffers_with_timestamp(indio_dev, buf,
> > + iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> > iio_get_time_ns(indio_dev));
> > err:
> > mutex_unlock(&data->lock);
> > --
> > 2.26.2
> >
