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

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

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.

But the reason I noticed this is because I ran into the issue in the 
wild where the timestamp ended up at the wrong offset in the buffer, so 
it does happen.

The following semantic patch finds affected drivers.

@@
type T;
identifier buf;
expression N;
expression ts;
expression indio_dev;
@@
*T buf[N];
...
*iio_push_to_buffers_with_timestamp(indio_dev, buf, ts)

Matched files:

--- drivers/iio/health/afe4403.c
--- drivers/iio/health/afe4404.c
--- drivers/iio/gyro/mpu3050-core.c
--- drivers/iio/gyro/itg3200_buffer.c
--- drivers/iio/chemical/ccs811.c
--- drivers/iio/chemical/sps30.c
--- drivers/iio/chemical/pms7003.c
--- drivers/iio/proximity/isl29501.c
--- drivers/iio/proximity/mb1232.c
--- drivers/iio/accel/kxsd9.c
--- drivers/iio/accel/mma8452.c
--- drivers/iio/accel/bmc150-accel-core.c
--- drivers/iio/accel/mma7455_core.c
--- drivers/iio/adc/ti-adc081c.c
--- drivers/iio/adc/ti-adc084s021.c
--- drivers/iio/adc/ti-ads1015.c
--- drivers/iio/adc/ti-ads124s08.c
--- drivers/iio/adc/ina2xx-adc.c
--- drivers/iio/adc/ti-ads8688.c
--- drivers/iio/adc/ti-adc0832.c
--- drivers/iio/adc/ti-adc12138.c
--- drivers/iio/adc/max1118.c
--- drivers/iio/adc/ad_sigma_delta.c
--- drivers/iio/light/si1145.c
--- drivers/iio/light/vcnl4035.c
--- drivers/iio/light/max44000.c
--- drivers/iio/light/rpr0521.c
--- drivers/iio/light/st_uvis25_core.c
--- drivers/iio/light/ltr501.c
--- drivers/iio/magnetometer/ak8974.c
--- drivers/iio/magnetometer/mag3110.c
--- drivers/iio/magnetometer/ak8975.c
--- drivers/iio/humidity/hdc100x.c
--- drivers/iio/humidity/hts221_buffer.c
--- drivers/iio/imu/bmi160/bmi160_core.c
--- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
--- drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
--- drivers/iio/pressure/ms5611_core.c
--- drivers/iio/pressure/mpl3115.c