All devices supported by this driver (currently veml6030, veml6035
and veml7700) have two 16-bit channels, and can profit for the same
configuration to support data access via triggered buffers.

The measurements are stored in two 16-bit consecutive registers
(addresses 0x04 and 0x05) as little endian, unsigned data.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Changes in v2:
- Use u16 instead of __le16 for the channels.
- Move scan struct from data to trigger_handler.
- Drop avail_scan_masks.
- Fix scan_type (IIO_CPU instead of IIO_LE, drop shift = 0).
- Link to v1: https://lore.kernel.org/r/20241107-veml6030_triggered_buffer-v1-1-4810ab86cc56@gmail.com
---
 drivers/iio/light/Kconfig    |  2 ++
 drivers/iio/light/veml6030.c | 74 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 29ffa8491927..0e2566ff5f7b 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -683,6 +683,8 @@ config VEML3235
 config VEML6030
 	tristate "VEML6030 and VEML6035 ambient light sensors"
 	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	depends on I2C
 	help
 	  Say Y here if you want to build a driver for the Vishay VEML6030
diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
index ccb43dfd5cf7..ce9af9a0e933 100644
--- a/drivers/iio/light/veml6030.c
+++ b/drivers/iio/light/veml6030.c
@@ -28,6 +28,8 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 /* Device registers */
 #define VEML6030_REG_ALS_CONF   0x00
@@ -37,6 +39,7 @@
 #define VEML6030_REG_ALS_DATA   0x04
 #define VEML6030_REG_WH_DATA    0x05
 #define VEML6030_REG_ALS_INT    0x06
+#define VEML6030_REG_DATA(ch)   (VEML6030_REG_ALS_DATA + (ch))
 
 /* Bit masks for specific functionality */
 #define VEML6030_ALS_IT       GENMASK(9, 6)
@@ -56,6 +59,12 @@
 #define VEML6035_INT_CHAN     BIT(3)
 #define VEML6035_CHAN_EN      BIT(2)
 
+enum veml6030_scan {
+	VEML6030_SCAN_ALS,
+	VEML6030_SCAN_WH,
+	VEML6030_SCAN_TIMESTAMP,
+};
+
 struct veml603x_chip {
 	const char *name;
 	const int(*scale_vals)[][2];
@@ -242,6 +251,13 @@ static const struct iio_chan_spec veml6030_channels[] = {
 						     BIT(IIO_CHAN_INFO_SCALE),
 		.event_spec = veml6030_event_spec,
 		.num_event_specs = ARRAY_SIZE(veml6030_event_spec),
+		.scan_index = VEML6030_SCAN_ALS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -253,7 +269,15 @@ static const struct iio_chan_spec veml6030_channels[] = {
 				BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
 						     BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = VEML6030_SCAN_WH,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(VEML6030_SCAN_TIMESTAMP),
 };
 
 static const struct iio_chan_spec veml7700_channels[] = {
@@ -266,6 +290,13 @@ static const struct iio_chan_spec veml7700_channels[] = {
 				BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
 						     BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = VEML6030_SCAN_ALS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -277,7 +308,15 @@ static const struct iio_chan_spec veml7700_channels[] = {
 				BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
 						     BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = VEML6030_SCAN_WH,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(VEML6030_SCAN_TIMESTAMP),
 };
 
 static const struct regmap_config veml6030_regmap_config = {
@@ -889,6 +928,35 @@ static irqreturn_t veml6030_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t veml6030_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *iio = pf->indio_dev;
+	struct veml6030_data *data = iio_priv(iio);
+	unsigned int reg;
+	int ch, ret, i = 0;
+	struct {
+		u16 chans[2];
+		aligned_s64 timestamp;
+	} scan;
+
+	iio_for_each_active_channel(iio, ch) {
+		ret = regmap_read(data->regmap, VEML6030_REG_DATA(ch),
+				  &reg);
+		if (ret)
+			goto done;
+
+		scan.chans[i++] = reg;
+	}
+
+	iio_push_to_buffers_with_timestamp(iio, &scan, pf->timestamp);
+
+done:
+	iio_trigger_notify_done(iio->trig);
+
+	return IRQ_HANDLED;
+}
+
 static int veml6030_set_info(struct iio_dev *indio_dev)
 {
 	struct veml6030_data *data = iio_priv(indio_dev);
@@ -1077,6 +1145,12 @@ static int veml6030_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+					      veml6030_trigger_handler, NULL);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to register triggered buffer");
+
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 

---
base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
change-id: 20241106-veml6030_triggered_buffer-a38886ca4cce

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

On Sun, 10 Nov 2024 18:49:05 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> All devices supported by this driver (currently veml6030, veml6035
> and veml7700) have two 16-bit channels, and can profit for the same
> configuration to support data access via triggered buffers.
> 
> The measurements are stored in two 16-bit consecutive registers
> (addresses 0x04 and 0x05) as little endian, unsigned data.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Hi Javier,

We have to be a little careful with pushing data from the stack.
Need to makes sure holes are zero filled.

Jonathan

> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> index ccb43dfd5cf7..ce9af9a0e933 100644
> --- a/drivers/iio/light/veml6030.c
> +++ b/drivers/iio/light/veml6030.c

>  
>  static const struct regmap_config veml6030_regmap_config = {
> @@ -889,6 +928,35 @@ static irqreturn_t veml6030_event_handler(int irq, void *private)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t veml6030_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6030_data *data = iio_priv(iio);
> +	unsigned int reg;
> +	int ch, ret, i = 0;
> +	struct {
> +		u16 chans[2];
There is a hole here... 
> +		aligned_s64 timestamp;
> +	} scan;
> +
> +	iio_for_each_active_channel(iio, ch) {
> +		ret = regmap_read(data->regmap, VEML6030_REG_DATA(ch),
> +				  &reg);
> +		if (ret)
> +			goto done;
> +
> +		scan.chans[i++] = reg;
This fills in at least 1 channel, but maybe not the second.
> +	}
> +
So this leaks random stack data I think.

Upshot, when holes are involved or not all the channels are set, need
memset(&scan, 0, sizeof(scan));
for the structure on the stack which will zero the holes as well as
both channels.

Ancient article on this: https://lwn.net/Articles/417989/

We get away with it when they are in the iio_priv space because they are
kzalloc + if we do leak data due to changes in configured channels it's
just old sensor data which is (I think) never a security problem!

> +	iio_push_to_buffers_with_timestamp(iio, &scan, pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int veml6030_set_info(struct iio_dev *indio_dev)
>  {
>  	struct veml6030_data *data = iio_priv(indio_dev);
> @@ -1077,6 +1145,12 @@ static int veml6030_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> +					      veml6030_trigger_handler, NULL);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to register triggered buffer");
> +
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  
> 
> ---
> base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
> change-id: 20241106-veml6030_triggered_buffer-a38886ca4cce
> 
> Best regards,

On 23/11/2024 16:16, Jonathan Cameron wrote:
> On Sun, 10 Nov 2024 18:49:05 +0100
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> 
>> All devices supported by this driver (currently veml6030, veml6035
>> and veml7700) have two 16-bit channels, and can profit for the same
>> configuration to support data access via triggered buffers.
>>
>> The measurements are stored in two 16-bit consecutive registers
>> (addresses 0x04 and 0x05) as little endian, unsigned data.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Hi Javier,
> 
> We have to be a little careful with pushing data from the stack.
> Need to makes sure holes are zero filled.
> 
> Jonathan
> 
>> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
>> index ccb43dfd5cf7..ce9af9a0e933 100644
>> --- a/drivers/iio/light/veml6030.c
>> +++ b/drivers/iio/light/veml6030.c
> 
>>  
>>  static const struct regmap_config veml6030_regmap_config = {
>> @@ -889,6 +928,35 @@ static irqreturn_t veml6030_event_handler(int irq, void *private)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static irqreturn_t veml6030_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *iio = pf->indio_dev;
>> +	struct veml6030_data *data = iio_priv(iio);
>> +	unsigned int reg;
>> +	int ch, ret, i = 0;
>> +	struct {
>> +		u16 chans[2];
> There is a hole here... 
>> +		aligned_s64 timestamp;
>> +	} scan;
>> +
>> +	iio_for_each_active_channel(iio, ch) {
>> +		ret = regmap_read(data->regmap, VEML6030_REG_DATA(ch),
>> +				  &reg);
>> +		if (ret)
>> +			goto done;
>> +
>> +		scan.chans[i++] = reg;
> This fills in at least 1 channel, but maybe not the second.
>> +	}
>> +
> So this leaks random stack data I think.
> 
> Upshot, when holes are involved or not all the channels are set, need
> memset(&scan, 0, sizeof(scan));
> for the structure on the stack which will zero the holes as well as
> both channels.
> 
> Ancient article on this: https://lwn.net/Articles/417989/
> 
> We get away with it when they are in the iio_priv space because they are
> kzalloc + if we do leak data due to changes in configured channels it's
> just old sensor data which is (I think) never a security problem!
> 
>> +	iio_push_to_buffers_with_timestamp(iio, &scan, pf->timestamp);
>> +
>> +done:
>> +	iio_trigger_notify_done(iio->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int veml6030_set_info(struct iio_dev *indio_dev)
>>  {
>>  	struct veml6030_data *data = iio_priv(indio_dev);
>> @@ -1077,6 +1145,12 @@ static int veml6030_probe(struct i2c_client *client)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>> +					      veml6030_trigger_handler, NULL);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret,
>> +				     "Failed to register triggered buffer");
>> +
>>  	return devm_iio_device_register(&client->dev, indio_dev);
>>  }
>>  
>>
>> ---
>> base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
>> change-id: 20241106-veml6030_triggered_buffer-a38886ca4cce
>>
>> Best regards,
> 


Hi Jonathan,

thanks a lot for your explanation and the link, it makes perfect sense.
By the way, when I moved this struct from the iio_priv to the function,
I took a look at some existing code, and a couple of them might have the
same issue:

- temperature/tmp006.c: it also has a hole between the two 16-bit
channels and the timestamp (aligned(8)), but it is not set to zero.

- adc/ti-ads1119.c: the scan consists of an unsigned int and the
timestamp (aligned(8)). I believe there is a hole there as well.

I did not go over all drivers (most of them store the scan struct in the
iio_priv space anyway), but at least those two look suspicious.

Should I fix (e.g. memset) those two I mentioned?

Best regards,
Javier Carrasco

On Sat, 23 Nov 2024 22:15:11 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> On 23/11/2024 16:16, Jonathan Cameron wrote:
> > On Sun, 10 Nov 2024 18:49:05 +0100
> > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> >   
> >> All devices supported by this driver (currently veml6030, veml6035
> >> and veml7700) have two 16-bit channels, and can profit for the same
> >> configuration to support data access via triggered buffers.
> >>
> >> The measurements are stored in two 16-bit consecutive registers
> >> (addresses 0x04 and 0x05) as little endian, unsigned data.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> > Hi Javier,
> > 
> > We have to be a little careful with pushing data from the stack.
> > Need to makes sure holes are zero filled.
> > 
> > Jonathan
> >   
> >> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> >> index ccb43dfd5cf7..ce9af9a0e933 100644
> >> --- a/drivers/iio/light/veml6030.c
> >> +++ b/drivers/iio/light/veml6030.c  
> >   
> >>  
> >>  static const struct regmap_config veml6030_regmap_config = {
> >> @@ -889,6 +928,35 @@ static irqreturn_t veml6030_event_handler(int irq, void *private)
> >>  	return IRQ_HANDLED;
> >>  }
> >>  
> >> +static irqreturn_t veml6030_trigger_handler(int irq, void *p)
> >> +{
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *iio = pf->indio_dev;
> >> +	struct veml6030_data *data = iio_priv(iio);
> >> +	unsigned int reg;
> >> +	int ch, ret, i = 0;
> >> +	struct {
> >> +		u16 chans[2];  
> > There is a hole here...   
> >> +		aligned_s64 timestamp;
> >> +	} scan;
> >> +
> >> +	iio_for_each_active_channel(iio, ch) {
> >> +		ret = regmap_read(data->regmap, VEML6030_REG_DATA(ch),
> >> +				  &reg);
> >> +		if (ret)
> >> +			goto done;
> >> +
> >> +		scan.chans[i++] = reg;  
> > This fills in at least 1 channel, but maybe not the second.  
> >> +	}
> >> +  
> > So this leaks random stack data I think.
> > 
> > Upshot, when holes are involved or not all the channels are set, need
> > memset(&scan, 0, sizeof(scan));
> > for the structure on the stack which will zero the holes as well as
> > both channels.
> > 
> > Ancient article on this: https://lwn.net/Articles/417989/
> > 
> > We get away with it when they are in the iio_priv space because they are
> > kzalloc + if we do leak data due to changes in configured channels it's
> > just old sensor data which is (I think) never a security problem!
> >   
> >> +	iio_push_to_buffers_with_timestamp(iio, &scan, pf->timestamp);
> >> +
> >> +done:
> >> +	iio_trigger_notify_done(iio->trig);
> >> +
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >>  static int veml6030_set_info(struct iio_dev *indio_dev)
> >>  {
> >>  	struct veml6030_data *data = iio_priv(indio_dev);
> >> @@ -1077,6 +1145,12 @@ static int veml6030_probe(struct i2c_client *client)
> >>  	if (ret < 0)
> >>  		return ret;
> >>  
> >> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> >> +					      veml6030_trigger_handler, NULL);
> >> +	if (ret)
> >> +		return dev_err_probe(&client->dev, ret,
> >> +				     "Failed to register triggered buffer");
> >> +
> >>  	return devm_iio_device_register(&client->dev, indio_dev);
> >>  }
> >>  
> >>
> >> ---
> >> base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
> >> change-id: 20241106-veml6030_triggered_buffer-a38886ca4cce
> >>
> >> Best regards,  
> >   
> 
> 
> Hi Jonathan,
> 
> thanks a lot for your explanation and the link, it makes perfect sense.
> By the way, when I moved this struct from the iio_priv to the function,
> I took a look at some existing code, and a couple of them might have the
> same issue:
> 
> - temperature/tmp006.c: it also has a hole between the two 16-bit
> channels and the timestamp (aligned(8)), but it is not set to zero.
> 
> - adc/ti-ads1119.c: the scan consists of an unsigned int and the
> timestamp (aligned(8)). I believe there is a hole there as well.
> 
> I did not go over all drivers (most of them store the scan struct in the
> iio_priv space anyway), but at least those two look suspicious.
> 
> Should I fix (e.g. memset) those two I mentioned?

Please do.  Thanks!

Jonathan


> 
> Best regards,
> Javier Carrasco
>

On 24/11/2024 13:43, Jonathan Cameron wrote:
>>>> +static irqreturn_t veml6030_trigger_handler(int irq, void *p)
>>>> +{
>>>> +	struct iio_poll_func *pf = p;
>>>> +	struct iio_dev *iio = pf->indio_dev;
>>>> +	struct veml6030_data *data = iio_priv(iio);
>>>> +	unsigned int reg;
>>>> +	int ch, ret, i = 0;
>>>> +	struct {
>>>> +		u16 chans[2];  
>>> There is a hole here...   
>>>> +		aligned_s64 timestamp;
>>>> +	} scan;
>>>> +
>>>> +	iio_for_each_active_channel(iio, ch) {
>>>> +		ret = regmap_read(data->regmap, VEML6030_REG_DATA(ch),
>>>> +				  &reg);
>>>> +		if (ret)
>>>> +			goto done;
>>>> +
>>>> +		scan.chans[i++] = reg;  
>>> This fills in at least 1 channel, but maybe not the second.  
>>>> +	}
>>>> +  
>>> So this leaks random stack data I think.
>>>
>>> Upshot, when holes are involved or not all the channels are set, need
>>> memset(&scan, 0, sizeof(scan));
>>> for the structure on the stack which will zero the holes as well as
>>> both channels.
>>>
>>> Ancient article on this: https://lwn.net/Articles/417989/
>>>
>>> We get away with it when they are in the iio_priv space because they are
>>> kzalloc + if we do leak data due to changes in configured channels it's
>>> just old sensor data which is (I think) never a security problem!
>>>   
>>>> +	iio_push_to_buffers_with_timestamp(iio, &scan, pf->timestamp);
>>>> +
>>>> +done:
>>>> +	iio_trigger_notify_done(iio->trig);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>>  static int veml6030_set_info(struct iio_dev *indio_dev)
>>>>  {
>>>>  	struct veml6030_data *data = iio_priv(indio_dev);
>>>> @@ -1077,6 +1145,12 @@ static int veml6030_probe(struct i2c_client *client)
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>>  
>>>> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>>>> +					      veml6030_trigger_handler, NULL);
>>>> +	if (ret)
>>>> +		return dev_err_probe(&client->dev, ret,
>>>> +				     "Failed to register triggered buffer");
>>>> +
>>>>  	return devm_iio_device_register(&client->dev, indio_dev);
>>>>  }
>>>>  
>>>>
>>>> ---
>>>> base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
>>>> change-id: 20241106-veml6030_triggered_buffer-a38886ca4cce
>>>>
>>>> Best regards,  
>>>   
>>
>>
>> Hi Jonathan,
>>
>> thanks a lot for your explanation and the link, it makes perfect sense.
>> By the way, when I moved this struct from the iio_priv to the function,
>> I took a look at some existing code, and a couple of them might have the
>> same issue:
>>
>> - temperature/tmp006.c: it also has a hole between the two 16-bit
>> channels and the timestamp (aligned(8)), but it is not set to zero.
>>
>> - adc/ti-ads1119.c: the scan consists of an unsigned int and the
>> timestamp (aligned(8)). I believe there is a hole there as well.
>>
>> I did not go over all drivers (most of them store the scan struct in the
>> iio_priv space anyway), but at least those two look suspicious.
>>
>> Should I fix (e.g. memset) those two I mentioned?
> 
> Please do.  Thanks!
> 
> Jonathan
> 

Ok, I will take a closer look to check if there are more drivers leaking
uninitialized data. By the way, would you tag the fixes for stable? This
becoming an attack vector might be a bit theoretical, and I don't know
the consensus about the danger of passing uninitialized data to userspace.

Thanks again,
Javier Carrasco

On Sun, 24 Nov 2024 15:47:23 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> On 24/11/2024 13:43, Jonathan Cameron wrote:
> >>>> +static irqreturn_t veml6030_trigger_handler(int irq, void *p)
> >>>> +{
> >>>> +	struct iio_poll_func *pf = p;
> >>>> +	struct iio_dev *iio = pf->indio_dev;
> >>>> +	struct veml6030_data *data = iio_priv(iio);
> >>>> +	unsigned int reg;
> >>>> +	int ch, ret, i = 0;
> >>>> +	struct {
> >>>> +		u16 chans[2];    
> >>> There is a hole here...     
> >>>> +		aligned_s64 timestamp;
> >>>> +	} scan;
> >>>> +
> >>>> +	iio_for_each_active_channel(iio, ch) {
> >>>> +		ret = regmap_read(data->regmap, VEML6030_REG_DATA(ch),
> >>>> +				  &reg);
> >>>> +		if (ret)
> >>>> +			goto done;
> >>>> +
> >>>> +		scan.chans[i++] = reg;    
> >>> This fills in at least 1 channel, but maybe not the second.    
> >>>> +	}
> >>>> +    
> >>> So this leaks random stack data I think.
> >>>
> >>> Upshot, when holes are involved or not all the channels are set, need
> >>> memset(&scan, 0, sizeof(scan));
> >>> for the structure on the stack which will zero the holes as well as
> >>> both channels.
> >>>
> >>> Ancient article on this: https://lwn.net/Articles/417989/
> >>>
> >>> We get away with it when they are in the iio_priv space because they are
> >>> kzalloc + if we do leak data due to changes in configured channels it's
> >>> just old sensor data which is (I think) never a security problem!
> >>>     
> >>>> +	iio_push_to_buffers_with_timestamp(iio, &scan, pf->timestamp);
> >>>> +
> >>>> +done:
> >>>> +	iio_trigger_notify_done(iio->trig);
> >>>> +
> >>>> +	return IRQ_HANDLED;
> >>>> +}
> >>>> +
> >>>>  static int veml6030_set_info(struct iio_dev *indio_dev)
> >>>>  {
> >>>>  	struct veml6030_data *data = iio_priv(indio_dev);
> >>>> @@ -1077,6 +1145,12 @@ static int veml6030_probe(struct i2c_client *client)
> >>>>  	if (ret < 0)
> >>>>  		return ret;
> >>>>  
> >>>> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> >>>> +					      veml6030_trigger_handler, NULL);
> >>>> +	if (ret)
> >>>> +		return dev_err_probe(&client->dev, ret,
> >>>> +				     "Failed to register triggered buffer");
> >>>> +
> >>>>  	return devm_iio_device_register(&client->dev, indio_dev);
> >>>>  }
> >>>>  
> >>>>
> >>>> ---
> >>>> base-commit: 9dd2270ca0b38ee16094817f4a53e7ba78e31567
> >>>> change-id: 20241106-veml6030_triggered_buffer-a38886ca4cce
> >>>>
> >>>> Best regards,    
> >>>     
> >>
> >>
> >> Hi Jonathan,
> >>
> >> thanks a lot for your explanation and the link, it makes perfect sense.
> >> By the way, when I moved this struct from the iio_priv to the function,
> >> I took a look at some existing code, and a couple of them might have the
> >> same issue:
> >>
> >> - temperature/tmp006.c: it also has a hole between the two 16-bit
> >> channels and the timestamp (aligned(8)), but it is not set to zero.
> >>
> >> - adc/ti-ads1119.c: the scan consists of an unsigned int and the
> >> timestamp (aligned(8)). I believe there is a hole there as well.
> >>
> >> I did not go over all drivers (most of them store the scan struct in the
> >> iio_priv space anyway), but at least those two look suspicious.
> >>
> >> Should I fix (e.g. memset) those two I mentioned?  
> > 
> > Please do.  Thanks!
> > 
> > Jonathan
> >   
> 
> Ok, I will take a closer look to check if there are more drivers leaking
> uninitialized data. By the way, would you tag the fixes for stable? This
> becoming an attack vector might be a bit theoretical, and I don't know
> the consensus about the danger of passing uninitialized data to userspace.

Yes. Stable would be appropriate for these. Thanks for looking into it.
Your patch had me adding checking this to my todo list but I'm very
happy to have you do it instead! :)


Jonathan

> 
> Thanks again,
> Javier Carrasco