| Message ID | 20200525170628.503283-11-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO: 2nd set of timestamp alignment fixes. | expand |
> 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: e4a70e3e7d84 ("iio: humidity: add support to hts221 rh/temp combo device") > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> Hi Jonathan, I guess you can drop HTS221_DATA_SIZE now since it seems no longer used. Acked-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > drivers/iio/humidity/hts221.h | 5 +++++ > drivers/iio/humidity/hts221_buffer.c | 9 +++++---- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h > index 7c650df77556..6ad579ca9cef 100644 > --- a/drivers/iio/humidity/hts221.h > +++ b/drivers/iio/humidity/hts221.h > @@ -39,6 +39,11 @@ struct hts221_hw { > > bool enabled; > u8 odr; > + /* Ensure natural alignment of timestamp */ > + struct { > + __le16 channels[2]; > + s64 ts; > + } scan; > }; > > extern const struct dev_pm_ops hts221_pm_ops; > diff --git a/drivers/iio/humidity/hts221_buffer.c b/drivers/iio/humidity/hts221_buffer.c > index 21c6c160462d..59ede9860185 100644 > --- a/drivers/iio/humidity/hts221_buffer.c > +++ b/drivers/iio/humidity/hts221_buffer.c > @@ -160,7 +160,6 @@ static const struct iio_buffer_setup_ops hts221_buffer_ops = { > > static irqreturn_t hts221_buffer_handler_thread(int irq, void *p) > { > - u8 buffer[ALIGN(2 * HTS221_DATA_SIZE, sizeof(s64)) + sizeof(s64)]; > struct iio_poll_func *pf = p; > struct iio_dev *iio_dev = pf->indio_dev; > struct hts221_hw *hw = iio_priv(iio_dev); > @@ -170,18 +169,20 @@ static irqreturn_t hts221_buffer_handler_thread(int irq, void *p) > /* humidity data */ > ch = &iio_dev->channels[HTS221_SENSOR_H]; > err = regmap_bulk_read(hw->regmap, ch->address, > - buffer, HTS221_DATA_SIZE); > + &hw->scan.channels[0], > + sizeof(hw->scan.channels[0])); > if (err < 0) > goto out; > > /* temperature data */ > ch = &iio_dev->channels[HTS221_SENSOR_T]; > err = regmap_bulk_read(hw->regmap, ch->address, > - buffer + HTS221_DATA_SIZE, HTS221_DATA_SIZE); > + &hw->scan.channels[1], > + sizeof(hw->scan.channels[1])); > if (err < 0) > goto out; > > - iio_push_to_buffers_with_timestamp(iio_dev, buffer, > + iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan, > iio_get_time_ns(iio_dev)); > > out: > -- > 2.26.2 >
On Tue, 26 May 2020 09:52:30 +0200 Lorenzo Bianconi <lorenzo@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. > > > > Fixes: e4a70e3e7d84 ("iio: humidity: add support to hts221 rh/temp combo device") > > Reported-by: Lars-Peter Clausen <lars@metafoo.de> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> > > Hi Jonathan, > > I guess you can drop HTS221_DATA_SIZE now since it seems no longer used. > > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org> This patch suffers from the issue with padding on x86_32 that Andy pointed out. For v2 I've added explicit __aligned(8) Also dropped HTS221_DATA_SIZE as you suggested. I've kept the Ack. Shout if you'd rather I didn't! Jonathan > > > --- > > drivers/iio/humidity/hts221.h | 5 +++++ > > drivers/iio/humidity/hts221_buffer.c | 9 +++++---- > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h > > index 7c650df77556..6ad579ca9cef 100644 > > --- a/drivers/iio/humidity/hts221.h > > +++ b/drivers/iio/humidity/hts221.h > > @@ -39,6 +39,11 @@ struct hts221_hw { > > > > bool enabled; > > u8 odr; > > + /* Ensure natural alignment of timestamp */ > > + struct { > > + __le16 channels[2]; > > + s64 ts; > > + } scan; > > }; > > > > extern const struct dev_pm_ops hts221_pm_ops; > > diff --git a/drivers/iio/humidity/hts221_buffer.c b/drivers/iio/humidity/hts221_buffer.c > > index 21c6c160462d..59ede9860185 100644 > > --- a/drivers/iio/humidity/hts221_buffer.c > > +++ b/drivers/iio/humidity/hts221_buffer.c > > @@ -160,7 +160,6 @@ static const struct iio_buffer_setup_ops hts221_buffer_ops = { > > > > static irqreturn_t hts221_buffer_handler_thread(int irq, void *p) > > { > > - u8 buffer[ALIGN(2 * HTS221_DATA_SIZE, sizeof(s64)) + sizeof(s64)]; > > struct iio_poll_func *pf = p; > > struct iio_dev *iio_dev = pf->indio_dev; > > struct hts221_hw *hw = iio_priv(iio_dev); > > @@ -170,18 +169,20 @@ static irqreturn_t hts221_buffer_handler_thread(int irq, void *p) > > /* humidity data */ > > ch = &iio_dev->channels[HTS221_SENSOR_H]; > > err = regmap_bulk_read(hw->regmap, ch->address, > > - buffer, HTS221_DATA_SIZE); > > + &hw->scan.channels[0], > > + sizeof(hw->scan.channels[0])); > > if (err < 0) > > goto out; > > > > /* temperature data */ > > ch = &iio_dev->channels[HTS221_SENSOR_T]; > > err = regmap_bulk_read(hw->regmap, ch->address, > > - buffer + HTS221_DATA_SIZE, HTS221_DATA_SIZE); > > + &hw->scan.channels[1], > > + sizeof(hw->scan.channels[1])); > > if (err < 0) > > goto out; > > > > - iio_push_to_buffers_with_timestamp(iio_dev, buffer, > > + iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan, > > iio_get_time_ns(iio_dev)); > > > > out: > > -- > > 2.26.2 > >
diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h index 7c650df77556..6ad579ca9cef 100644 --- a/drivers/iio/humidity/hts221.h +++ b/drivers/iio/humidity/hts221.h @@ -39,6 +39,11 @@ struct hts221_hw { bool enabled; u8 odr; + /* Ensure natural alignment of timestamp */ + struct { + __le16 channels[2]; + s64 ts; + } scan; }; extern const struct dev_pm_ops hts221_pm_ops; diff --git a/drivers/iio/humidity/hts221_buffer.c b/drivers/iio/humidity/hts221_buffer.c index 21c6c160462d..59ede9860185 100644 --- a/drivers/iio/humidity/hts221_buffer.c +++ b/drivers/iio/humidity/hts221_buffer.c @@ -160,7 +160,6 @@ static const struct iio_buffer_setup_ops hts221_buffer_ops = { static irqreturn_t hts221_buffer_handler_thread(int irq, void *p) { - u8 buffer[ALIGN(2 * HTS221_DATA_SIZE, sizeof(s64)) + sizeof(s64)]; struct iio_poll_func *pf = p; struct iio_dev *iio_dev = pf->indio_dev; struct hts221_hw *hw = iio_priv(iio_dev); @@ -170,18 +169,20 @@ static irqreturn_t hts221_buffer_handler_thread(int irq, void *p) /* humidity data */ ch = &iio_dev->channels[HTS221_SENSOR_H]; err = regmap_bulk_read(hw->regmap, ch->address, - buffer, HTS221_DATA_SIZE); + &hw->scan.channels[0], + sizeof(hw->scan.channels[0])); if (err < 0) goto out; /* temperature data */ ch = &iio_dev->channels[HTS221_SENSOR_T]; err = regmap_bulk_read(hw->regmap, ch->address, - buffer + HTS221_DATA_SIZE, HTS221_DATA_SIZE); + &hw->scan.channels[1], + sizeof(hw->scan.channels[1])); if (err < 0) goto out; - iio_push_to_buffers_with_timestamp(iio_dev, buffer, + iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan, iio_get_time_ns(iio_dev)); out: