On Wed, Jul 22, 2020 at 10:43 PM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > On Wed, Jul 22, 2020 at 6:53 PM 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 24 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 array in the iio_priv() data with alignment > > explicitly requested. This data is allocated with kzalloc so no > > data can leak appart from previous readings. > > > > Depending on the enabled channels, the location of the timestamp > > can be at various aligned offsets through the buffer. As such we > > any use of a structure to enforce this alignment would incorrectly > > suggest a single location for the timestamp. > > ... > > > + /* Ensure timestamp will be naturally aligned if present */ > > + u8 buffer[24] __aligned(8); > > Why can't we use proper structure here? > > > @@ -445,7 +447,6 @@ static irqreturn_t si1145_trigger_handler(int irq, void *private) > > * 6*2 bytes channels data + 4 bytes alignment + > > * 8 bytes timestamp > > */ > > - u8 buffer[24]; > > Seems even the old comment shows how it should look like... I think I understand now. Basically it's a dynamic amount of channels (up to 6) before you get a timestamp. -- With Best Regards, Andy Shevchenko
