Re: [PATCH v5 5/6] iio: imu: Add support for adis16475

["Nuno Sá" <noname.nuno@gmail.com>]

On Sun, 2020-05-03 at 11:07 +0200, Lars-Peter Clausen wrote:
> On 5/3/20 10:47 AM, Jonathan Cameron wrote:
> > On Sat, 02 May 2020 21:52:18 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > > On Sat, 2020-05-02 at 20:01 +0200, Lars-Peter Clausen wrote:
> > > > On 5/2/20 7:40 PM, Jonathan Cameron wrote:
> > > > > On Mon, 27 Apr 2020 20:06:07 +0200
> > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > >    
> > > > > > On 4/13/20 10:24 AM, Nuno Sá wrote:
> > > > > > > [...]
> > > > > > > +static irqreturn_t adis16475_trigger_handler(int irq,
> > > > > > > void *p)
> > > > > > > +{
> > > > > > > [...]
> > > > > > > +	__be16 data[ADIS16475_MAX_SCAN_DATA], *buffer;
> > > > > > > [...]
> > > > > > > +
> > > > > > > +	iio_push_to_buffers_with_timestamp(indio_dev, data, pf-
> > > > > > > > timestamp);
> > > > > > If the timestamp is enabled the IIO core might insert
> > > > > > padding
> > > > > > between
> > > > > > the data channels and the timestamp. If that happens this
> > > > > > will
> > > > > > disclose
> > > > > > kernel stack memory to userspace.
> > > > > > 
> > > > > > This needs either a memset(data, 0x00, sizeof(data)) or
> > > > > > maybe put
> > > > > > data
> > > > > > into the state struct and kzalloc it.
> > > > > Good spot. Could simply do __be16 data[ADI..] = {0}; rather
> > > > > than
> > > > > explicit
> > > > > memset, but some form of zeroization is needed.
> > > > > 
> > > > > I've fixed up the applied patch with the above approach.
> > > > There is actually another issue. The stack data is not
> > > > necessarily
> > > > aligned to 64 bit, which causes issues if we try to put the 64-
> > > > bit
> > > Oh, this is actually more problematic. Yes, since we have an
> > > array of
> > > u16, that is not guaranteed to be 64bit aligned. Doing a quick
> > > search
> > > of `iio_push_to_buffers_with_timestamp()` users and I could
> > > quickly
> > > find 4/5 drivers with the same problem. I guess the API should
> > > clearly
> > > state that `data` needs to be __at least__ 64 bits aligned (maybe
> > > a
> > > future patch). Or we could even check the address and guarantee
> > > that it
> > > is properly aligned before continuing (though Im guessing this
> > > will
> > > break a lot of users...)
> > > > timestamp in it. I think data should really be in the state
> > > > struct.
> > > Yes, with a proper __aligned(8) attribute... Or couldn't we just
> > > use
> > > __aligned(8) on the stack variable?
> > Forcing alignment on the stack isn't terribly reliable, which is
> > why
> > we never do that for dma safe buffers.
> > 
> > Probably better to just move it to the state structure.
> > I'll fix it up to do that. Please sanity check what will shortly
> > be in the testing branch.
> > 
> > The moment Lars mentioned this I groaned. As you've noted a few
> > other
> > drivers have the same problem + the ABI doesn't clearly state
> > or check this.
> > 
> > We should certainly fix all the drivers that suffer this problem
> > first then we can think about adding a runtime check.
> 
> It looks like it is actually quite a few drivers, maybe we should
> switch 

Yeps, not surprised...

> to put_unaligned(). We probably got lucky in most cases and the
> buffer

This would keep us from having to fix all the users (just need to fix
the memory leakage you mention on the next email) to use a properly
aligned buffer. And later on, if we want, we can always add an
`aligned` variant of `iio_push_to_buffers_with_timestamp()` were we
check for alignment...

- Nuno Sá