| Message ID | 20200720152822.437100100@linuxfoundation.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Mon, 2020-07-20 at 17:35 +0200, Greg Kroah-Hartman wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > commit ea5e7a7bb6205d24371373cd80325db1bc15eded upstream. > > 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. [] > +++ 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 __aligned(8); Why does an s64 need __aligned(8) ? This seems needlessly redundant. Isn't this naturally aligned by the compiler? The struct isn't packed.
On Mon, Jul 20, 2020 at 9:50 AM Joe Perches <joe@perches.com> wrote: > > On Mon, 2020-07-20 at 17:35 +0200, Greg Kroah-Hartman wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > commit ea5e7a7bb6205d24371373cd80325db1bc15eded upstream. > > > > 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. > [] > > +++ 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 __aligned(8); > > Why does an s64 need __aligned(8) ? This is due to on 32-bit x86 it is aligned to 4 bytes by default. - Matt > This seems needlessly redundant. > > Isn't this naturally aligned by the compiler? > > The struct isn't packed. >
--- 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 __aligned(8); + } scan; }; /* integration time in us */ @@ -319,7 +324,6 @@ static irqreturn_t hdc100x_trigger_handl 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); @@ -330,13 +334,13 @@ static irqreturn_t hdc100x_trigger_handl } 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);