On Wed, 22 Jul 2020 16:50:45 +0100
Jonathan Cameron <jic23@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 a 16 byte 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().
> This data is allocated with kzalloc so no data can leak appart
> from previous readings.
>
> It is necessary to force the alignment of ts to avoid the padding
> on x86_32 being different from 64 bit platorms (it alows for
> 4 bytes aligned 8 byte types.
>
> Fixes: 06ad7ea10e2b ("max44000: Initial triggered buffer support")
> Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
Applied and marked for stable
Thanks,
Jonathan
> ---
> drivers/iio/light/max44000.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index aa8ed1e3e89a..b8e721bced5b 100644
> --- a/drivers/iio/light/max44000.c
> +++ b/drivers/iio/light/max44000.c
> @@ -75,6 +75,11 @@
> struct max44000_data {
> struct mutex lock;
> struct regmap *regmap;
> + /* Ensure naturally aligned timestamp */
> + struct {
> + u16 channels[2];
> + s64 ts __aligned(8);
> + } scan;
> };
>
> /* Default scale is set to the minimum of 0.03125 or 1 / (1 << 5) lux */
> @@ -488,7 +493,6 @@ static irqreturn_t max44000_trigger_handler(int irq, void *p)
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> struct max44000_data *data = iio_priv(indio_dev);
> - u16 buf[8]; /* 2x u16 + padding + 8 bytes timestamp */
> int index = 0;
> unsigned int regval;
> int ret;
> @@ -498,17 +502,17 @@ static irqreturn_t max44000_trigger_handler(int irq, void *p)
> ret = max44000_read_alsval(data);
> if (ret < 0)
> goto out_unlock;
> - buf[index++] = ret;
> + data->scan.channels[index++] = ret;
> }
> if (test_bit(MAX44000_SCAN_INDEX_PRX, indio_dev->active_scan_mask)) {
> ret = regmap_read(data->regmap, MAX44000_REG_PRX_DATA, ®val);
> if (ret < 0)
> goto out_unlock;
> - buf[index] = regval;
> + data->scan.channels[index] = regval;
> }
> mutex_unlock(&data->lock);
>
> - 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));
> iio_trigger_notify_done(indio_dev->trig);
> return IRQ_HANDLED;
