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

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

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.
>
> 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
>
Most of these drivers also seem to have the issue that they leak stack 
memory. Only 11 of them clear the buffer.