On Wed, 22 Jul 2020 22:45:59 +0300 Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > 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. > Exactly. Comment is giving the largest it can be, not what is needed for a given configuration of the device. Should indeed drop that comment. Obviously went into automated mode and stopped actually reading what was in front of me. Jonathan
