[PATCH v2 0/2] iio: fix information leaks in triggered buffers

[PATCH v2 0/2] iio: fix information leaks in triggered buffers

[Javier Carrasco]

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@gmail.com

---
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,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH v2 1/2] iio: temperature: tmp006: fix information leak in triggered buffer

[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 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;

-- 
2.43.0

[PATCH v2 2/2] iio: light: as73211: fix channel handling in only-color triggered buffer

[Javier Carrasco]

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));

-- 
2.43.0

Re: [PATCH v2 2/2] iio: light: as73211: fix channel handling in only-color triggered buffer

[Christian Eggers]

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]));
>  		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));
> 
> 

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>

Re: [PATCH v2 1/2] iio: temperature: tmp006: fix information leak in triggered buffer

[Jonathan Cameron]

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;
> 

Re: [PATCH v2 2/2] iio: light: as73211: fix channel handling in only-color triggered buffer

[Jonathan Cameron]

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]));
> >  		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));
> > 
> >   
> 
> 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>
> 
> 
> 
>