This issue was found after attempting to make the same mistake for a driver I maintain, which was fortunately spotted by Jonathan [1].
Keeping old sensor values if the channel configuration changes is known and not considered an issue, which is also mentioned in [1], so it has not been addressed by this series. That keeps most of the drivers out of the way because they store the scan element in iio private data, which is kzalloc() allocated.
This series only addresses cases where uninitialized i.e. unknown data is pushed to the userspace, either due to holes in structs or uninitialized struct members/array elements.
While analyzing involved functions, I found and fixed some triviality (wrong function name) in the documentation of iio_dev_opaque.
Link: https://lore.kernel.org/linux-iio/20241123151634.303aa860@jic23-huawei/ [1]
Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- Javier Carrasco (11): iio: temperature: tmp006: fix information leak in triggered buffer iio: adc: ti-ads1119: fix information leak in triggered buffer iio: pressure: zpa2326: fix information leak in triggered buffer iio: adc: rockchip_saradc: fix information leak in triggered buffer iio: imu: kmx61: fix information leak in triggered buffer iio: light: vcnl4035: fix information leak in triggered buffer iio: light: bh1745: fix information leak in triggered buffer iio: adc: ti-ads8688: fix information leak in triggered buffer iio: dummy: iio_simply_dummy_buffer: fix information leak in triggered buffer iio: light: as73211: fix information leak in triggered buffer iio: core: fix doc reference to iio_push_to_buffers_with_ts_unaligned
drivers/iio/adc/rockchip_saradc.c | 2 ++ drivers/iio/adc/ti-ads1119.c | 2 ++ drivers/iio/adc/ti-ads8688.c | 2 +- drivers/iio/dummy/iio_simple_dummy_buffer.c | 2 +- drivers/iio/imu/kmx61.c | 2 +- drivers/iio/light/as73211.c | 3 +++ drivers/iio/light/bh1745.c | 2 ++ drivers/iio/light/vcnl4035.c | 2 +- drivers/iio/pressure/zpa2326.c | 2 ++ drivers/iio/temperature/tmp006.c | 2 ++ include/linux/iio/iio-opaque.h | 2 +- 11 files changed, 18 insertions(+), 5 deletions(-) --- base-commit: ab376e4d674037f45d5758c1dc391bd4e11c5dc4 change-id: 20241123-iio_memset_scan_holes-a673833ef932
Best regards,
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the two 16-bit data channels and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 91f75ccf9f03 ("iio: temperature: tmp006: add triggered buffer support") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/temperature/tmp006.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c index 0c844137d7aa..02b27f471baa 100644 --- a/drivers/iio/temperature/tmp006.c +++ b/drivers/iio/temperature/tmp006.c @@ -252,6 +252,8 @@ static irqreturn_t tmp006_trigger_handler(int irq, void *p) } scan; s32 ret;
+ memset(&scan, 0, sizeof(scan)); + ret = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT); if (ret < 0) goto err;
On 25/11/2024 22:16, Javier Carrasco wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the two 16-bit data channels and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 91f75ccf9f03 ("iio: temperature: tmp006: add triggered buffer support") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
drivers/iio/temperature/tmp006.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c index 0c844137d7aa..02b27f471baa 100644 --- a/drivers/iio/temperature/tmp006.c +++ b/drivers/iio/temperature/tmp006.c @@ -252,6 +252,8 @@ static irqreturn_t tmp006_trigger_handler(int irq, void *p) } scan; s32 ret;
- memset(&scan, 0, sizeof(scan));
- ret = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT); if (ret < 0) goto err;
@Jonathan, this patch requires 91f75ccf9f03 ("iio: temperature: tmp006: add triggered buffer support"), which is in the mainline kernel, but not accessible from iio/fixes-to-greg.
Is there any branch in IIO where the fixes and the new features are put together? I would like to rebase my series to automatically get rid of the applied patches, but iio/fixes-to-greg (where the patches were applied) does not have the feature this patch fixes. Of course I can manually drop the applied patches, but that is error-prone.
This is not the first time I face this inconvenience, and I suppose there is a cleaner way that I might be missing, or maybe that branch I am looking for already exists.
Thanks and best regards, Javier Carrasco
On Mon, 2 Dec 2024 20:28:12 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
On 25/11/2024 22:16, Javier Carrasco wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the two 16-bit data channels and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 91f75ccf9f03 ("iio: temperature: tmp006: add triggered buffer support") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
drivers/iio/temperature/tmp006.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c index 0c844137d7aa..02b27f471baa 100644 --- a/drivers/iio/temperature/tmp006.c +++ b/drivers/iio/temperature/tmp006.c @@ -252,6 +252,8 @@ static irqreturn_t tmp006_trigger_handler(int irq, void *p) } scan; s32 ret;
- memset(&scan, 0, sizeof(scan));
- ret = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT); if (ret < 0) goto err;
@Jonathan, this patch requires 91f75ccf9f03 ("iio: temperature: tmp006: add triggered buffer support"), which is in the mainline kernel, but not accessible from iio/fixes-to-greg.
Yeah. That happens briefly around merge windows. In this particular case to just after rc1 as there were some tree wide refactors that needed merging. Sometimes it takes me a few days to find the time to rebase. Doing anything mid merge window is a challenge at best.
Is there any branch in IIO where the fixes and the new features are put together? I would like to rebase my series to automatically get rid of the applied patches, but iio/fixes-to-greg (where the patches were applied) does not have the feature this patch fixes. Of course I can manually drop the applied patches, but that is error-prone.
No. I don't push out such a tree, though I often do test merges.
You could use linux-next for your automation as that normally contains both the fixes-togreg and togreg branches. Mind you that doesn't right now because of the merge issue mentioned above,
Jonathan
This is not the first time I face this inconvenience, and I suppose there is a cleaner way that I might be missing, or maybe that branch I am looking for already exists.
Thanks and best regards, Javier Carrasco
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret;
+ memset(&scan, 0, sizeof(scan)); + if (!iio_trigger_using_own(indio_dev)) { index = find_first_bit(indio_dev->active_scan_mask, iio_get_masklength(indio_dev));
On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret;
- memset(&scan, 0, sizeof(scan));
Did you consider adding a reserved field after sample and just initializing that one to zero?
It seems a trivial optimization not adding much value, but I thought about it, so I'd like to be sure you considered it.
In any case, the change is fine.
Reviewed-by: Francesco Dolcini francesco.dolcini@toradex.com
Thanks, Francesco
On 26/11/2024 09:59, Francesco Dolcini wrote:
On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret;
- memset(&scan, 0, sizeof(scan));
Did you consider adding a reserved field after sample and just initializing that one to zero?
It seems a trivial optimization not adding much value, but I thought about it, so I'd like to be sure you considered it.
In any case, the change is fine.
Reviewed-by: Francesco Dolcini francesco.dolcini@toradex.com
Thanks, Francesco
Hi Francesco, thanks for your review.
In this particular case where unsigned int is used for the sample, the padding would _in theory_ depend on the architecture. The size of the unsigned int is usually 4 bytes, but the standard only specifies that it must be able to contain values in the [0, 65535] range i.e. 2 bytes. That is indeed theory, and I don't know if there is a real case where a new version of Linux is able to run on an architecture that uses 2 bytes for an int. I guess there is not, but better safe than sorry.
We could be more specific with u32 for the sample and then add the reserved field, but I would still prefer a memset() for this small struct. Adding and initializing a reserved field looks a bit artificial to me, especially for such marginal gains.
Moreover, the common practice (at least in IIO)is a plain memset() to initialize struct holes, and such common patterns are easier to maintain :)
Best regards, Javier Carrasco
On Tue, 26 Nov 2024 10:46:37 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
On 26/11/2024 09:59, Francesco Dolcini wrote:
On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret;
- memset(&scan, 0, sizeof(scan));
Did you consider adding a reserved field after sample and just initializing that one to zero?
It seems a trivial optimization not adding much value, but I thought about it, so I'd like to be sure you considered it.
In any case, the change is fine.
Reviewed-by: Francesco Dolcini francesco.dolcini@toradex.com
Thanks, Francesco
Hi Francesco, thanks for your review.
In this particular case where unsigned int is used for the sample, the padding would _in theory_ depend on the architecture. The size of the unsigned int is usually 4 bytes, but the standard only specifies that it must be able to contain values in the [0, 65535] range i.e. 2 bytes. That is indeed theory, and I don't know if there is a real case where a new version of Linux is able to run on an architecture that uses 2 bytes for an int. I guess there is not, but better safe than sorry.
Using an unsigned int here is a bug as well as we should present consistent formatted data whatever the architecture.
We could be more specific with u32 for the sample and then add the reserved field, but I would still prefer a memset() for this small struct. Adding and initializing a reserved field looks a bit artificial to me, especially for such marginal gains.
Issue with reserved fields is we would have to be very very careful to spot them all. A memset avoids that care being needed.
Jonathan
Moreover, the common practice (at least in IIO)is a plain memset() to initialize struct holes, and such common patterns are easier to maintain :)
Best regards, Javier Carrasco
On Tue Nov 26, 2024 at 7:52 PM CET, Jonathan Cameron wrote:
On Tue, 26 Nov 2024 10:46:37 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
On 26/11/2024 09:59, Francesco Dolcini wrote:
On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret;
- memset(&scan, 0, sizeof(scan));
Did you consider adding a reserved field after sample and just initializing that one to zero?
It seems a trivial optimization not adding much value, but I thought about it, so I'd like to be sure you considered it.
In any case, the change is fine.
Reviewed-by: Francesco Dolcini francesco.dolcini@toradex.com
Thanks, Francesco
Hi Francesco, thanks for your review.
In this particular case where unsigned int is used for the sample, the padding would _in theory_ depend on the architecture. The size of the unsigned int is usually 4 bytes, but the standard only specifies that it must be able to contain values in the [0, 65535] range i.e. 2 bytes. That is indeed theory, and I don't know if there is a real case where a new version of Linux is able to run on an architecture that uses 2 bytes for an int. I guess there is not, but better safe than sorry.
Using an unsigned int here is a bug as well as we should present consistent formatted data whatever the architecture.
Would you prefer that in the same patch as they are related issues? I could switch to u32 in v2 along with anything else that might arise in the reviews of the rest of the series. If you prefer a separate patch, that's fine too.
We could be more specific with u32 for the sample and then add the reserved field, but I would still prefer a memset() for this small struct. Adding and initializing a reserved field looks a bit artificial to me, especially for such marginal gains.
Issue with reserved fields is we would have to be very very careful to spot them all. A memset avoids that care being needed.
Jonathan
Moreover, the common practice (at least in IIO)is a plain memset() to initialize struct holes, and such common patterns are easier to maintain :)
Best regards, Javier Carrasco
On 26/11/2024 23:00, Javier Carrasco wrote:
On Tue Nov 26, 2024 at 7:52 PM CET, Jonathan Cameron wrote:
On Tue, 26 Nov 2024 10:46:37 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
On 26/11/2024 09:59, Francesco Dolcini wrote:
On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret;
- memset(&scan, 0, sizeof(scan));
Did you consider adding a reserved field after sample and just initializing that one to zero?
It seems a trivial optimization not adding much value, but I thought about it, so I'd like to be sure you considered it.
In any case, the change is fine.
Reviewed-by: Francesco Dolcini francesco.dolcini@toradex.com
Thanks, Francesco
Hi Francesco, thanks for your review.
In this particular case where unsigned int is used for the sample, the padding would _in theory_ depend on the architecture. The size of the unsigned int is usually 4 bytes, but the standard only specifies that it must be able to contain values in the [0, 65535] range i.e. 2 bytes. That is indeed theory, and I don't know if there is a real case where a new version of Linux is able to run on an architecture that uses 2 bytes for an int. I guess there is not, but better safe than sorry.
Using an unsigned int here is a bug as well as we should present consistent formatted data whatever the architecture.
Would you prefer that in the same patch as they are related issues? I could switch to u32 in v2 along with anything else that might arise in the reviews of the rest of the series. If you prefer a separate patch, that's fine too.
Although now that I am looking into it, and according to the datasheet and defined scan_type, the right size should be s16.
We could be more specific with u32 for the sample and then add the reserved field, but I would still prefer a memset() for this small struct. Adding and initializing a reserved field looks a bit artificial to me, especially for such marginal gains.
Issue with reserved fields is we would have to be very very careful to spot them all. A memset avoids that care being needed.
Jonathan
Moreover, the common practice (at least in IIO)is a plain memset() to initialize struct holes, and such common patterns are easier to maintain :)
Best regards, Javier Carrasco
On Wed, 27 Nov 2024 01:30:36 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
On 26/11/2024 23:00, Javier Carrasco wrote:
On Tue Nov 26, 2024 at 7:52 PM CET, Jonathan Cameron wrote:
On Tue, 26 Nov 2024 10:46:37 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
On 26/11/2024 09:59, Francesco Dolcini wrote:
On Mon, Nov 25, 2024 at 10:16:10PM +0100, Javier Carrasco wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret;
- memset(&scan, 0, sizeof(scan));
Did you consider adding a reserved field after sample and just initializing that one to zero?
It seems a trivial optimization not adding much value, but I thought about it, so I'd like to be sure you considered it.
In any case, the change is fine.
Reviewed-by: Francesco Dolcini francesco.dolcini@toradex.com
Thanks, Francesco
Hi Francesco, thanks for your review.
In this particular case where unsigned int is used for the sample, the padding would _in theory_ depend on the architecture. The size of the unsigned int is usually 4 bytes, but the standard only specifies that it must be able to contain values in the [0, 65535] range i.e. 2 bytes. That is indeed theory, and I don't know if there is a real case where a new version of Linux is able to run on an architecture that uses 2 bytes for an int. I guess there is not, but better safe than sorry.
Using an unsigned int here is a bug as well as we should present consistent formatted data whatever the architecture.
Would you prefer that in the same patch as they are related issues? I could switch to u32 in v2 along with anything else that might arise in the reviews of the rest of the series. If you prefer a separate patch, that's fine too.
Although now that I am looking into it, and according to the datasheet and defined scan_type, the right size should be s16.
Separate patch would be great!
Thanks
Jonathan
We could be more specific with u32 for the sample and then add the reserved field, but I would still prefer a memset() for this small struct. Adding and initializing a reserved field looks a bit artificial to me, especially for such marginal gains.
Issue with reserved fields is we would have to be very very careful to spot them all. A memset avoids that care being needed.
Jonathan
Moreover, the common practice (at least in IIO)is a plain memset() to initialize struct holes, and such common patterns are easier to maintain :)
Best regards, Javier Carrasco
On Mon, 25 Nov 2024 22:16:10 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it has a hole between the sample (unsigned int) and the timestamp. This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: a9306887eba4 ("iio: adc: ti-ads1119: Add driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Applied.
drivers/iio/adc/ti-ads1119.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c index e9d9d4d46d38..2615a275acb3 100644 --- a/drivers/iio/adc/ti-ads1119.c +++ b/drivers/iio/adc/ti-ads1119.c @@ -506,6 +506,8 @@ static irqreturn_t ads1119_trigger_handler(int irq, void *private) unsigned int index; int ret;
- memset(&scan, 0, sizeof(scan));
- if (!iio_trigger_using_own(indio_dev)) { index = find_first_bit(indio_dev->active_scan_mask, iio_get_masklength(indio_dev));
The 'sample' local struct is used to push data to user space from a triggered buffer, but it has a hole between the temperature and the timestamp (u32 pressure, u16 temperature, GAP, u64 timestamp). This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 03b262f2bbf4 ("iio:pressure: initial zpa2326 barometer support") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/pressure/zpa2326.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/pressure/zpa2326.c b/drivers/iio/pressure/zpa2326.c index 950f8dee2b26..b4c6c7c47256 100644 --- a/drivers/iio/pressure/zpa2326.c +++ b/drivers/iio/pressure/zpa2326.c @@ -586,6 +586,8 @@ static int zpa2326_fill_sample_buffer(struct iio_dev *indio_dev, } sample; int err;
+ memset(&sample, 0, sizeof(sample)); + if (test_bit(0, indio_dev->active_scan_mask)) { /* Get current pressure from hardware FIFO. */ err = zpa2326_dequeue_pressure(indio_dev, &sample.pressure);
On Mon, 25 Nov 2024 22:16:11 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'sample' local struct is used to push data to user space from a triggered buffer, but it has a hole between the temperature and the timestamp (u32 pressure, u16 temperature, GAP, u64 timestamp). This hole is never initialized.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 03b262f2bbf4 ("iio:pressure: initial zpa2326 barometer support") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Applied.
drivers/iio/pressure/zpa2326.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/pressure/zpa2326.c b/drivers/iio/pressure/zpa2326.c index 950f8dee2b26..b4c6c7c47256 100644 --- a/drivers/iio/pressure/zpa2326.c +++ b/drivers/iio/pressure/zpa2326.c @@ -586,6 +586,8 @@ static int zpa2326_fill_sample_buffer(struct iio_dev *indio_dev, } sample; int err;
- memset(&sample, 0, sizeof(sample));
- if (test_bit(0, indio_dev->active_scan_mask)) { /* Get current pressure from hardware FIFO. */ err = zpa2326_dequeue_pressure(indio_dev, &sample.pressure);
The 'data' local struct is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 4e130dc7b413 ("iio: adc: rockchip_saradc: Add support iio buffers") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/adc/rockchip_saradc.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c index 240cfa391674..dfd47a6e1f4a 100644 --- a/drivers/iio/adc/rockchip_saradc.c +++ b/drivers/iio/adc/rockchip_saradc.c @@ -368,6 +368,8 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p) int ret; int i, j = 0;
+ memset(&data, 0, sizeof(data)); + mutex_lock(&info->lock);
iio_for_each_active_channel(i_dev, i) {
On Mon, 25 Nov 2024 22:16:12 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'data' local struct is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 4e130dc7b413 ("iio: adc: rockchip_saradc: Add support iio buffers") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Applied.
drivers/iio/adc/rockchip_saradc.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c index 240cfa391674..dfd47a6e1f4a 100644 --- a/drivers/iio/adc/rockchip_saradc.c +++ b/drivers/iio/adc/rockchip_saradc.c @@ -368,6 +368,8 @@ static irqreturn_t rockchip_saradc_trigger_handler(int irq, void *p) int ret; int i, j = 0;
- memset(&data, 0, sizeof(data));
- mutex_lock(&info->lock);
iio_for_each_active_channel(i_dev, i) {
The 'buffer' local array is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Initialize the array to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: c3a23ecc0901 ("iio: imu: kmx61: Add support for data ready triggers") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/imu/kmx61.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c index 324c38764656..e19c5d3137c6 100644 --- a/drivers/iio/imu/kmx61.c +++ b/drivers/iio/imu/kmx61.c @@ -1193,7 +1193,7 @@ static irqreturn_t kmx61_trigger_handler(int irq, void *p) struct kmx61_data *data = kmx61_get_data(indio_dev); int bit, ret, i = 0; u8 base; - s16 buffer[8]; + s16 buffer[8] = { };
if (indio_dev == data->acc_indio_dev) base = KMX61_ACC_XOUT_L;
On Mon, 25 Nov 2024 22:16:13 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'buffer' local array is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Initialize the array to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: c3a23ecc0901 ("iio: imu: kmx61: Add support for data ready triggers") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Applied.
drivers/iio/imu/kmx61.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c index 324c38764656..e19c5d3137c6 100644 --- a/drivers/iio/imu/kmx61.c +++ b/drivers/iio/imu/kmx61.c @@ -1193,7 +1193,7 @@ static irqreturn_t kmx61_trigger_handler(int irq, void *p) struct kmx61_data *data = kmx61_get_data(indio_dev); int bit, ret, i = 0; u8 base;
- s16 buffer[8];
- s16 buffer[8] = { };
if (indio_dev == data->acc_indio_dev) base = KMX61_ACC_XOUT_L;
The 'buffer' local array is used to push data to userspace from a triggered buffer, but it does not set an initial value for the single data element, which is an u16 aligned to 8 bytes. That leaves at least 4 bytes uninitialized even after writing an integer value with regmap_read().
Initialize the array to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: ec90b52c07c0 ("iio: light: vcnl4035: Fix buffer alignment in iio_push_to_buffers_with_timestamp()") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/light/vcnl4035.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c index 337a1332c2c6..67c94be02018 100644 --- a/drivers/iio/light/vcnl4035.c +++ b/drivers/iio/light/vcnl4035.c @@ -105,7 +105,7 @@ static irqreturn_t vcnl4035_trigger_consumer_handler(int irq, void *p) struct iio_dev *indio_dev = pf->indio_dev; struct vcnl4035_data *data = iio_priv(indio_dev); /* Ensure naturally aligned timestamp */ - u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)] __aligned(8); + u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)] __aligned(8) = { }; int ret;
ret = regmap_read(data->regmap, VCNL4035_ALS_DATA, (int *)buffer);
On Mon, 25 Nov 2024 22:16:14 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'buffer' local array is used to push data to userspace from a triggered buffer, but it does not set an initial value for the single data element, which is an u16 aligned to 8 bytes. That leaves at least 4 bytes uninitialized even after writing an integer value with regmap_read().
Initialize the array to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: ec90b52c07c0 ("iio: light: vcnl4035: Fix buffer alignment in iio_push_to_buffers_with_timestamp()") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Applied.
drivers/iio/light/vcnl4035.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c index 337a1332c2c6..67c94be02018 100644 --- a/drivers/iio/light/vcnl4035.c +++ b/drivers/iio/light/vcnl4035.c @@ -105,7 +105,7 @@ static irqreturn_t vcnl4035_trigger_consumer_handler(int irq, void *p) struct iio_dev *indio_dev = pf->indio_dev; struct vcnl4035_data *data = iio_priv(indio_dev); /* Ensure naturally aligned timestamp */
- u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)] __aligned(8);
- u8 buffer[ALIGN(sizeof(u16), sizeof(s64)) + sizeof(s64)] __aligned(8) = { }; int ret;
ret = regmap_read(data->regmap, VCNL4035_ALS_DATA, (int *)buffer);
The 'scan' local struct is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: eab35358aae7 ("iio: light: ROHM BH1745 colour sensor") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/light/bh1745.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c index 23e9f16090cc..2ffc839c7501 100644 --- a/drivers/iio/light/bh1745.c +++ b/drivers/iio/light/bh1745.c @@ -746,6 +746,8 @@ static irqreturn_t bh1745_trigger_handler(int interrupt, void *p) int i; int j = 0;
+ memset(&scan, 0, sizeof(scan)); + iio_for_each_active_channel(indio_dev, i) { ret = regmap_bulk_read(data->regmap, BH1745_RED_LSB + 2 * i, &value, 2);
On Mon, 25 Nov 2024 22:16:15 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'scan' local struct is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Initialize the struct to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: eab35358aae7 ("iio: light: ROHM BH1745 colour sensor") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Applied.
drivers/iio/light/bh1745.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c index 23e9f16090cc..2ffc839c7501 100644 --- a/drivers/iio/light/bh1745.c +++ b/drivers/iio/light/bh1745.c @@ -746,6 +746,8 @@ static irqreturn_t bh1745_trigger_handler(int interrupt, void *p) int i; int j = 0;
- memset(&scan, 0, sizeof(scan));
- iio_for_each_active_channel(indio_dev, i) { ret = regmap_bulk_read(data->regmap, BH1745_RED_LSB + 2 * i, &value, 2);
The 'buffer' local array is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Initialize the array to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 61fa5dfa5f52 ("iio: adc: ti-ads8688: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/adc/ti-ads8688.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c index 9b1814f1965a..a31658b760a4 100644 --- a/drivers/iio/adc/ti-ads8688.c +++ b/drivers/iio/adc/ti-ads8688.c @@ -381,7 +381,7 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p) struct iio_poll_func *pf = p; struct iio_dev *indio_dev = pf->indio_dev; /* Ensure naturally aligned timestamp */ - u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8); + u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8) = { }; int i, j = 0;
iio_for_each_active_channel(indio_dev, i) {
On Mon, 25 Nov 2024 22:16:16 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'buffer' local array is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Initialize the array to zero before using it to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 61fa5dfa5f52 ("iio: adc: ti-ads8688: Fix alignment of buffer in iio_push_to_buffers_with_timestamp()") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Applied.
drivers/iio/adc/ti-ads8688.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c index 9b1814f1965a..a31658b760a4 100644 --- a/drivers/iio/adc/ti-ads8688.c +++ b/drivers/iio/adc/ti-ads8688.c @@ -381,7 +381,7 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p) struct iio_poll_func *pf = p; struct iio_dev *indio_dev = pf->indio_dev; /* Ensure naturally aligned timestamp */
- u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8);
- u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)] __aligned(8) = { }; int i, j = 0;
iio_for_each_active_channel(indio_dev, i) {
The 'data' array is allocated via kmalloc() and it is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Use kzalloc for the memory allocation to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 415f79244757 ("iio: Move IIO Dummy Driver out of staging") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/dummy/iio_simple_dummy_buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c index 4ca3f1aaff99..288880346707 100644 --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c @@ -48,7 +48,7 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) int i = 0, j; u16 *data;
- data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); + data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL); if (!data) goto done;
On Mon, 25 Nov 2024 22:16:17 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'data' array is allocated via kmalloc() and it is used to push data to user space from a triggered buffer, but it does not set values for inactive channels, as it only uses iio_for_each_active_channel() to assign new values.
Use kzalloc for the memory allocation to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 415f79244757 ("iio: Move IIO Dummy Driver out of staging") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Oops. Got it wrong even in the example driver.
Applied to the fixes-togreg branch of iio.git.
Thanks,
Jonathan
drivers/iio/dummy/iio_simple_dummy_buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c index 4ca3f1aaff99..288880346707 100644 --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c @@ -48,7 +48,7 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) int i = 0, j; u16 *data;
- data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
- data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL); if (!data) goto done;
The 'scan' local struct is used to push data to userspace from a triggered buffer, but it leaves the first channel uninitialized if AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel readings.
Set the temperature channel to zero if only color channels are relevant to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 403e5586b52e ("iio: light: as73211: New driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/iio/light/as73211.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..99679b686146 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); if (ret < 0) goto done; + + /* Avoid leaking uninitialized data */ + scan.chan[0] = 0; }
if (data_result) {
On Mon, 25 Nov 2024 22:16:18 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'scan' local struct is used to push data to userspace from a triggered buffer, but it leaves the first channel uninitialized if AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel readings.
Set the temperature channel to zero if only color channels are relevant to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 403e5586b52e ("iio: light: as73211: New driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Huh.
If the temperature channel is turned off the data should shift. So should be read into scan.chan[0] and [1] and [2], but not [3].
Not skipping [0] as here.
So this code path currently doesn't work as far as I can tell.
Jonathan
drivers/iio/light/as73211.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..99679b686146 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); if (ret < 0) goto done;
/* Avoid leaking uninitialized data */scan.chan[0] = 0;if (data_result) {
On 30/11/2024 21:49, Jonathan Cameron wrote:
On Mon, 25 Nov 2024 22:16:18 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'scan' local struct is used to push data to userspace from a triggered buffer, but it leaves the first channel uninitialized if AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel readings.
Set the temperature channel to zero if only color channels are relevant to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 403e5586b52e ("iio: light: as73211: New driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Huh.
If the temperature channel is turned off the data should shift. So should be read into scan.chan[0] and [1] and [2], but not [3].
Not skipping [0] as here.
So this code path currently doesn't work as far as I can tell.
Jonathan
drivers/iio/light/as73211.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..99679b686146 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); if (ret < 0) goto done;
/* Avoid leaking uninitialized data */scan.chan[0] = 0;if (data_result) {
Adding the driver maintainer (should have been added from the beginning) to the conversation.
@Christian, could you please confirm this?
Apparently, the optimization to read the color channels without temperature is not right. I don't have access to the AS7331 at the moment, but I remember that you could test my patches on your hardware with an AS73211, so maybe you can confirm whether wrong data is delivered or not in that case.
Thanks and best regards, Javier Carrasco
Hi Jonathan, hi Javier,
On Monday, 2 December 2024, 16:38:50 CET, Javier Carrasco wrote:
On 30/11/2024 21:49, Jonathan Cameron wrote:
On Mon, 25 Nov 2024 22:16:18 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'scan' local struct is used to push data to userspace from a triggered buffer, but it leaves the first channel uninitialized if AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel readings.
Set the temperature channel to zero if only color channels are relevant to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 403e5586b52e ("iio: light: as73211: New driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Huh.
If the temperature channel is turned off the data should shift. So should be read into scan.chan[0] and [1] and [2], but not [3].
Not skipping [0] as here.
So this code path currently doesn't work as far as I can tell.
I've just tested and you are right! In our application we never had the case that we didn't read the temperature channel. If I don't enable scan_elements/in_temp_en, I need to put the data into scan.chan[0..2] in order to get correct values in my application. This also means that the "Optimization for reading only color channel" (and the following saturation block) isn't correct at all, especially if reading only one or two of the available channels.
Jonathan
drivers/iio/light/as73211.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..99679b686146 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); if (ret < 0) goto done;
/* Avoid leaking uninitialized data */scan.chan[0] = 0;if (data_result) {
Adding the driver maintainer (should have been added from the beginning) to the conversation.
@Christian, could you please confirm this?
Apparently, the optimization to read the color channels without temperature is not right. I don't have access to the AS7331 at the moment, but I remember that you could test my patches on your hardware with an AS73211, so maybe you can confirm whether wrong data is delivered or not in that case.
Yes, the delivered data is wrong (as already stated above).
@Javier: If you like to rework this, I can test your patches (I have still access to the hardware). Otherwise I can also try to fix this on my own.
Thanks and best regards, Javier Carrasco
Thanks for reporting this! Christian
On 02/12/2024 19:00, Christian Eggers wrote:
Hi Jonathan, hi Javier,
On Monday, 2 December 2024, 16:38:50 CET, Javier Carrasco wrote:
On 30/11/2024 21:49, Jonathan Cameron wrote:
On Mon, 25 Nov 2024 22:16:18 +0100 Javier Carrasco javier.carrasco.cruz@gmail.com wrote:
The 'scan' local struct is used to push data to userspace from a triggered buffer, but it leaves the first channel uninitialized if AS73211_SCAN_MASK_ALL is not set. That is used to optimize color channel readings.
Set the temperature channel to zero if only color channels are relevant to avoid pushing uninitialized information to userspace.
Cc: stable@vger.kernel.org Fixes: 403e5586b52e ("iio: light: as73211: New driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com
Huh.
If the temperature channel is turned off the data should shift. So should be read into scan.chan[0] and [1] and [2], but not [3].
Not skipping [0] as here.
So this code path currently doesn't work as far as I can tell.
I've just tested and you are right! In our application we never had the case that we didn't read the temperature channel. If I don't enable scan_elements/in_temp_en, I need to put the data into scan.chan[0..2] in order to get correct values in my application. This also means that the "Optimization for reading only color channel" (and the following saturation block) isn't correct at all, especially if reading only one or two of the available channels.
Jonathan
drivers/iio/light/as73211.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..99679b686146 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -675,6 +675,9 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); if (ret < 0) goto done;
/* Avoid leaking uninitialized data */scan.chan[0] = 0;if (data_result) {
Adding the driver maintainer (should have been added from the beginning) to the conversation.
@Christian, could you please confirm this?
Apparently, the optimization to read the color channels without temperature is not right. I don't have access to the AS7331 at the moment, but I remember that you could test my patches on your hardware with an AS73211, so maybe you can confirm whether wrong data is delivered or not in that case.
Yes, the delivered data is wrong (as already stated above).
@Javier: If you like to rework this, I can test your patches (I have still access to the hardware). Otherwise I can also try to fix this on my own.
Thanks and best regards, Javier Carrasco
Thanks for reporting this! Christian
Thanks for your prompt reply. I will rework it for v2, as the current patch does not apply. For this path, scan.chan[0]..scan.chan[2] will be read from the sensor, and scan.chan[3] will be set to zero.
Best regards, Javier Carrasco
linux-stable-mirror@lists.linaro.org
-
Christian Eggers -
Francesco Dolcini -
Javier Carrasco -
Jonathan Cameron