On 5/3/20 10:47 AM, Jonathan Cameron wrote:
On Sat, 02 May 2020 21:52:18 +0200
Nuno Sá <noname.nuno@xxxxxxxxx> 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@xxxxxxxxxx> 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
