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?
- Nuno Sá
