On 5/25/20 7:52 PM, Andy Shevchenko wrote:
On Mon, May 25, 2020 at 06:06:22PM +0100, Jonathan Cameron 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. Here we use an explicit structure and rely on that to enforce alignment on the stack. Note there was never a data leak here due to the explicit memset. Fixes: ecc24e72f437 ("iio: adc: Add TI ADS1015 ADC driver support") Reported-by: Lars-Peter Clausen <lars@xxxxxxxxxx> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> --- drivers/iio/adc/ti-ads1015.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c index 5ea4f45d6bad..05853723dbdb 100644 --- a/drivers/iio/adc/ti-ads1015.c +++ b/drivers/iio/adc/ti-ads1015.c @@ -385,10 +385,14 @@ static irqreturn_t ads1015_trigger_handler(int irq, void *p) struct iio_poll_func *pf = p; struct iio_dev *indio_dev = pf->indio_dev; struct ads1015_data *data = iio_priv(indio_dev); - s16 buf[8]; /* 1x s16 ADC val + 3x s16 padding + 4x s16 timestamp */ + /* Ensure natural alignment for buffer elements */ + struct { + s16 channel; + s64 ts; + } scan;Hmm... On x86_32 and x86_64 this will give different padding. Is it okay from iio_push_to_buffers_with_timestamp() point of view?
No, this is terrible. IIO expects 64 bit words to be 64 bit aligned. Looks like we can't rely on implicit padding, but need to always explicitly specify it.
Or maybe we can typedef and IIO timestamp type with an explicit __aligned attribute. I wonder if that works... After having a quick look, the kernel already defines aligned_u64, so maybe using that is an option.
