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

[Lars-Peter Clausen <lars@metafoo.de>]

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!