On Tue, 26 May 2020 12:24:15 +0300
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> On Mon, May 25, 2020 at 06:06:10PM +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.
> > 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.
>
> I'm not sure I understood the issue in full.
>
> s16 members are aligned on 2 bytes at least. Unaligned access easily to fix by
> moving from (int64_t *)... = ...; to put_unaligned(); in
> iio_push_to_buffers_with_timestamp() once for all.
The problem is that consumers of the buffer also need to know
that it's potentially unaligned. So ultimately all consumers need
to then do a get_unaligned for the timestamp.
Note in some of these cases they would need to do a get_unaligned
for the channels as well. So I think we are better off fixing all
these up and ensuring predictability.
Moving to the heap here was just to avoid having to memset the thing
each time rather than the alignment issue.
>
> On stack the driver allocates proper amount of data with padding.
>
> > This data is allocated with kzalloc so no data can leak apart from
> > previous readings.
> >
> > Fixes: bc11ca4a0b84 ("iio:magnetometer:ak8975: triggered buffer support")
>
> Unfortunately breaks as per other patch review :-)
>
Actually I think this one is coincidentally fine as we have 6 bytes of channels,
but see that other thread for why. + we'd still want to change the
code here to make that explicit.
Jonathan
