On Wed, 22 Jul 2020 16:51:02 +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 32 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() data with alignment
> explicitly requested. This data is allocated with kzalloc so no
> data can leak apart from previous readings. The explicit alignment
> isn't technically needed here, but it reduced fragility and avoids
> cut and paste into drivers where it will be needed.
>
> If we want this in older stables will need manual backport due to
> driver reworks.
>
> Fixes: c43a102e67db ("iio: ina2xx: add support for TI INA2xx Power Monitors")
> Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Cc: Stefan Brüns <stefan.bruens@xxxxxxxxxxxxxx>
> Cc: Marc Titinger <mtitinger@xxxxxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
Applied and marked for stable.
Thanks,
Jonathan
> ---
> drivers/iio/adc/ina2xx-adc.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 5ed63e874292..b573ec60a8b8 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -146,6 +146,11 @@ struct ina2xx_chip_info {
> int range_vbus; /* Bus voltage maximum in V */
> int pga_gain_vshunt; /* Shunt voltage PGA gain */
> bool allow_async_readout;
> + /* data buffer needs space for channel data and timestamp */
> + struct {
> + u16 chan[4];
> + u64 ts __aligned(8);
> + } scan;
> };
>
> static const struct ina2xx_config ina2xx_config[] = {
> @@ -738,8 +743,6 @@ static int ina2xx_conversion_ready(struct iio_dev *indio_dev)
> static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> {
> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> - /* data buffer needs space for channel data and timestap */
> - unsigned short data[4 + sizeof(s64)/sizeof(short)];
> int bit, ret, i = 0;
> s64 time;
>
> @@ -758,10 +761,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> if (ret < 0)
> return ret;
>
> - data[i++] = val;
> + chip->scan.chan[i++] = val;
> }
>
> - iio_push_to_buffers_with_timestamp(indio_dev, data, time);
> + iio_push_to_buffers_with_timestamp(indio_dev, &chip->scan, time);
>
> return 0;
> };
