IIO: Fused set 1 and 2 of timestamp alignment fixes
Jonathan Cameron
maintainerv4 cover IIO: Fused set 1 and 2 of timestamp alignment fixes
From: Jonathan Cameron <[email protected]>
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(-)
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerPATCH v4 1/8 iio:light:rpr0521: Fix timestamp alignment and prevent data leak.
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review. iio_push_to_buffers_with_timestamp() assumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses an array of smaller elements on the stack. As Lars also noted this anti pattern can involve a leak of data to userspace and that indeed can happen here. We close both issues by moving to a suitable structure in the iio_priv().
This data is allocated with kzalloc() so no data can leak apart from previous readings and in this case the status byte from the device.
The forced alignment of ts is not necessary in this case but it potentially makes the code less fragile.
From personal communications with Mikko:
We could probably split the reading of the int register, but it would mean a significant performance cost of 20 i2c clock cycles.
---
v4: Fixed typo. Also added to patch description Mikko's information
drivers/iio/light/rpr0521.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/iio/light/rpr0521.c +13 -4
@@ -194,6 +194,17 @@ struct rpr0521_data {bool pxs_need_dis;struct regmap *regmap; + +/* +* Ensure correct naturally aligned timestamp. +* Note that the read will put garbage data into +* the padding but this should not be a problem +*/ +struct { +__le16 channels[3 ]; +u8 garbage; +s64 ts__aligned(8) ; + } scan; }; static IIO_CONST_ATTR(in_intensity_scale_available, RPR0521_ALS_SCALE_AVAIL);
@@ -449,8 +460,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)struct rpr0521_data *data =iio_priv (indio_dev);int err; -u8 buffer[16 ];/* 3 16-bit channels + padding + ts */ -/* Use irq timestamp when reasonable. */ if (iio_trigger_using_own (indio_dev) && data->irq_timestamp) { pf->timestamp = data->irq_timestamp;
@@ -461,11 +470,11 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p) pf->timestamp =iio_get_time_ns (indio_dev); err =regmap_bulk_read (data->regmap,RPR0521_REG_PXS_DATA , - &buffer, + data->scan.channels, (3 *2 ) +1 );/* 3 * 16-bit + (discarded) int clear reg. */ if (!err)iio_push_to_buffers_with_timestamp (indio_dev, - buffer, pf->timestamp); + &data->scan, pf->timestamp);else dev_err (&data->client->dev,"Trigger consumer can't read from sensor.\n" );
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerPATCH v4 2/8 iio:light:st_uvis25: Fix timestamp alignment and prevent data leak.
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review. iio_push_to_buffers_with_timestamp() assumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses an array of smaller elements on the stack. As Lars also noted this anti pattern can involve a leak of data to userspace and that indeed can happen here. We close both issues by moving to a suitable structure in the iio_priv()
This data is allocated with kzalloc() so no data can leak apart from previous readings.
A local unsigned int variable is used for the regmap call so it is clear there is no potential issue with writing into the padding of the structure.
---
v4: Drop the pointless masking.
drivers/iio/light/st_uvis25.h | 5 +++++ drivers/iio/light/st_uvis25_core.c | 8 +++++--- 2 files changed, 10 insertions(+), 3 deletions(-)
drivers/iio/light/st_uvis25.h +5 -0
@@ -27,6 +27,11 @@ struct st_uvis25_hw {struct iio_trigger *trig;bool enabled;int irq; +/* Ensure timestamp is naturally aligned */ +struct { +u8 chan; +s64 ts__aligned(8) ; + } scan; };extern const struct dev_pm_ops st_uvis25_pm_ops;
drivers/iio/light/st_uvis25_core.c +5 -3
@@ -232,17 +232,19 @@ static const struct iio_buffer_setup_ops st_uvis25_buffer_ops = {static irqreturn_t st_uvis25_buffer_handler_thread (int irq,void *p) { -u8 buffer[ALIGN (sizeof (u8),sizeof (s64)) +sizeof (s64)];struct iio_poll_func *pf = p;struct iio_dev *iio_dev = pf->indio_dev;struct st_uvis25_hw *hw =iio_priv (iio_dev); +unsigned int val;int err; - err =regmap_read (hw->regmap,ST_UVIS25_REG_OUT_ADDR , (int *)buffer); + err =regmap_read (hw->regmap,ST_UVIS25_REG_OUT_ADDR , &val);if (err <0 ) goto out; -iio_push_to_buffers_with_timestamp (iio_dev, buffer, + hw->scan.chan = val; + +iio_push_to_buffers_with_timestamp (iio_dev, &hw->scan,iio_get_time_ns (iio_dev)); out:
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerPATCH v4 3/8 iio:magnetometer:mag3110: Fix alignment and data leak issues.
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review. iio_push_to_buffers_with_timestamp() assumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses an array of smaller elements on the stack. As Lars also noted this anti pattern can involve a leak of data to userspace and that indeed can happen here. We close both issues by moving to a suitable structure in the iio_priv() data.
This data is allocated with kzalloc() so no data can leak apart from previous readings.
The explicit alignment of ts is not necessary in this case but does make the code slightly less fragile so I have included it.
---
v4: Rename element to temperature to avoid confusion.
drivers/iio/magnetometer/mag3110.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/iio/magnetometer/mag3110.c +9 -4
@@ -56,6 +56,12 @@ struct mag3110_data {int sleep_val;struct regulator *vdd_reg;struct regulator *vddio_reg; +/* Ensure natural alignment of timestamp */ +struct { +__be16 channels[3 ]; +u8 temperature; +s64 ts__aligned(8) ; + } scan; };static int mag3110_request (struct mag3110_data *data)
@@ -387,10 +393,9 @@ static irqreturn_t mag3110_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct mag3110_data *data =iio_priv (indio_dev); -u8 buffer[16 ];/* 3 16-bit channels + 1 byte temp + padding + ts */ int ret; - ret =mag3110_read (data, (__be16 *) buffer); + ret =mag3110_read (data, data->scan.channels);if (ret <0 ) goto done;
@@ -399,10 +404,10 @@ static irqreturn_t mag3110_trigger_handler(int irq, void *p) MAG3110_DIE_TEMP);if (ret <0 ) goto done; - buffer[6 ] = ret; + data->scan.temperature = ret; } -iio_push_to_buffers_with_timestamp (indio_dev, buffer, +iio_push_to_buffers_with_timestamp (indio_dev, &data->scan,iio_get_time_ns (indio_dev)); done:
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerPATCH v4 4/8 iio:imu:bmi160: Fix too large a buffer.
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
The comment implies this device has 3 sensor types, but it only has an accelerometer and a gyroscope (both 3D). As such the buffer does not need to be as long as stated.
Note I've separated this from the following patch which fixes the alignment for passing to iio_push_to_buffers_with_timestamp() as they are different issues even if they affect the same line of code.
---
v4: New patch
drivers/iio/imu/bmi160/bmi160_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/iio/imu/bmi160/bmi160_core.c +2 -2
@@ -427,8 +427,8 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct bmi160_data *data =iio_priv (indio_dev); -__le16 buf[16 ]; -/* 3 sens x 3 axis x __le16 + 3 x __le16 pad + 4 x __le16 tstamp */ +__le16 buf[12 ]; +/* 2 sens x 3 axis x __le16 + 2 x __le16 pad + 4 x __le16 tstamp */ int i, ret, j =0 , base =BMI160_REG_DATA_MAGN_XOUT_L ;__le16 sample;
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerPATCH v4 5/8 iio:imu:bmi160: Fix alignment and data leak issues
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review. iio_push_to_buffers_with_timestamp assumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses an array of smaller elements on the stack. As Lars also noted this anti pattern can involve a leak of data to userspace and that indeed can happen here. We close both issues by moving to a suitable array in the iio_priv() data with alignment explicitly requested. This data is allocated with kzalloc() so no data can leak apart from previous readings.
In this driver, depending on which channels are enabled, the timestamp can be in a number of locations. Hence we cannot use a structure to specify the data layout without it being misleading.
---
v4: Improve comments and carry forwards shorter length.
drivers/iio/imu/bmi160/bmi160.h | 7 +++++++ drivers/iio/imu/bmi160/bmi160_core.c | 6 ++---- 2 files changed, 9 insertions(+), 4 deletions(-)
drivers/iio/imu/bmi160/bmi160.h +7 -0
@@ -10,6 +10,13 @@ struct bmi160_data {struct iio_trigger *trig;struct regulator_bulk_data supplies[2 ];struct iio_mount_matrix orientation; +/* +* Ensure natural alignment for timestamp if present. +* Max length needed: 2 * 3 channels + 4 bytes padding + 8 byte ts. +* If fewer channels are enabled, less space may be needed, as +* long as the timestamp is still aligned to 8 bytes. +*/ +__le16 buf[12 ]__aligned(8) ; };extern const struct regmap_config bmi160_regmap_config;
drivers/iio/imu/bmi160/bmi160_core.c +2 -4
@@ -427,8 +427,6 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct bmi160_data *data =iio_priv (indio_dev); -__le16 buf[12 ]; -/* 2 sens x 3 axis x __le16 + 2 x __le16 pad + 4 x __le16 tstamp */ int i, ret, j =0 , base =BMI160_REG_DATA_MAGN_XOUT_L ;__le16 sample;
@@ -438,10 +436,10 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p) &sample, sizeof(sample));if (ret) goto done; - buf[j++] = sample; + data->buf[j++] = sample; } -iio_push_to_buffers_with_timestamp (indio_dev, buf, pf->timestamp); +iio_push_to_buffers_with_timestamp (indio_dev, data->buf, pf->timestamp); done:iio_trigger_notify_done (indio_dev->trig);return IRQ_HANDLED ;
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerPATCH v4 6/8 iio:pressure:mpl3115: Force alignment of buffer
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
Whilst this is another case of the issue Lars reported with an array of elements of smaller than 8 bytes being passed to iio_push_to_buffers_with_timestamp(), the solution here is a bit different from the other cases and relies on __aligned working on the stack (true since 4.6?)
This one is unusual. We have to do an explicit memset() each time as we are reading 3 bytes into a potential 4 byte channel which may sometimes be a 2 byte channel depending on what is enabled. As such, moving the buffer to the heap in the iio_priv structure doesn't save us much. We can't use a nice explicit structure on the stack either as the data channels have different storage sizes and are all separately controlled.
---
v4: Added a 'special' comment as this one is unique.
drivers/iio/pressure/mpl3115.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/iio/pressure/mpl3115.c +8 -1
@@ -144,7 +144,14 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct mpl3115_data *data =iio_priv (indio_dev); -u8 buffer[16 ];/* 32-bit channel + 16-bit channel + padding + ts */ +/* +* 32-bit channel + 16-bit channel + padding + ts +* Note that it is possible for only one of the first 2 +* channels to be enabled. If that happens, the first element +* of the buffer may be either 16 or 32-bits. As such we cannot +* use a simple structure definition to express this data layout. +*/ +u8 buffer[16 ]__aligned(8) ;int ret, pos =0 ;mutex_lock (&data->lock);
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerPATCH v4 7/8 iio:adc:ti-ads124s08: Fix buffer being too long.
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
The buffer is expressed as a u32 array, yet the extra space for the s64 timestamp was expressed as sizeof(s64)/sizeof(u16). This will result in 2 extra u32 elements.
Fix by dividing by sizeof(u32).
---
v4: New patch
drivers/iio/adc/ti-ads124s08.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/adc/ti-ads124s08.c +1 -1
@@ -269,7 +269,7 @@ static irqreturn_t ads124s_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct ads124s_private *priv =iio_priv (indio_dev); -u32 buffer[ADS124S08_MAX_CHANNELS +sizeof (s64)/sizeof (u16)]; +u32 buffer[ADS124S08_MAX_CHANNELS +sizeof (s64)/sizeof (u32)];int scan_index, j =0 ;int ret;
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerPATCH v4 8/8 iio:adc:ti-ads124s08: Fix alignment and data leak issues.
In reply to Jonathan CameronFrom: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review. iio_push_to_buffers_with_timestamp() assumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses an array of smaller elements on the stack. As Lars also noted this anti pattern can involve a leak of data to userspace and that indeed can happen here. We close both issues by moving to a suitable structure in the iio_priv() data with alignment explicitly requested. This data is allocated with kzalloc() so no data can leak apart from previous readings.
In this driver the timestamp can end up in various different locations depending on what other channels are enabled. As a result, we don't use a structure to specify it's position as that would be misleading.
---
v4: Expand comment to express the buffer length not all needed if
not all channels are enabled.
drivers/iio/adc/ti-ads124s08.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
drivers/iio/adc/ti-ads124s08.c +10 -3
@@ -99,6 +99,14 @@ struct ads124s_private {struct gpio_desc *reset_gpio;struct spi_device *spi;struct mutex lock; +/* +* Used to correctly align data. +* Ensure timestamp is naturally aligned. +* Note that the full buffer length may not be needed if not +* all channels are enabled, as long as the alignment of the +* timestamp is maintained. +*/ +u32 buffer[ADS124S08_MAX_CHANNELS +sizeof (s64)/sizeof (u32)]__aligned(8) ;u8 data[5 ]____cacheline_aligned ; };
@@ -269,7 +277,6 @@ static irqreturn_t ads124s_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct ads124s_private *priv =iio_priv (indio_dev); -u32 buffer[ADS124S08_MAX_CHANNELS +sizeof (s64)/sizeof (u32)];int scan_index, j =0 ;int ret;
@@ -284,7 +291,7 @@ static irqreturn_t ads124s_trigger_handler(int irq, void *p)if (ret)dev_err (&priv->spi->dev,"Start ADC conversions failed\n" ); - buffer[j] =ads124s_read (indio_dev, scan_index); + priv->buffer[j] =ads124s_read (indio_dev, scan_index); ret =ads124s_write_cmd (indio_dev,ADS124S08_STOP_CONV );if (ret)dev_err (&priv->spi->dev,"Stop ADC conversions failed\n" );
@@ -292,7 +299,7 @@ static irqreturn_t ads124s_trigger_handler(int irq, void *p) j++; } -iio_push_to_buffers_with_timestamp (indio_dev, buffer, +iio_push_to_buffers_with_timestamp (indio_dev, priv->buffer, pf->timestamp);iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
2.28.0
Jonathan Cameron
maintainerDear 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 :)
Quoted 167 lines · On Sun, 20 Sep 2020 12:27:34 +0100 Jonathan Cameron <[email protected]> wrote:
From: Jonathan Cameron <[email protected]>
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: Usesizeof()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 aregmap_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_unalignedto 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 theiio_push_to_buffers_with_timestampcall) 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_32s64 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 casesiio_push_to_buffers_with_tsis 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_alignedbut 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_unalignedto 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 currentactive_scanmaskthen 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(-)
Quoted 9 lines · Jonathan Cameron
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!
This looks good AFAICT.
But it also looks like it could do with some re-design/re-thinking. Maybe somehow moving more of this buffer + timestamp management somewhere in IIO core.
It would change the paradigm a bit, in the sense that a driver would ask IIO core for a buffer/scratchpad area, where to put the data read from the device.
This buffer pool management could be interesting [in the long run]. Maybe with some zero-copy mechanism.
Quoted 178 lines
Thanks
Jonathan
+CC a few additional helpful souls :)
Quoted 167 lines · On Sun, 20 Sep 2020 12:27:34 +0100 Jonathan Cameron <[email protected]> wrote:
From: Jonathan Cameron <[email protected]>
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: Usesizeof()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 aregmap_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_unalignedto 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 theiio_push_to_buffers_with_timestampcall) 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_32s64 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 casesiio_push_to_buffers_with_tsis 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_alignedbut 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_unalignedto 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 currentactive_scanmaskthen 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(-)
Jonathan Cameron
maintainerQuoted 4 lines · On Sun, 29 Nov 2020 18:15:28 +0200 Alexandru Ardelean <[email protected]>…
Quoted 3 lines · Jonathan Cameron
Dear All,
Quoted 8 lines
Quoted 6 lines
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!
This looks good AFAICT.
Thanks. If you are comfortable giving a tag that would be great. If not, no problem!
Quoted 9 lines
But it also looks like it could do with some re-design/re-thinking. Maybe somehow moving more of this buffer + timestamp management somewhere in IIO core.
It would change the paradigm a bit, in the sense that a driver would ask IIO core for a buffer/scratchpad area, where to put the data read from the device.
This buffer pool management could be interesting [in the long run]. Maybe with some zero-copy mechanism.
Whilst an interesting idea, it would run into various challenges.
1) Need to be DMA safe for SPI drivers, or they would still need to have bounce buffers. Current cases where we have to copy twice are a bit annoying of course.
2) Need to show sufficient performance benefit to be worth the churn to make the change + the potential complexity.
3) That interface isn't necessarily just going to one place as there may be multiple consumers. There are probably still ways that could be dealt with but it's another level of complexity.
So perhaps worth exploring but the performance vs complexity question is where I suspect it would come unstuck.
Thanks,
Jonathan
Quoted 179 lines
Quoted 179 lines
Thanks
Jonathan
+CC a few additional helpful souls :)
Quoted 167 lines · On Sun, 20 Sep 2020 12:27:34 +0100 Jonathan Cameron <[email protected]> wrote:
From: Jonathan Cameron <[email protected]>
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: Usesizeof()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 aregmap_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_unalignedto 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 theiio_push_to_buffers_with_timestampcall) 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_32s64 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 casesiio_push_to_buffers_with_tsis 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_alignedbut 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_unalignedto 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 currentactive_scanmaskthen 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(-)
Alexandru Ardelean
Reviewed-byQuoted 12 lines · Jonathan Cameron
Quoted 8 lines · On Sun, 29 Nov 2020 18:15:28 +0200 Alexandru Ardelean <[email protected]>…
Quoted 7 lines · Jonathan Cameron
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.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
As such, if anyone has a chance to do a quick sanity check that would be much appreciated!
This looks good AFAICT.
Thanks. If you are comfortable giving a tag that would be great. If not, no problem!
Sure. Apologies for not adding one sooner.
Quoted 21 lines
Quoted 10 lines
But it also looks like it could do with some re-design/re-thinking. Maybe somehow moving more of this buffer + timestamp management somewhere in IIO core.
It would change the paradigm a bit, in the sense that a driver would ask IIO core for a buffer/scratchpad area, where to put the data read from the device.
This buffer pool management could be interesting [in the long run]. Maybe with some zero-copy mechanism.Whilst an interesting idea, it would run into various challenges.
1) Need to be DMA safe for SPI drivers, or they would still need to have bounce buffers. Current cases where we have to copy twice are a bit annoying of course.
2) Need to show sufficient performance benefit to be worth the churn to make the change + the potential complexity.
3) That interface isn't necessarily just going to one place as there may be multiple consumers. There are probably still ways that could be dealt with but it's another level of complexity.
Quoted 2 lines
So perhaps worth exploring but the performance vs complexity question is where I suspect it would come unstuck.
I still feel there may be some reasonably simple mechanisms, that would benefit drivers that use iio_push_to_buffers_with_timestamp(). I don't have anything clear in mind yet.
I still need to dig through other backlog stuff.
But, I'll try to make a note of this and see later if I can get to it.
Quoted 185 lines
Thanks,
Jonathan
Quoted 179 lines
Quoted 179 lines
Thanks
Jonathan
+CC a few additional helpful souls :)
Quoted 167 lines · On Sun, 20 Sep 2020 12:27:34 +0100 Jonathan Cameron <[email protected]> wrote:
From: Jonathan Cameron <[email protected]>
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: Usesizeof()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 aregmap_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_unalignedto 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 theiio_push_to_buffers_with_timestampcall) 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_32s64 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 casesiio_push_to_buffers_with_tsis 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_alignedbut 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_unalignedto 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 currentactive_scanmaskthen 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(-)
Quoted 16 lines · On Sun, 29 Nov 2020 22:26:46 +0200 Alexandru Ardelean <[email protected]>…
Quoted 15 lines · Jonathan Cameron
Quoted 11 lines · On Sun, 29 Nov 2020 18:15:28 +0200 Alexandru Ardelean <[email protected]>…
Quoted 9 lines · Jonathan Cameron
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!
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
This looks good AFAICT.
Thanks. If you are comfortable giving a tag that would be great. If not, no problem!
Sure. Apologies for not adding one sooner.
Reviewed-by Alexandru Ardelean <[email protected]>
Thanks, Applied to the togreg branch of iio.git and marked for stable. Didn't send these the faster route as we are near the next merge window anyway and these have been there for a very long time.
Quoted 21 lines
Quoted 21 lines
Quoted 10 lines
But it also looks like it could do with some re-design/re-thinking. Maybe somehow moving more of this buffer + timestamp management somewhere in IIO core.
It would change the paradigm a bit, in the sense that a driver would ask IIO core for a buffer/scratchpad area, where to put the data read from the device.
This buffer pool management could be interesting [in the long run]. Maybe with some zero-copy mechanism.Whilst an interesting idea, it would run into various challenges.
1) Need to be DMA safe for SPI drivers, or they would still need to have bounce buffers. Current cases where we have to copy twice are a bit annoying of course.
2) Need to show sufficient performance benefit to be worth the churn to make the change + the potential complexity.
3) That interface isn't necessarily just going to one place as there may be multiple consumers. There are probably still ways that could be dealt with but it's another level of complexity.
Quoted 8 lines
Quoted 2 lines
So perhaps worth exploring but the performance vs complexity question is where I suspect it would come unstuck.
I still feel there may be some reasonably simple mechanisms, that would benefit drivers that use
iio_push_to_buffers_with_timestamp(). I don't have anything clear in mind yet.
I still need to dig through other backlog stuff.
But, I'll try to make a note of this and see later if I can get to it.
Cool. There might be stuff that is worth doing in this area. Will see what you come up with!
Jonathan
Quoted 186 lines
Quoted 186 lines
Thanks,
Jonathan
Quoted 179 lines
Quoted 179 lines
Thanks
Jonathan
+CC a few additional helpful souls :)
Quoted 167 lines · On Sun, 20 Sep 2020 12:27:34 +0100 Jonathan Cameron <[email protected]> wrote:
From: Jonathan Cameron <[email protected]>
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: Usesizeof()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 aregmap_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_unalignedto 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 theiio_push_to_buffers_with_timestampcall) 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_32s64 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 casesiio_push_to_buffers_with_tsis 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_alignedbut 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_unalignedto 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 currentactive_scanmaskthen 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(-)