Re: [PATCH v2] iio: light: veml6030: add support for triggered buffer

[Jonathan Cameron <jic23@kernel.org>]

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,