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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 17 May 2020 21:07:45 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 5/17/20 6:15 PM, Jonathan Cameron wrote:
> > On Sun, 3 May 2020 11:12:34 +0200
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> >  
> >> On 5/3/20 11:07 AM, 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 to put_unaligned(). We probably got lucky in most cases and the
> >>> buffer is naturally aligned to 64 bit.  
> > Just a quick update on this.  I've been taking a deeper look and there
> > are some 'interesting' cases in here so the put_unaligned is attractive
> > unfortunately I don't think we can go that way because it would be
> > reasonable for consumers of the buffer to expect it to be appropriately
> > aligned.   We need to rework many of these anyway to fix the related
> > data leak.
> >
> > I've done some of below and will post shortly - a few will take more
> > effort and probably need testing rather than just relying on review.
> >
> > So far the 'interesting ones' are mpu3050 and isl29501.  
> 
> isl29501 looks OK to me. mpu3050 is clearly broken, buffer is both 
> unaligned and too small!

isl29501 reads a pair of 8 bit registers, then writes them to a 32 bit value
for no particular reason and puts that in the first 32 bits of the buffer
having declared the channel to have a storage size of 16 bits.

So going to have some fun on one or other of the endian types...

J

>