On 5/17/20 6:15 PM, Jonathan Cameron wrote:
On Sun, 3 May 2020 11:12:34 +0200
Lars-Peter Clausen <lars@xxxxxxxxxx> 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@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.
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!
