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... -- With Best Regards, Andy Shevchenko
