On Sun, 10 Nov 2024 18:49:05 +0100
Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> 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@xxxxxxxxx>
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),
> + ®);
> + 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,
