[PATCH v4 0/8] IIO: Fused set 1 and 2 of timestamp alignment fixes

* [PATCH v4 0/8] IIO: Fused set 1 and 2 of timestamp alignment fixes
@ 2020-09-20 11:27 Jonathan Cameron
  2020-09-20 11:27 ` [PATCH v4 1/8] iio:light:rpr0521: Fix timestamp alignment and prevent data leak Jonathan Cameron
                   ` (8 more replies)
  0 siblings, 9 replies; 14+ messages in thread
From: Jonathan Cameron @ 2020-09-20 11:27 UTC (permalink / raw)
  To: linux-iio; +Cc: Andy Shevchenko, Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Took me a while to get back to these.  We have 2 new patches in here to
fix issues unrelated to the main topic, but which effect the buffer lengths.
I've done those as precursors so it is clear what is going on.

Note there are still a few outstanding drivers to be fixed before we can
think about adding a warning if unaligned buffers are provided. Naturally
they are the hardest ones, or the ones where I couldn't work out how
the code is working today, so may take a little while.

Changes since v3:
* Applied all the ones where only minor comment changes were needed.
* rpr0521: Fixed typo. Also added to patch description Mikko's information
  on why it would be costly to split off the interrupt read.
* st_uvis: Drop the pointless masking.
* mag3110: Rename element to temperature
* bmi160: Add fix to length of buffer.
* bmi160: Improve comments and carry forwards shorter length.
* mpl3115: Sufficiently unusual to need a 'special' comment and another review.
* ti-ads124s08: Add fix to length of buffer.
* ti-ads124s08: Expand comment to express the buffer length not all needed if
  not all channels are enabled.

Changes since v2:
* bmc150-accel: Use sizeof() for channel size (Andy Shevchenko)
* st_uvis25: Use local variable for regmap call (Andy Shevchenko)
* st_lsm6dsx: Use array of scan[] rather than 3 structures (Lorenzo Bianconi)
* inv_mpu6050: Add patch switching to a regmap_noinc_read (Jean-Baptiste Maneyrol)
* ina2xx: Use a structure (previously failed to notice that works here)
* I've added clarifying notes to patch descriptions based on questions asked.
  These were mainly about why we didn't use the structure approach everywhere
  and why I've forced alignment in places it wasn't strictly needed.

Previous cover letter:
A few notes based on questions on v1.

1. Why not use put_unaligned to avoid the whole thing?
   This interface can pass directly to a variety of in kernel users
   who would reasonably assume that we have true natural alignment.
   When it gets passed directly is subtle as it depends on whether
   the demux has to realign the data or not.  So enabling an extra
   channel could result in a previously working alignment no longer
   being true.

   Even if this is fine for existing usecases we are likely to
   store up subtle long term issues if we don't fix it explicitly.
   It's also worth noting that the data channel sometimes suffered
   the same problem as the timestamp.

2. Why not specify explicit padding?
   In my view this is error prone in comparisom with relying on
   c to do the hard work for us.

3. Why not move the timestamp to the start?
   ABI breakage and as timestamp is optional (no obvious from the
   iio_push_to_buffers_with_timestamp call) we can end up having
   to shift the rest of the data within that call.

Changes since v1.

Andy Schevchenko pointed out that on x86_32 s64 elements are only
aligned to 4 bytes.  Where I had tried to use a structure to avoid
explicit need to list the padding, there were some cases where
this results in insufficient padding being inserted.

This doesn't affect the few patches that had already been applied and
sent upstream. (which was lucky ;)

The fix was to take advantage of __aligned(8) which (according to
my reading of the c spec and the gcc docs) enforces the alignment of
both the element within a structure and the structure itself.
The kernel now requires a recent enough version of GCC to ensure this
works both on the stack and heap.  This is done in lots of other
userspace interfaces. In some cases iio_push_to_buffers_with_ts
is aligning data for passing to userspace, be it via a kfifo
so it is sensible we should use the same solution.

Note that we could have used u64_aligned but there is no equivalent
for s64 and explicit use of __aligned(8) is common in
the kernel so we adopt this here.

Note that there were about 8 drivers that would have been broken with
v1 of the patch.  I have also forced alignment of timestamps in cases
where (mostly by coincidence) we would have been fine (padding was
less than 4 bytes anyway.  I did this partly to reduce fragility if
other elements are added in future and also to avoid cut and paste
errors in new drivers.

There were a few other minor tidying up changes inline with reviews
of v1.

I've kept tags given for v1 on basis the changes are minor. Shout if
you disagree.

Version 1 part 1 cover letter.

Lars noted in a recent review [1] of the adis16475 that we had an issue around
the alignment requirements of iio_push_to_buffers_with_timestamp.
Whilst it's not documented, that function assumes that the overall buffer
is 8 byte aligned, to ensure the timestamp is itself naturally aligned.
We have drivers that use arrays (typically on the stack) that do
not guarantee this alignment.

We could have fixed this by using a put_unaligned to write the timestamp
but I think that just pushes the problem down the line.  If we were to
have a consumer buffer wanting all the channels in the current
active_scanmask then it will get the raw buffer from the driver passed
straight through.  It seems odd to me if we allow passing a buffer
that is not naturally aligned through to a consumer.
Hence I'm proposing to fix up all existing drivers that might pass
a buffer with insufficient alignment guarantees.
Sometimes the timestamp is guaranteed to be in a particular location,
in which case we can use C structure alignment guarantees to fix this
in a nice readable fashion.  In other cases, the timestamp location
depends on which channels are enabled, and in those case we can
use explicit alignment __aligned(8) to ensure the whole array is
appropriately aligned.

Lars-Peter also noted that, in many of these cases, there are holes
in the stack array that we never write.  Those provide a potential
leak of kernel data to userspace.  For drivers where this applies
we either need to zero those holes each time, or allocate the buffer
on the heap (only once), ensuring it is zeroed at that time.
We may leak previous values from the sensor but currently that seems
unlikely to present any form of security risk.

As such, this first set contains a mixture of fixes.  Where there
are no possible holes, the buffer is kept on the stack but a
c structure is used to guarantee appropriate alignment.  Where
there are holes, the buffer is moved into the iio_priv() accessed
data private structure. A c structure or __aligned(8) is used
as appropriate.

I've stopped at this point rather than doing all the drivers Lars
found in order to both throttle the review burden and also to
see find any general problems with the fixes before doign futher
similar series.  A few of the remaining ones will be rather more
complex to deal with.

These have been there a long time, so whilst they are fixes we
will want in stable I'm not that bothered if it takes us a little
while to get them there!

[1] https://www.spinics.net/lists/devicetree/msg350590.html
[2] https://patchwork.kernel.org/cover/11554215/

Jonathan Cameron (8):
  iio:light:rpr0521: Fix timestamp alignment and prevent data leak.
  iio:light:st_uvis25: Fix timestamp alignment and prevent data leak.
  iio:magnetometer:mag3110: Fix alignment and data leak issues.
  iio:imu:bmi160: Fix too large a buffer.
  iio:imu:bmi160: Fix alignment and data leak issues
  iio:pressure:mpl3115: Force alignment of buffer
  iio:adc:ti-ads124s08: Fix buffer being too long.
  iio:adc:ti-ads124s08: Fix alignment and data leak issues.

 drivers/iio/adc/ti-ads124s08.c       | 13 ++++++++++---
 drivers/iio/imu/bmi160/bmi160.h      |  7 +++++++
 drivers/iio/imu/bmi160/bmi160_core.c |  6 ++----
 drivers/iio/light/rpr0521.c          | 17 +++++++++++++----
 drivers/iio/light/st_uvis25.h        |  5 +++++
 drivers/iio/light/st_uvis25_core.c   |  8 +++++---
 drivers/iio/magnetometer/mag3110.c   | 13 +++++++++----
 drivers/iio/pressure/mpl3115.c       |  9 ++++++++-
 8 files changed, 59 insertions(+), 19 deletions(-)

-- 
2.28.0

^ permalink raw reply	[flat|nested] 14+ messages in thread