IIO: 1st set of timestamp alignment fixes.
Subsystems (4)
[email protected]
maintainercover IIO: 1st set of timestamp alignment fixes.
From: Jonathan Cameron <[email protected]>
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
Jonathan Cameron (11):
iio: accel: kxsd9: Fix alignment of local buffer. iio:accel:mma8452: Fix timestamp alignment and prevent data leak. iio:accel:bmc150-accel: Fix timestamp alignment and prevent data leak. iio:accel:mma7455: Fix timestamp alignment and prevent data leak. iio:gyro:itg3200: Fix timestamp alignment and prevent data leak. iio:proximity:mb1232: Fix timestamp alignment and prevent data leak. iio:health:afe4403 Fix timestamp alignment and prevent data leak. iio:health:afe4404 Fix timestamp alignment and prevent data leak. iio:chemical:ccs811: Fix timestamp alignment and prevent data leak. iio:chemical:sps30: Fix timestamp alignment iio:chemical:pms7003: Fix timestamp alignment and prevent data leak.
drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++--- drivers/iio/accel/kxsd9.c | 16 +++++++++++----- drivers/iio/accel/mma7455_core.c | 16 ++++++++++++---- drivers/iio/accel/mma8452.c | 11 ++++++++--- drivers/iio/chemical/ccs811.c | 13 +++++++++---- drivers/iio/chemical/pms7003.c | 17 ++++++++++++----- drivers/iio/chemical/sps30.c | 9 ++++++--- drivers/iio/gyro/itg3200_buffer.c | 15 +++++++++++---- drivers/iio/health/afe4403.c | 9 ++++++--- drivers/iio/health/afe4404.c | 8 +++++--- drivers/iio/proximity/mb1232.c | 17 +++++++++-------- 11 files changed, 101 insertions(+), 45 deletions(-)
Signature · 1 line
2.26.2
[email protected]
maintainerPATCH 1/11 iio: accel: kxsd9: Fix alignment of local buffer.
From: Jonathan Cameron <[email protected]>
iio_push_to_buffers_with_timestamp assumes 8 byte alignment which is not guaranteed by an array of smaller elements.
--- drivers/iio/accel/kxsd9.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/iio/accel/kxsd9.c +11 -5
@@ -209,14 +209,20 @@ static irqreturn_t kxsd9_trigger_handler(int irq, void *p)const struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct kxsd9_state *st =iio_priv (indio_dev); +/* +* Ensure correct positioning and alignment of timestamp. +* No need to zero initialize as all elements written. +*/ +struct { +__be16 chan[4 ]; +s64 ts; + } hw_values;int ret; -/* 4 * 16bit values AND timestamp */ -__be16 hw_values[8 ]; ret =regmap_bulk_read (st->map,KXSD9_REG_X , - &hw_values, -8 ); + hw_values.chan, +sizeof (hw_values.chan));if (ret) {dev_err (st->dev,"error reading data\n" );
@@ -224,7 +230,7 @@ static irqreturn_t kxsd9_trigger_handler(int irq, void *p) }iio_push_to_buffers_with_timestamp (indio_dev, - hw_values, + &hw_values,iio_get_time_ns (indio_dev));iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
2.26.2
[email protected]
maintainerPATCH 2/11 iio:accel:mma8452: Fix timestamp alignment and prevent data leak.
In reply to [email protected]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 a 16 byte u8 array 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 ensured by use of an explicit c structure. This data is allocated with kzalloc so no data can leak appart from previous readings.
--- drivers/iio/accel/mma8452.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/iio/accel/mma8452.c +8 -3
@@ -110,6 +110,12 @@ struct mma8452_data {int sleep_val;struct regulator *vdd_reg;struct regulator *vddio_reg; + +/* Ensure correct alignment of time stamp when present */ +struct { +__be16 channels[3 ]; +s64 ts; + } buffer; };/**
@@ -1091,14 +1097,13 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct mma8452_data *data =iio_priv (indio_dev); -u8 buffer[16 ];/* 3 16-bit channels + padding + ts */ int ret; - ret =mma8452_read (data, (__be16 *)buffer); + ret =mma8452_read (data, data->buffer.channels);if (ret <0 ) goto done; -iio_push_to_buffers_with_timestamp (indio_dev, buffer, +iio_push_to_buffers_with_timestamp (indio_dev, &data->buffer,iio_get_time_ns (indio_dev)); done:
Signature · 1 line
2.26.2
Re: [PATCH 02/11] iio:accel:mma8452: Fix timestamp alignment and prevent data leak.
Quoted 25 lines · On 5/17/20 7:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 16 byte u8 array 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 theiio_priv()data with alignment ensured by use of an explicit c structure. This data is allocated with kzalloc so no data can leak appart from previous readings.Fixes c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Peter Meerwald <[email protected]>--- drivers/iio/accel/mma8452.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/iio/accel/mma8452.c +0
-0
drivers/iio/accel/mma8452.c struct mma8452_data { · line 118
Quoted 9 lines
drivers/iio/accel/mma8452.c +6
-0@@ -110,6 +110,12 @@ struct mma8452_data {int sleep_val;struct regulator *vdd_reg;struct regulator *vddio_reg; + +/* Ensure correct alignment of time stamp when present */ +struct { +__be16 channels[3 ]; +s64 ts; + } buffer;
I feel we should have a macro for this.
DECLARE_IIO_BUFFER_WITH_TIMESTAMP(buffer, __be16, 3);
The name is maybe a bit too long.
And potentially also DECLARE_IIO_BUFFER_WITH_TIMESTAMP_ON_STACK() which initializes it to zero.
Quoted 20 lines
drivers/iio/accel/mma8452.c +2
-3};/** @@ -1091,14 +1097,13 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct mma8452_data *data =iio_priv (indio_dev); -u8 buffer[16 ];/* 3 16-bit channels + padding + ts */ int ret; - ret =mma8452_read (data, (__be16 *)buffer); + ret =mma8452_read (data, data->buffer.channels);if (ret <0 ) goto done; -iio_push_to_buffers_with_timestamp (indio_dev, buffer, +iio_push_to_buffers_with_timestamp (indio_dev, &data->buffer,iio_get_time_ns (indio_dev)); done:
Re: [PATCH 02/11] iio:accel:mma8452: Fix timestamp alignment and prevent data leak.
Quoted 39 lines · On Sun, 17 May 2020 20:57:26 +0200 Lars-Peter Clausen <[email protected]> wrote:
Quoted 25 lines · On 5/17/20 7:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 16 byte u8 array 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 theiio_priv()data with alignment ensured by use of an explicit c structure. This data is allocated with kzalloc so no data can leak appart from previous readings.Fixes c7eeea93ac60 ("iio: Add Freescale MMA8452Q 3-axis accelerometer driver")Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Peter Meerwald <[email protected]>--- drivers/iio/accel/mma8452.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)drivers/iio/accel/mma8452.c +0
-0drivers/iio/accel/mma8452.c struct mma8452_data { · line 118
Quoted 9 lines
drivers/iio/accel/mma8452.c +6
-0@@ -110,6 +110,12 @@ struct mma8452_data {int sleep_val;struct regulator *vdd_reg;struct regulator *vddio_reg; + +/* Ensure correct alignment of time stamp when present */ +struct { +__be16 channels[3 ]; +s64 ts; + } buffer;I feel we should have a macro for this.
Quoted 3 lines
DECLARE_IIO_BUFFER_WITH_TIMESTAMP(buffer, __be16, 3);The name is maybe a bit too long.
It runs into the issue we had with the afe4403 and other devices that are happy to do very different numbers of enabled channels. Such a macro would imply that the timestamp will always be written at a fixed location, which isn't true. So far, I've made sure we didn't do anything like this unless the number of channels was small enough there was only one possible location for the timestamp (now we insist that at least one channel is enabled to start a buffer).
Maybe we just deal with that with some suitable documentation though.
Quoted 3 lines
And potentially also
DECLARE_IIO_BUFFER_WITH_TIMESTAMP_ON_STACK()which initializes it to zero.
The thing there is we should only need to initialize it to zero if there are holes. If we always write data up to the 8 byte boundary it won't matter if the timestamp is disabled or not, I don't think we will get a data leak. We can't even do some magic in the call to identify if there are potential holes because we can't tell if all the channels will be written or not...
I'm not totally convinced hiding what is going on behind a macro is a good idea. Sometimes bashing people over the head with the fact there are some non obvious requirements is a good idea.
Not sure...
J
Quoted 23 lines
Quoted 21 lines
drivers/iio/accel/mma8452.c +2
-3};/** @@ -1091,14 +1097,13 @@ static irqreturn_t mma8452_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct mma8452_data *data =iio_priv (indio_dev); -u8 buffer[16 ];/* 3 16-bit channels + padding + ts */ int ret; - ret =mma8452_read (data, (__be16 *)buffer); + ret =mma8452_read (data, data->buffer.channels);if (ret <0 ) goto done; -iio_push_to_buffers_with_timestamp (indio_dev, buffer, +iio_push_to_buffers_with_timestamp (indio_dev, &data->buffer,iio_get_time_ns (indio_dev)); done:
[email protected]
maintainerPATCH 3/11 iio:accel:bmc150-accel: Fix timestamp alignment and prevent data leak.
In reply to [email protected]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 a 16 byte 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 ensured by use of an explicit c structure. This data is allocated with kzalloc so no data can leak appart from previous readings.
Fixes tag is beyond some major refactoring so likely manual backporting would be needed to get that far back.
--- drivers/iio/accel/bmc150-accel-core.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/iio/accel/bmc150-accel-core.c +12 -3
@@ -189,6 +189,14 @@ struct bmc150_accel_data {struct mutex mutex;u8 fifo_mode, watermark;s16 buffer[8 ]; +/* +* Ensure there is sufficient space and correct alignment for +* the timestamp if enabled +*/ +struct { +__le16 channels[3 ]; +s64 ts; + } scan;u8 bw_bits;u32 slope_dur;u32 slope_thres;
@@ -922,15 +930,16 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,* now. */ for (i =0 ; i < count; i++) { -u16 sample[8 ];int j, bit; j =0 ;for_each_set_bit (bit, indio_dev->active_scan_mask, indio_dev->masklength) -memcpy (&sample[j++], &buffer[i *3 + bit],2 ); +memcpy (&data->scan.channels[j++], &buffer[i *3 + bit], +2 ); -iio_push_to_buffers_with_timestamp (indio_dev, sample, tstamp); +iio_push_to_buffers_with_timestamp (indio_dev, &data->scan, + tstamp); tstamp += sample_period; }
Signature · 1 line
2.26.2
[email protected]
maintainerPATCH 4/11 iio:accel:mma7455: Fix timestamp alignment and prevent data leak.
In reply to [email protected]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 a 16 byte u8 array 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 ensured by use of an explicit c structure. This data is allocated with kzalloc so no data can leak appart from previous readings.
--- drivers/iio/accel/mma7455_core.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/iio/accel/mma7455_core.c +12 -4
@@ -52,6 +52,14 @@struct mma7455_data {struct regmap *regmap; +/* +* Used to reorganize data. Will ensure correct alignment of +* the timestamp if present +*/ +struct { +__le16 channels[3 ]; +s64 ts; + } scan; };static int mma7455_drdy (struct mma7455_data *mma7455)
@@ -82,19 +90,19 @@ static irqreturn_t mma7455_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct mma7455_data *mma7455 =iio_priv (indio_dev); -u8 buf[16 ];/* 3 x 16-bit channels + padding + ts */ int ret; ret =mma7455_drdy (mma7455);if (ret) goto done; - ret =regmap_bulk_read (mma7455->regmap,MMA7455_REG_XOUTL , buf, -sizeof (__le16) *3 ); + ret =regmap_bulk_read (mma7455->regmap,MMA7455_REG_XOUTL , + mma7455->scan.channels, +sizeof (mma7455->scan.channels));if (ret) goto done; -iio_push_to_buffers_with_timestamp (indio_dev, buf, +iio_push_to_buffers_with_timestamp (indio_dev, &mma7455->scan,iio_get_time_ns (indio_dev)); done:
Signature · 1 line
2.26.2
[email protected]
maintainerPATCH 5/11 iio:gyro:itg3200: Fix timestamp alignment and prevent data leak.
In reply to [email protected]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 a 16 byte array of smaller elements on the stack. This is fixed by using an explicit c structure. As there are no holes in the structure, there is no possiblity of data leakage in this case.
--- drivers/iio/gyro/itg3200_buffer.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/iio/gyro/itg3200_buffer.c +11 -4
@@ -46,13 +46,20 @@ static irqreturn_t itg3200_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct itg3200 *st =iio_priv (indio_dev); -__be16 buf[ITG3200_SCAN_ELEMENTS +sizeof (s64)/sizeof (u16)]; - -int ret =itg3200_read_all_channels (st->i2c, buf); +/* +* Ensure correct alignment and padding including for the +* timestamp that may be inserted. +*/ +struct { +__be16 buf[ITG3200_SCAN_ELEMENTS ]; +s64 ts; + } scan; + +int ret =itg3200_read_all_channels (st->i2c, scan.buf);if (ret <0 ) goto error_ret; -iio_push_to_buffers_with_timestamp (indio_dev, buf, pf->timestamp); +iio_push_to_buffers_with_timestamp (indio_dev, &scan, pf->timestamp);iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
2.26.2
[email protected]
maintainerPATCH 6/11 iio:proximity:mb1232: Fix timestamp alignment and prevent data leak.
In reply to [email protected]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 a 16 byte s16 array 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 ensured by use of an explicit c structure. This data is allocated with kzalloc so no data can leak appart from previous readings.
--- drivers/iio/proximity/mb1232.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/iio/proximity/mb1232.c +9 -8
@@ -40,6 +40,11 @@ struct mb1232_data {*/ struct completion ranging;int irqnr; +/* Ensure correct alignment of data to push to IIO buffer */ +struct { +s16 distance; +s64 ts; + } scan; };static irqreturn_t mb1232_handle_irq (int irq,void *dev_id)
@@ -113,17 +118,13 @@ static irqreturn_t mb1232_trigger_handler(int irq, void *p)struct iio_poll_func *pf = p;struct iio_dev *indio_dev = pf->indio_dev;struct mb1232_data *data =iio_priv (indio_dev); -/* -* triggered buffer -* 16-bit channel + 48-bit padding + 64-bit timestamp -*/ -s16 buffer[8 ] = {0 }; - buffer[0 ] =mb1232_read_distance (data); -if (buffer[0 ] <0 ) + data->scan.distance =mb1232_read_distance (data); +if (data->scan.distance <0 ) goto err; -iio_push_to_buffers_with_timestamp (indio_dev, buffer, pf->timestamp); +iio_push_to_buffers_with_timestamp (indio_dev, &data->scan, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
2.26.2
[email protected]
maintainerPATCH 7/11 iio:health:afe4403 Fix timestamp alignment and prevent data leak.
In reply to [email protected]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 a 32 byte 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 appart from previous readings.
--- drivers/iio/health/afe4403.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/iio/health/afe4403.c +6 -3
@@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[] = {* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct data layout to push into IIO buffer. */ struct afe4403_data {struct device *dev;
@@ -74,6 +75,8 @@ struct afe4403_data {struct regulator *regulator;struct iio_trigger *trig;int irq; +/* Ensure suitable alignment for timestamp */ +s32 buffer[8 ]__aligned(8) ; };enum afe4403_chan_id {
@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)struct iio_dev *indio_dev = pf->indio_dev;struct afe4403_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[8 ];u8 tx[4 ] = {AFE440X_CONTROL0 ,0x0 ,0x0 ,AFE440X_CONTROL0_READ };u8 rx[3 ];
@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)if (ret) goto err; - buffer[i++] =get_unaligned_be24 (&rx[0 ]); + afe->buffer[i++] =get_unaligned_be24 (&rx[0 ]); }/* Disable reading from the device */
@@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)if (ret) goto err; -iio_push_to_buffers_with_timestamp (indio_dev, buffer, pf->timestamp); +iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
2.26.2
Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.
Quoted 33 lines · On 5/17/20 1:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 32 byte 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 theiio_priv()data with alignment explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.Fixes eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403 heart monitor")Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Andrew F. Davis <[email protected]>--- drivers/iio/health/afe4403.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/iio/health/afe4403.c +1
-0@@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[] = {* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct data layout to push into IIO buffer. */ struct afe4403_data {struct device *dev;
drivers/iio/health/afe4403.c struct afe4403_data { · line 79
Quoted 5 lines
drivers/iio/health/afe4403.c +2
-0@@ -74,6 +75,8 @@ struct afe4403_data {struct regulator *regulator;struct iio_trigger *trig;int irq; +/* Ensure suitable alignment for timestamp */ +s32 buffer[8 ]__aligned(8) ;
One of those fancy structs with the timestamp specified would be nice here like the other patches. IIRC we have 6 s32 channels, plus a s64 ts.
Other than that everything looks good.
Andrew
Quoted 31 lines
drivers/iio/health/afe4403.c +3
-3};enum afe4403_chan_id {@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)struct iio_dev *indio_dev = pf->indio_dev;struct afe4403_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[8 ];u8 tx[4 ] = {AFE440X_CONTROL0 ,0x0 ,0x0 ,AFE440X_CONTROL0_READ };u8 rx[3 ];@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)if (ret) goto err; - buffer[i++] =get_unaligned_be24 (&rx[0 ]); + afe->buffer[i++] =get_unaligned_be24 (&rx[0 ]); }/* Disable reading from the device */ @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int irq, void *private)if (ret) goto err; -iio_push_to_buffers_with_timestamp (indio_dev, buffer, pf->timestamp); +iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.
Quoted 40 lines · Andrew F. Davis
Quoted 6 lines · On 5/17/20 1:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 32 byte array of smaller elements on thestack.
Quoted 7 lines
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 appart from previous readings.Fixes eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403heart monitor")
Review
Quoted 8 lines
Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Andrew F. Davis <[email protected]>--- drivers/iio/health/afe4403.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/iio/health/afe4403.cdrivers/iio/health/afe4403.c static const struct reg_field afe4403_reg_fields[]
Quoted 4 lines
index
e9f87e42ff4f..a3507624b30f100644drivers/iio/health/afe4403.c +0
-0@@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[]= {
Quoted 10 lines
drivers/iio/health/afe4403.c +1
-0* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct data layout to push into IIO buffer. */ struct afe4403_data {struct device *dev;@@ -74,6 +75,8 @@ struct afe4403_data {struct regulator *regulator;struct iio_trigger *trig;
Quoted 8 lines
drivers/iio/health/afe4403.c struct afe4403_data { · line 79
Quoted 3 lines
drivers/iio/health/afe4403.c +2
-0int irq; +/* Ensure suitable alignment for timestamp */ +s32 buffer[8 ]__aligned(8) ;One of those fancy structs with the timestamp specified would be nice here like the other patches. IIRC we have 6 s32 channels, plus a s64 ts.
I think we may only have some of those channels enabled. So ts may be it several locations in the buffer.
Hence we could use the structure approach but it might give a false sense of what is going on.
J
Quoted 41 lines
Other than that everything looks good.
Andrew
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-0};enum afe4403_chan_id {@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 8 lines
drivers/iio/health/afe4403.c +0
-1struct iio_dev *indio_dev = pf->indio_dev;struct afe4403_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[8 ];u8 tx[4 ] = {AFE440X_CONTROL0 ,0x0 ,0x0 ,AFE440X_CONTROL0_READ };u8 rx[3 ];@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 9 lines
drivers/iio/health/afe4403.c +1
-1if (ret) goto err; - buffer[i++] =get_unaligned_be24 (&rx[0 ]); + afe->buffer[i++] =get_unaligned_be24 (&rx[0 ]); }/* Disable reading from the device */ @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int irq, void *private)
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-1if (ret) goto err; - iio_push_to_buffers_with_timestamp(indio_dev, buffer,
pf->timestamp);Quoted 6 lines
drivers/iio/health/afe4403.c +2
-0+iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.
Quoted 52 lines · Jonathan Cameron
Quoted 40 lines · Andrew F. Davis
Quoted 6 lines · On 5/17/20 1:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 32 byte array of smaller elements on thestack.
Quoted 7 lines
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 appart from previous readings.Fixes eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403heart monitor")
Review
Quoted 8 lines
Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Andrew F. Davis <[email protected]>--- drivers/iio/health/afe4403.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/iio/health/afe4403.cdrivers/iio/health/afe4403.c static const struct reg_field afe4403_reg_fields[]
Quoted 4 lines
index
e9f87e42ff4f..a3507624b30f100644drivers/iio/health/afe4403.c +0
-0@@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[]= {
Quoted 10 lines
drivers/iio/health/afe4403.c +1
-0* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct data layout to push into IIO buffer. */ struct afe4403_data {struct device *dev;@@ -74,6 +75,8 @@ struct afe4403_data {struct regulator *regulator;struct iio_trigger *trig;Quoted 8 lines
drivers/iio/health/afe4403.c struct afe4403_data { · line 79
Quoted 3 lines
drivers/iio/health/afe4403.c +2
-0int irq; +/* Ensure suitable alignment for timestamp */ +s32 buffer[8 ]__aligned(8) ;One of those fancy structs with the timestamp specified would be nice here like the other patches. IIRC we have 6 s32 channels, plus a s64 ts.
Quoted 3 lines
I think we may only have some of those channels enabled. So ts may be it several locations in the buffer.
Might have been better to have the ts at the beginning, could have also helped with alignment for when an odd number of channels are enabled.
Quoted 3 lines
Hence we could use the structure approach but it might give a false sense of what is going on.
That's true, it can always be cleaned later then.
Andrew
Quoted 43 lines
J
Quoted 41 lines
Other than that everything looks good.
Andrew
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-0};enum afe4403_chan_id {@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 8 lines
drivers/iio/health/afe4403.c +0
-1struct iio_dev *indio_dev = pf->indio_dev;struct afe4403_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[8 ];u8 tx[4 ] = {AFE440X_CONTROL0 ,0x0 ,0x0 ,AFE440X_CONTROL0_READ };u8 rx[3 ];@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 9 lines
drivers/iio/health/afe4403.c +1
-1if (ret) goto err; - buffer[i++] =get_unaligned_be24 (&rx[0 ]); + afe->buffer[i++] =get_unaligned_be24 (&rx[0 ]); }/* Disable reading from the device */ @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int irq, void *private)
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-1if (ret) goto err; - iio_push_to_buffers_with_timestamp(indio_dev, buffer,
pf->timestamp);Quoted 6 lines
drivers/iio/health/afe4403.c +2
-0+iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.
Quoted 59 lines · Andrew F. Davis
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Andrew F. Davis
Quoted 4 lines · On 5/17/20 1:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used isaligned
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 32 byte array of smaller elements on the
stack.
Quoted 3 lines
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 withalignment
Quoted 12 lines
Quoted 12 lines
Quoted 4 lines
explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.
Fixes eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403heart monitor")
Quoted 7 lines
Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Andrew F. Davis <[email protected]>--- drivers/iio/health/afe4403.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)Quoted 6 lines
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/iio/health/afe4403.cdrivers/iio/health/afe4403.c static const struct reg_field
Quoted 4 lines
index
e9f87e42ff4f..a3507624b30f100644drivers/iio/health/afe4403.c +0
-0@@ -65,6 +65,7 @@ static const struct reg_fieldafe4403_reg_fields[]
Quoted 17 lines
Quoted 17 lines
= {
drivers/iio/health/afe4403.c struct afe4403_data { · line 79
Quoted 13 lines
drivers/iio/health/afe4403.c +3
-0* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct data layout to push into IIO buffer. */ struct afe4403_data {struct device *dev;@@ -74,6 +75,8 @@ struct afe4403_data {struct regulator *regulator;struct iio_trigger *trig;int irq; +/* Ensure suitable alignment for timestamp */ +s32 buffer[8 ]__aligned(8) ;One of those fancy structs with the timestamp specified would be
nice
Quoted 3 lines
Quoted 2 lines
here like the other patches. IIRC we have 6 s32 channels, plus a s64 ts.
Quoted 7 lines
Quoted 1 line
I think we may only have some of those channels enabled. So ts may
be it several
Quoted 2 lines
locations in the buffer.
Might have been better to have the ts at the beginning, could have also helped with alignment for when an odd number of channels are enabled.
Perhaps but for many use cases we don't turn timestamps on and we would need to pad
anyway for alignment of the fifo.
Quoted 7 lines
Quoted 2 lines
Hence we could use the structure approach but it might give a false
sense
Quoted 2 lines
of what is going on.
That's true, it can always be cleaned later then.
Agreed. This is definitely something we can revisit sometime in the future.
J
Quoted 46 lines
Andrew
Quoted 43 lines
J
Quoted 41 lines
Other than that everything looks good.
Andrew
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-0};enum afe4403_chan_id {@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 8 lines
drivers/iio/health/afe4403.c +0
-1struct iio_dev *indio_dev = pf->indio_dev;struct afe4403_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[8 ];u8 tx[4 ] = {AFE440X_CONTROL0 ,0x0 ,0x0 ,AFE440X_CONTROL0_READ };u8 rx[3 ];@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 9 lines
drivers/iio/health/afe4403.c +1
-1if (ret) goto err; - buffer[i++] =get_unaligned_be24 (&rx[0 ]); + afe->buffer[i++] =get_unaligned_be24 (&rx[0 ]); }/* Disable reading from the device */ @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int irq, void *private)
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-1if (ret) goto err; - iio_push_to_buffers_with_timestamp(indio_dev, buffer,
pf->timestamp);Quoted 6 lines
drivers/iio/health/afe4403.c +2
-0+iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Jonathan Cameron
maintainerRe: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.
Quoted 72 lines · On Thu, 21 May 2020 15:44:43 +0100 Jonathan Cameron…
Quoted 59 lines · Andrew F. Davis
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Andrew F. Davis
Quoted 4 lines · On 5/17/20 1:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used isaligned
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 32 byte array of smaller elements on the
stack.
Quoted 3 lines
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 withalignment
Quoted 12 lines
Quoted 12 lines
Quoted 4 lines
explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.
Fixes eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403heart monitor")
Quoted 7 lines
Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Andrew F. Davis <[email protected]>--- drivers/iio/health/afe4403.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)Quoted 6 lines
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/iio/health/afe4403.cdrivers/iio/health/afe4403.c static const struct reg_field
Quoted 4 lines
index
e9f87e42ff4f..a3507624b30f100644drivers/iio/health/afe4403.c +0
-0@@ -65,6 +65,7 @@ static const struct reg_fieldafe4403_reg_fields[]
Quoted 17 lines
Quoted 17 lines
= {
drivers/iio/health/afe4403.c struct afe4403_data { · line 79
Quoted 13 lines
drivers/iio/health/afe4403.c +3
-0* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct data layout to push into IIO buffer. */ struct afe4403_data {struct device *dev;@@ -74,6 +75,8 @@ struct afe4403_data {struct regulator *regulator;struct iio_trigger *trig;int irq; +/* Ensure suitable alignment for timestamp */ +s32 buffer[8 ]__aligned(8) ;One of those fancy structs with the timestamp specified would be
nice
Quoted 3 lines
Quoted 2 lines
here like the other patches. IIRC we have 6 s32 channels, plus a s64 ts.
Quoted 7 lines
Quoted 1 line
I think we may only have some of those channels enabled. So ts may
be it several
Quoted 2 lines
locations in the buffer.
Might have been better to have the ts at the beginning, could have also helped with alignment for when an odd number of channels are enabled.
Perhaps but for many use cases we don't turn timestamps on and we would need to pad
anyway for alignment of the fifo.Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Hence we could use the structure approach but it might give a false
sense
Quoted 2 lines
of what is going on.
That's true, it can always be cleaned later then.
Agreed. This is definitely something we can revisit sometime in the future.
Formal Ack on applying the two afe patches?
Thanks,
Jonathan
Quoted 49 lines
J
Quoted 46 lines
Andrew
Quoted 43 lines
J
Quoted 41 lines
Other than that everything looks good.
Andrew
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-0};enum afe4403_chan_id {@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 8 lines
drivers/iio/health/afe4403.c +0
-1struct iio_dev *indio_dev = pf->indio_dev;struct afe4403_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[8 ];u8 tx[4 ] = {AFE440X_CONTROL0 ,0x0 ,0x0 ,AFE440X_CONTROL0_READ };u8 rx[3 ];@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 9 lines
drivers/iio/health/afe4403.c +1
-1if (ret) goto err; - buffer[i++] =get_unaligned_be24 (&rx[0 ]); + afe->buffer[i++] =get_unaligned_be24 (&rx[0 ]); }/* Disable reading from the device */ @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int irq, void *private)
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-1if (ret) goto err; - iio_push_to_buffers_with_timestamp(indio_dev, buffer,
pf->timestamp);Quoted 6 lines
drivers/iio/health/afe4403.c +2
-0+iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Andrew F. Davis
Acked-byRe: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.
Quoted 77 lines · Jonathan Cameron
Quoted 74 lines · On Thu, 21 May 2020 15:44:43 +0100 Jonathan Cameron…
Quoted 59 lines · Andrew F. Davis
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Andrew F. Davis
Quoted 4 lines · On 5/17/20 1:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used isaligned
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 32 byte array of smaller elements on the
stack.
Quoted 3 lines
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 withalignment
Quoted 12 lines
Quoted 12 lines
Quoted 4 lines
explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.
Fixes eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403heart monitor")
Quoted 7 lines
Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Andrew F. Davis <[email protected]>--- drivers/iio/health/afe4403.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)Quoted 6 lines
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/iio/health/afe4403.cdrivers/iio/health/afe4403.c static const struct reg_field
Quoted 4 lines
index
e9f87e42ff4f..a3507624b30f100644drivers/iio/health/afe4403.c +0
-0@@ -65,6 +65,7 @@ static const struct reg_fieldafe4403_reg_fields[]
Quoted 17 lines
Quoted 17 lines
= {
drivers/iio/health/afe4403.c struct afe4403_data { · line 79
Quoted 13 lines
drivers/iio/health/afe4403.c +3
-0* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct data layout to push into IIO buffer. */ struct afe4403_data {struct device *dev;@@ -74,6 +75,8 @@ struct afe4403_data {struct regulator *regulator;struct iio_trigger *trig;int irq; +/* Ensure suitable alignment for timestamp */ +s32 buffer[8 ]__aligned(8) ;One of those fancy structs with the timestamp specified would be
nice
Quoted 3 lines
Quoted 2 lines
here like the other patches. IIRC we have 6 s32 channels, plus a s64 ts.
Quoted 7 lines
Quoted 1 line
I think we may only have some of those channels enabled. So ts may
be it several
Quoted 2 lines
locations in the buffer.
Might have been better to have the ts at the beginning, could have also helped with alignment for when an odd number of channels are enabled.
Perhaps but for many use cases we don't turn timestamps on and we would need to pad
anyway for alignment of the fifo.Quoted 3 lines
Quoted 2 lines
Hence we could use the structure approach but it might give a false
sense
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
of what is going on.
That's true, it can always be cleaned later then.
Agreed. This is definitely something we can revisit sometime in the future.
Formal Ack on applying the two afe patches?
Quoted 53 lines
Thanks,
Jonathan
Quoted 49 lines
J
Quoted 46 lines
Andrew
Quoted 43 lines
J
Quoted 41 lines
Other than that everything looks good.
Andrew
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-0};enum afe4403_chan_id {@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 8 lines
drivers/iio/health/afe4403.c +0
-1struct iio_dev *indio_dev = pf->indio_dev;struct afe4403_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[8 ];u8 tx[4 ] = {AFE440X_CONTROL0 ,0x0 ,0x0 ,AFE440X_CONTROL0_READ };u8 rx[3 ];@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 9 lines
drivers/iio/health/afe4403.c +1
-1if (ret) goto err; - buffer[i++] =get_unaligned_be24 (&rx[0 ]); + afe->buffer[i++] =get_unaligned_be24 (&rx[0 ]); }/* Disable reading from the device */ @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int irq, void *private)
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-1if (ret) goto err; - iio_push_to_buffers_with_timestamp(indio_dev, buffer,
pf->timestamp);Quoted 6 lines
drivers/iio/health/afe4403.c +2
-0+iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.
Quoted 81 lines · On Thu, 21 May 2020 15:05:37 -0400 "Andrew F. Davis" <[email protected]> wrote:
Quoted 80 lines · Jonathan Cameron
Quoted 77 lines · On Thu, 21 May 2020 15:44:43 +0100 Jonathan Cameron…
Quoted 59 lines · Andrew F. Davis
Quoted 8 lines · Jonathan Cameron
Quoted 5 lines · Andrew F. Davis
Quoted 4 lines · On 5/17/20 1:29 PM, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used isaligned
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 32 byte array of smaller elements on the
stack.
Quoted 3 lines
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 withalignment
Quoted 12 lines
Quoted 12 lines
Quoted 4 lines
explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.
Fixes eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403heart monitor")
Quoted 7 lines
Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Andrew F. Davis <[email protected]>--- drivers/iio/health/afe4403.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)Quoted 6 lines
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/iio/health/afe4403.cdrivers/iio/health/afe4403.c static const struct reg_field
Quoted 4 lines
index
e9f87e42ff4f..a3507624b30f100644drivers/iio/health/afe4403.c +0
-0@@ -65,6 +65,7 @@ static const struct reg_fieldafe4403_reg_fields[]
Quoted 17 lines
Quoted 17 lines
= {
drivers/iio/health/afe4403.c struct afe4403_data { · line 79
Quoted 13 lines
drivers/iio/health/afe4403.c +3
-0* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct data layout to push into IIO buffer. */ struct afe4403_data {struct device *dev;@@ -74,6 +75,8 @@ struct afe4403_data {struct regulator *regulator;struct iio_trigger *trig;int irq; +/* Ensure suitable alignment for timestamp */ +s32 buffer[8 ]__aligned(8) ;One of those fancy structs with the timestamp specified would be
nice
Quoted 3 lines
Quoted 2 lines
here like the other patches. IIRC we have 6 s32 channels, plus a s64 ts.
Quoted 7 lines
Quoted 1 line
I think we may only have some of those channels enabled. So ts may
be it several
Quoted 2 lines
locations in the buffer.
Might have been better to have the ts at the beginning, could have also helped with alignment for when an odd number of channels are enabled.
Perhaps but for many use cases we don't turn timestamps on and we would need to pad
anyway for alignment of the fifo.Quoted 6 lines
Quoted 2 lines
Hence we could use the structure approach but it might give a false
sense
Quoted 2 lines
of what is going on.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
That's true, it can always be cleaned later then.
Agreed. This is definitely something we can revisit sometime in the future.
Formal Ack on applying the two afe patches?
Acked-by Andrew F. Davis <[email protected]>
Thanks.
Applied to the fixes-togreg branch of iio.git and marked for stable. These won't go anywhere now until after the merge window.
Thanks,
Jonathan
Quoted 54 lines
Quoted 54 lines
Thanks,
Jonathan
Quoted 49 lines
J
Quoted 46 lines
Andrew
Quoted 43 lines
J
Quoted 41 lines
Other than that everything looks good.
Andrew
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-0};enum afe4403_chan_id {@@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 8 lines
drivers/iio/health/afe4403.c +0
-1struct iio_dev *indio_dev = pf->indio_dev;struct afe4403_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[8 ];u8 tx[4 ] = {AFE440X_CONTROL0 ,0x0 ,0x0 ,AFE440X_CONTROL0_READ };u8 rx[3 ];@@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int
Quoted 9 lines
drivers/iio/health/afe4403.c +1
-1if (ret) goto err; - buffer[i++] =get_unaligned_be24 (&rx[0 ]); + afe->buffer[i++] =get_unaligned_be24 (&rx[0 ]); }/* Disable reading from the device */ @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(intirq, void *private)
drivers/iio/health/afe4403.c static irqreturn_t afe4403_trigger_handler(int · line 338
Quoted 4 lines
drivers/iio/health/afe4403.c +0
-1if (ret) goto err; - iio_push_to_buffers_with_timestamp(indio_dev, buffer,
pf->timestamp);Quoted 6 lines
drivers/iio/health/afe4403.c +2
-0+iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
[email protected]
maintainerPATCH 8/11 iio:health:afe4404 Fix timestamp alignment and prevent data leak.
In reply to [email protected]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 a 40 byte 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 appart from previous readings.
--- drivers/iio/health/afe4404.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/iio/health/afe4404.c +5 -3
@@ -83,6 +83,7 @@ static const struct reg_field afe4404_reg_fields[] = {* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct a scan to push to the iio buffer. */ struct afe4404_data {struct device *dev;
@@ -91,6 +92,7 @@ struct afe4404_data {struct regulator *regulator;struct iio_trigger *trig;int irq; +s32 buffer[10 ]__aligned(8) ; };enum afe4404_chan_id {
@@ -328,17 +330,17 @@ static irqreturn_t afe4404_trigger_handler(int irq, void *private)struct iio_dev *indio_dev = pf->indio_dev;struct afe4404_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[10 ];for_each_set_bit (bit, indio_dev->active_scan_mask, indio_dev->masklength) { ret =regmap_read (afe->regmap, afe4404_channel_values[bit], - &buffer[i++]); + &afe->buffer[i++]);if (ret) goto err; } -iio_push_to_buffers_with_timestamp (indio_dev, buffer, pf->timestamp); +iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
2.26.2
Re: [PATCH 08/11] iio:health:afe4404 Fix timestamp alignment and prevent data leak.
Quoted 12 lines · On Sun, 17 May 2020 18:29:57 +0100 [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes the buffer used is aligned to the size of the timestamp (8 bytes). This is not guaranteed in this driver which uses a 40 byte 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 theiio_priv()data with alignment explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.
Quoted 4 lines
Fixes 87aec56e27ef ("iio: health: Add driver for the TI AFE4404 heart monitor")Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Andrew F. Davis <[email protected]>
Applied to the fixes-togreg branch of iio.git and marked for stable.
Thanks,
Jonathan
Quoted 45 lines
--- drivers/iio/health/afe4404.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/iio/health/afe4404.c +5
-3@@ -83,6 +83,7 @@ static const struct reg_field afe4404_reg_fields[] = {* @regulator: Pointer to the regulator for the IC * @trig: IIO trigger for this device * @irq: ADC_RDY line interrupt number +* @buffer: Used to construct a scan to push to the iio buffer. */ struct afe4404_data {struct device *dev;@@ -91,6 +92,7 @@ struct afe4404_data {struct regulator *regulator;struct iio_trigger *trig;int irq; +s32 buffer[10 ]__aligned(8) ; };enum afe4404_chan_id {@@ -328,17 +330,17 @@ static irqreturn_t afe4404_trigger_handler(int irq, void *private)struct iio_dev *indio_dev = pf->indio_dev;struct afe4404_data *afe =iio_priv (indio_dev);int ret, bit, i =0 ; -s32 buffer[10 ];for_each_set_bit (bit, indio_dev->active_scan_mask, indio_dev->masklength) { ret =regmap_read (afe->regmap, afe4404_channel_values[bit], - &buffer[i++]); + &afe->buffer[i++]);if (ret) goto err; } -iio_push_to_buffers_with_timestamp (indio_dev, buffer, pf->timestamp); +iio_push_to_buffers_with_timestamp (indio_dev, afe->buffer, + pf->timestamp); err:iio_trigger_notify_done (indio_dev->trig);
[email protected]
maintainerPATCH 9/11 iio:chemical:ccs811: Fix timestamp alignment and prevent data leak.
In reply to [email protected]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() data with alignment explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.
--- drivers/iio/chemical/ccs811.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/iio/chemical/ccs811.c +9 -4
@@ -78,6 +78,11 @@ struct ccs811_data {struct iio_trigger *drdy_trig;struct gpio_desc *wakeup_gpio;bool drdy_trig_on; +/* Ensures correct alignment of timestamp if present */ +struct { +s16 channels[2 ]; +s64 ts; + } scan; };static const struct iio_chan_spec ccs811_channels[] = {
@@ -327,17 +332,17 @@ static irqreturn_t ccs811_trigger_handler(int irq, void *p)struct iio_dev *indio_dev = pf->indio_dev;struct ccs811_data *data =iio_priv (indio_dev);struct i2c_client *client = data->client; -s16 buf[8 ];/* s16 eCO2 + s16 TVOC + padding + 8 byte timestamp */ int ret; - ret =i2c_smbus_read_i2c_block_data (client,CCS811_ALG_RESULT_DATA ,4 , - (u8 *)&buf); + ret =i2c_smbus_read_i2c_block_data (client,CCS811_ALG_RESULT_DATA , +sizeof (data->scan.channels), + (u8 *)data->scan.channels);if (ret !=4 ) {dev_err (&client->dev,"cannot read sensor data\n" ); goto err; } -iio_push_to_buffers_with_timestamp (indio_dev, buf, +iio_push_to_buffers_with_timestamp (indio_dev, &data->scan,iio_get_time_ns (indio_dev)); err:
Signature · 1 line
2.26.2
[email protected]
maintainerPATCH 10/11 iio:chemical:sps30: Fix timestamp alignment
In reply to [email protected]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.
--- drivers/iio/chemical/sps30.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/iio/chemical/sps30.c +6 -3
@@ -230,15 +230,18 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)struct iio_dev *indio_dev = pf->indio_dev;struct sps30_state *state =iio_priv (indio_dev);int ret; -s32 data[4 +2 ];/* PM1, PM2P5, PM4, PM10, timestamp */ +struct { +s32 data[4 ];/* PM1, PM2P5, PM4, PM10 */ +s64 ts; + } scan;mutex_lock (&state->lock); - ret =sps30_do_meas (state, data,4 ); + ret =sps30_do_meas (state, scan.data,ARRAY_SIZE (scan.data));mutex_unlock (&state->lock);if (ret) goto err; -iio_push_to_buffers_with_timestamp (indio_dev, data, +iio_push_to_buffers_with_timestamp (indio_dev, &scan,iio_get_time_ns (indio_dev)); err:iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
2.26.2
Re: [PATCH 10/11] iio:chemical:sps30: Fix timestamp alignment
Quoted 42 lines · On Sun, May 17, 2020 at 06:29:59PM +0100, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes 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.Fixes 232e0f6ddeae ("iio: chemical: add support for Sensirion SPS30 sensor")Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Tomasz Duszynski <[email protected]>--- drivers/iio/chemical/sps30.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/iio/chemical/sps30.c +6
-4@@ -230,15 +230,18 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)struct iio_dev *indio_dev = pf->indio_dev;struct sps30_state *state =iio_priv (indio_dev);int ret; -s32 data[4 +2 ];/* PM1, PM2P5, PM4, PM10, timestamp */ +struct { +s32 data[4 ];/* PM1, PM2P5, PM4, PM10 */ +s64 ts; + } scan;mutex_lock (&state->lock); - ret =sps30_do_meas (state, data,4 ); + ret =sps30_do_meas (state, scan.data,ARRAY_SIZE (scan.data));mutex_unlock (&state->lock);if (ret) goto err; -iio_push_to_buffers_with_timestamp (indio_dev, data, +iio_push_to_buffers_with_timestamp (indio_dev, &scan,iio_get_time_ns (indio_dev)); err:iio_trigger_notify_done (indio_dev->trig); --
Quoted 2 lines
2.26.2
Thanks for the fix.
Re: [PATCH 10/11] iio:chemical:sps30: Fix timestamp alignment
Quoted 47 lines · On Sun, 17 May 2020 21:07:28 +0200 Tomasz Duszynski…
Quoted 42 lines · On Sun, May 17, 2020 at 06:29:59PM +0100, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes 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.Fixes 232e0f6ddeae ("iio: chemical: add support for Sensirion SPS30 sensor")Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Tomasz Duszynski <[email protected]>--- drivers/iio/chemical/sps30.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/iio/chemical/sps30.c +6
-4@@ -230,15 +230,18 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)struct iio_dev *indio_dev = pf->indio_dev;struct sps30_state *state =iio_priv (indio_dev);int ret; -s32 data[4 +2 ];/* PM1, PM2P5, PM4, PM10, timestamp */ +struct { +s32 data[4 ];/* PM1, PM2P5, PM4, PM10 */ +s64 ts; + } scan;mutex_lock (&state->lock); - ret =sps30_do_meas (state, data,4 ); + ret =sps30_do_meas (state, scan.data,ARRAY_SIZE (scan.data));mutex_unlock (&state->lock);if (ret) goto err; -iio_push_to_buffers_with_timestamp (indio_dev, data, +iio_push_to_buffers_with_timestamp (indio_dev, &scan,iio_get_time_ns (indio_dev)); err:iio_trigger_notify_done (indio_dev->trig); --Quoted 2 lines
2.26.2
Thanks for the fix.
Quoted 1 line
Acked-by Tomasz Duszynski <[email protected]>
Applied to the fixes-togreg branch of iio.git.
I'm going to pick these up as and when I get reviews for them rather than waiting for the whole series to have suitable reviews.
They are a small part of all the instances we have of this issue so no point in waiting.
Thanks,
Jonathan
[email protected]
maintainerPATCH 11/11 iio:chemical:pms7003: Fix timestamp alignment and prevent data leak.
In reply to [email protected]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() data with alignment explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.
--- drivers/iio/chemical/pms7003.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/iio/chemical/pms7003.c +12 -5
@@ -73,6 +73,11 @@ struct pms7003_state {struct pms7003_frame frame;struct completion frame_ready;struct mutex lock;/* must be held whenever state gets touched */ +/* Used to construct scan to push to the IIO buffer */ +struct { +u16 data[3 ];/* PM1, PM2P5, PM10 */ +s64 ts; + } scan; };static int pms7003_do_cmd (struct pms7003_state *state,enum pms7003_cmd cmd)
@@ -104,7 +109,6 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)struct iio_dev *indio_dev = pf->indio_dev;struct pms7003_state *state =iio_priv (indio_dev);struct pms7003_frame *frame = &state->frame; -u16 data[3 +1 +4 ];/* PM1, PM2P5, PM10, padding, timestamp */ int ret;mutex_lock (&state->lock);
@@ -114,12 +118,15 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p) goto err; } - data[PM1 ] =pms7003_get_pm (frame->data +PMS7003_PM1_OFFSET ); - data[PM2P5 ] =pms7003_get_pm (frame->data +PMS7003_PM2P5_OFFSET ); - data[PM10 ] =pms7003_get_pm (frame->data +PMS7003_PM10_OFFSET ); + state->scan.data[PM1 ] = +pms7003_get_pm (frame->data +PMS7003_PM1_OFFSET ); + state->scan.data[PM2P5 ] = +pms7003_get_pm (frame->data +PMS7003_PM2P5_OFFSET ); + state->scan.data[PM10 ] = +pms7003_get_pm (frame->data +PMS7003_PM10_OFFSET );mutex_unlock (&state->lock); -iio_push_to_buffers_with_timestamp (indio_dev, data, +iio_push_to_buffers_with_timestamp (indio_dev, &state->scan,iio_get_time_ns (indio_dev)); err:iio_trigger_notify_done (indio_dev->trig);
Signature · 1 line
2.26.2
Re: [PATCH 11/11] iio:chemical:pms7003: Fix timestamp alignment and prevent data leak.
Quoted 65 lines · On Sun, May 17, 2020 at 06:30:00PM +0100, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes 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 theiio_priv()data with alignment explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.Fixes a1d642266c14 ("iio: chemical: add support for Plantower PMS7003 sensor")Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Tomasz Duszynski <[email protected]>--- drivers/iio/chemical/pms7003.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)drivers/iio/chemical/pms7003.c +12
-6@@ -73,6 +73,11 @@ struct pms7003_state {struct pms7003_frame frame;struct completion frame_ready;struct mutex lock;/* must be held whenever state gets touched */ +/* Used to construct scan to push to the IIO buffer */ +struct { +u16 data[3 ];/* PM1, PM2P5, PM10 */ +s64 ts; + } scan; };static int pms7003_do_cmd (struct pms7003_state *state,enum pms7003_cmd cmd)@@ -104,7 +109,6 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)struct iio_dev *indio_dev = pf->indio_dev;struct pms7003_state *state =iio_priv (indio_dev);struct pms7003_frame *frame = &state->frame; -u16 data[3 +1 +4 ];/* PM1, PM2P5, PM10, padding, timestamp */ int ret;mutex_lock (&state->lock);@@ -114,12 +118,15 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p) goto err; } - data[PM1 ] =pms7003_get_pm (frame->data +PMS7003_PM1_OFFSET ); - data[PM2P5 ] =pms7003_get_pm (frame->data +PMS7003_PM2P5_OFFSET ); - data[PM10 ] =pms7003_get_pm (frame->data +PMS7003_PM10_OFFSET ); + state->scan.data[PM1 ] = +pms7003_get_pm (frame->data +PMS7003_PM1_OFFSET ); + state->scan.data[PM2P5 ] = +pms7003_get_pm (frame->data +PMS7003_PM2P5_OFFSET ); + state->scan.data[PM10 ] = +pms7003_get_pm (frame->data +PMS7003_PM10_OFFSET );mutex_unlock (&state->lock); -iio_push_to_buffers_with_timestamp (indio_dev, data, +iio_push_to_buffers_with_timestamp (indio_dev, &state->scan,iio_get_time_ns (indio_dev)); err:iio_trigger_notify_done (indio_dev->trig); --
Quoted 2 lines
2.26.2
Thanks for the fix.
Re: [PATCH 11/11] iio:chemical:pms7003: Fix timestamp alignment and prevent data leak.
Quoted 70 lines · On Sun, 17 May 2020 21:06:55 +0200 Tomasz Duszynski…
Quoted 65 lines · On Sun, May 17, 2020 at 06:30:00PM +0100, [email protected] wrote:
From: Jonathan Cameron <[email protected]>
One of a class of bugs pointed out by Lars in a recent review.
iio_push_to_buffers_with_timestampassumes 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 theiio_priv()data with alignment explicitly requested. This data is allocated with kzalloc so no data can leak appart from previous readings.Fixes a1d642266c14 ("iio: chemical: add support for Plantower PMS7003 sensor")Reported-by Lars-Peter Clausen <[email protected]>Signed-off-by Jonathan Cameron <[email protected]>Cc Tomasz Duszynski <[email protected]>--- drivers/iio/chemical/pms7003.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)drivers/iio/chemical/pms7003.c +12
-6@@ -73,6 +73,11 @@ struct pms7003_state {struct pms7003_frame frame;struct completion frame_ready;struct mutex lock;/* must be held whenever state gets touched */ +/* Used to construct scan to push to the IIO buffer */ +struct { +u16 data[3 ];/* PM1, PM2P5, PM10 */ +s64 ts; + } scan; };static int pms7003_do_cmd (struct pms7003_state *state,enum pms7003_cmd cmd)@@ -104,7 +109,6 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p)struct iio_dev *indio_dev = pf->indio_dev;struct pms7003_state *state =iio_priv (indio_dev);struct pms7003_frame *frame = &state->frame; -u16 data[3 +1 +4 ];/* PM1, PM2P5, PM10, padding, timestamp */ int ret;mutex_lock (&state->lock);@@ -114,12 +118,15 @@ static irqreturn_t pms7003_trigger_handler(int irq, void *p) goto err; } - data[PM1 ] =pms7003_get_pm (frame->data +PMS7003_PM1_OFFSET ); - data[PM2P5 ] =pms7003_get_pm (frame->data +PMS7003_PM2P5_OFFSET ); - data[PM10 ] =pms7003_get_pm (frame->data +PMS7003_PM10_OFFSET ); + state->scan.data[PM1 ] = +pms7003_get_pm (frame->data +PMS7003_PM1_OFFSET ); + state->scan.data[PM2P5 ] = +pms7003_get_pm (frame->data +PMS7003_PM2P5_OFFSET ); + state->scan.data[PM10 ] = +pms7003_get_pm (frame->data +PMS7003_PM10_OFFSET );mutex_unlock (&state->lock); -iio_push_to_buffers_with_timestamp (indio_dev, data, +iio_push_to_buffers_with_timestamp (indio_dev, &state->scan,iio_get_time_ns (indio_dev)); err:iio_trigger_notify_done (indio_dev->trig); --Quoted 2 lines
2.26.2
Thanks for the fix.
Quoted 2 lines
Acked-by Tomasz Duszynski <[email protected]>
Applied. Thanks,