Re: [PATCH 10/25] iio:humidity:hts221 Fix alignment and data leak issues

[Jonathan Cameron <jic23@kernel.org>]

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
> >