On Tue, 26 May 2020 09:52:30 +0200
Lorenzo Bianconi <lorenzo@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.
> >
> > Fixes: e4a70e3e7d84 ("iio: humidity: add support to hts221 rh/temp combo device")
> > Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Cc: Lorenzo Bianconi <lorenzo.bianconi83@xxxxxxxxx>
>
> Hi Jonathan,
>
> I guess you can drop HTS221_DATA_SIZE now since it seems no longer used.
>
> Acked-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
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
> >
