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 --- Changes in v2: - as73211.c: shift channels if no temperature is available and initialize chan[3] to zero. - Link to v1: https://lore.kernel.org/r/20241125-iio_memset_scan_holes-v1-0-0cb6e98d895c@g...
--- Javier Carrasco (2): iio: temperature: tmp006: fix information leak in triggered buffer iio: light: as73211: fix channel handling in only-color triggered buffer
drivers/iio/light/as73211.c | 17 +++++++++++++---- drivers/iio/temperature/tmp006.c | 2 ++ 2 files changed, 15 insertions(+), 4 deletions(-) --- base-commit: 1694dea95b02eff1a64c893ffee4626df533b2ab 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 Wed, 04 Dec 2024 00:55:31 +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 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
Applied but dropped the stable tag. The patch this is fixing isn't in a release yet.
Jonathan
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;
The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not set (optimized path for color channel readings), and it must be shifted instead of leaving an empty channel for the temperature when it is off.
Once the channel index is fixed, the uninitialized channel must be set to zero to avoid pushing uninitialized data.
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 | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..2d45dfeda406 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -672,9 +672,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
/* AS73211 starts reading at address 2 */ ret = i2c_master_recv(data->client, - (char *)&scan.chan[1], 3 * sizeof(scan.chan[1])); + (char *)&scan.chan[0], 3 * sizeof(scan.chan[0])); if (ret < 0) goto done; + + /* Avoid pushing uninitialized data */ + scan.chan[3] = 0; }
if (data_result) { @@ -682,9 +685,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) * Saturate all channels (in case of overflows). Temperature channel * is not affected by overflows. */ - scan.chan[1] = cpu_to_le16(U16_MAX); - scan.chan[2] = cpu_to_le16(U16_MAX); - scan.chan[3] = cpu_to_le16(U16_MAX); + if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) { + scan.chan[1] = cpu_to_le16(U16_MAX); + scan.chan[2] = cpu_to_le16(U16_MAX); + scan.chan[3] = cpu_to_le16(U16_MAX); + } else { + scan.chan[0] = cpu_to_le16(U16_MAX); + scan.chan[1] = cpu_to_le16(U16_MAX); + scan.chan[2] = cpu_to_le16(U16_MAX); + } }
iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
On Wednesday, 4 December 2024, 00:55:32 CET, Javier Carrasco wrote:
The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not set (optimized path for color channel readings), and it must be shifted instead of leaving an empty channel for the temperature when it is off.
Once the channel index is fixed, the uninitialized channel must be set to zero to avoid pushing uninitialized data.
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 | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..2d45dfeda406 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -672,9 +672,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) /* AS73211 starts reading at address 2 */ ret = i2c_master_recv(data->client,
(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
(char *)&scan.chan[0], 3 * sizeof(scan.chan[0]));/* Avoid pushing uninitialized data */scan.chan[3] = 0;if (data_result) { @@ -682,9 +685,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) * Saturate all channels (in case of overflows). Temperature channel * is not affected by overflows. */
scan.chan[1] = cpu_to_le16(U16_MAX);scan.chan[2] = cpu_to_le16(U16_MAX);scan.chan[3] = cpu_to_le16(U16_MAX);
if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {scan.chan[1] = cpu_to_le16(U16_MAX);scan.chan[2] = cpu_to_le16(U16_MAX);scan.chan[3] = cpu_to_le16(U16_MAX);} else {scan.chan[0] = cpu_to_le16(U16_MAX);scan.chan[1] = cpu_to_le16(U16_MAX);scan.chan[2] = cpu_to_le16(U16_MAX);}iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
With this change, having only X, Y and Z in the scan_mask (without the temperature channel) works fine.
But it looks that there is still another problem if a single color channel (e.g. X) is omitted from the scan mask (which probably wouldn't make much sense in practice). If I am right, the layout of scan.chan[] is also wrong for that case, so e.g. if omitting X, the application will get the X values where it expects the temperature value (which isn't read from the hardware at all).
Does it make sense to write a follow-up patch for this? I fear that taking all possible combinations into account could make the driver more complicated than necessary. Or is there a good example how to handle such combinations?
Tested-by: Christian Eggers ceggers@arri.de
On Wed, 4 Dec 2024 17:20:47 +0100 Christian Eggers ceggers@arri.de wrote:
On Wednesday, 4 December 2024, 00:55:32 CET, Javier Carrasco wrote:
The channel index is off by one unit if AS73211_SCAN_MASK_ALL is not set (optimized path for color channel readings), and it must be shifted instead of leaving an empty channel for the temperature when it is off.
Once the channel index is fixed, the uninitialized channel must be set to zero to avoid pushing uninitialized data.
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 | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c index be0068081ebb..2d45dfeda406 100644 --- a/drivers/iio/light/as73211.c +++ b/drivers/iio/light/as73211.c @@ -672,9 +672,12 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) /* AS73211 starts reading at address 2 */ ret = i2c_master_recv(data->client,
(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
(char *)&scan.chan[0], 3 * sizeof(scan.chan[0]));/* Avoid pushing uninitialized data */scan.chan[3] = 0;if (data_result) { @@ -682,9 +685,15 @@ static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p) * Saturate all channels (in case of overflows). Temperature channel * is not affected by overflows. */
scan.chan[1] = cpu_to_le16(U16_MAX);scan.chan[2] = cpu_to_le16(U16_MAX);scan.chan[3] = cpu_to_le16(U16_MAX);
if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {scan.chan[1] = cpu_to_le16(U16_MAX);scan.chan[2] = cpu_to_le16(U16_MAX);scan.chan[3] = cpu_to_le16(U16_MAX);} else {scan.chan[0] = cpu_to_le16(U16_MAX);scan.chan[1] = cpu_to_le16(U16_MAX);scan.chan[2] = cpu_to_le16(U16_MAX);}iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
With this change, having only X, Y and Z in the scan_mask (without the temperature channel) works fine.
But it looks that there is still another problem if a single color channel (e.g. X) is omitted from the scan mask (which probably wouldn't make much sense in practice). If I am right, the layout of scan.chan[] is also wrong for that case, so e.g. if omitting X, the application will get the X values where it expects the temperature value (which isn't read from the hardware at all).
Does it make sense to write a follow-up patch for this? I fear that taking all possible combinations into account could make the driver more complicated than necessary. Or is there a good example how to handle such combinations?
Good spot. I'd fallen for assuming a driver worked the way I thought it would and not checked everything necessary was there.
Hmm. This is a bit odd. Driver seems to be written with assumption that the IIO core is doing demux. That doesn't work unless available_scan_masks is set.
Make that { AS73211_SCAN_MASK_ALL, AS73211_SCAN_MASK_COLOR, 0, };
And then if you enable fewer channels, the IIO core will still enable one of the sets in available_scan_masks and then do the relevant data manipulation to repack as necessary.
I'll not pick this patch up as it makes sense to fix both issues together.
Thanks
Jonathan
Tested-by: Christian Eggers ceggers@arri.de
linux-stable-mirror@lists.linaro.org
-
Christian Eggers -
Javier Carrasco -
Jonathan Cameron