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

From: Jonathan Cameron <jic23@kernel.org>
To: "Nuno Sá" <noname.nuno@gmail.com>
Cc: "Lars-Peter Clausen" <lars@metafoo.de>,
	"Nuno Sá" <nuno.sa@analog.com>,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	"Hartmut Knaack" <knaack.h@gmx.de>,
	"Peter Meerwald-Stadler" <pmeerw@pmeerw.net>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Alexandru Ardelean" <alexandru.Ardelean@analog.com>,
	"Michael Hennerich" <Michael.Hennerich@analog.com>
Subject: Re: [PATCH v5 5/6] iio: imu: Add support for adis16475
Date: Sun, 3 May 2020 09:47:51 +0100	[thread overview]
Message-ID: <20200503094751.633feda1@archlinux> (raw)
In-Reply-To: <df0f04296fafccfcf18b08b213bae20ff69c2384.camel@gmail.com>

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.

Jonathan
> 
> - Nuno Sá
> 
> 