Dear All, Whilst I'm reasonably confident this series is correct (famous last words) I don't like applying anything non trivial without having had at least one set of additional eyes on it. As such, if anyone has a chance to do a quick sanity check that would be much appreciated! Thanks Jonathan +CC a few additional helpful souls :) On Sun, 20 Sep 2020 12:27:34 +0100 Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > 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(-) >
