| Message ID | 20200525170628.503283-10-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 10:09 AM 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 apart > from previous readings. > Acked-by: Matt Ranostay <matt.ranostay@konsulko.com> > Fixes: 16bf793f86b2 ("iio: humidity: hdc100x: add triggered buffer support for HDC100X") > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Alison Schofield <amsfield22@gmail.com> > Cc: Matt Ranostay <matt.ranostay@konsulko.com> > --- > 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 >
On Tue, 26 May 2020 12:31:11 -0700 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > On Mon, May 25, 2020 at 10:09 AM 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 apart > > from previous readings. > > > > Acked-by: Matt Ranostay <matt.ranostay@konsulko.com> 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@metafoo.de> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Alison Schofield <amsfield22@gmail.com> > > Cc: Matt Ranostay <matt.ranostay@konsulko.com> > > --- > > 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 > >
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);