Re: [PATCH v8 4/4] iio: light: veml6031x00: add support for events and trigger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free of iio->pollfunc in IRQ handler during device unbind or probe failure.
- [Medium] Unintended I2C read of configuration register when pushing timestamp to buffer
- [Medium] The interrupt_users counter can permanently underflow during error recovery, breaking hardware interrupt control.
--

commit c87f3d48e1ee614fc184995b816c8f8679a6b774
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

iio: light: veml6031x00: add support for events and trigger

The device provides a shared interrupt line to notify events and data ready.
Add support for configurations with and without an interrupt line, providing
events and trigger support when an interrupt line is available.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 3d5a27a0baaed..7bd7975eb1a76 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c
[ ... ]
> +static irqreturn_t veml6031x00_irq(int irq, void *private)
> +{
[ ... ]
> +	if (trigger_poll) {
> +		iio->pollfunc->timestamp = timestamp;
> +		iio_trigger_poll_nested(data->trig);
> +	}
> +
> +	pm_runtime_put(dev);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Could this lead to a use-after-free of the pollfunc during device unbind or
if probe fails?

Because the IRQ is requested via devres inside veml6031x00_init_iiodev()
before the triggered buffer is set up in veml6031x00_probe(), devres will
tear down the buffer (and free iio->pollfunc) before freeing the IRQ. If an
interrupt fires during this teardown window, the IRQ thread could be
preempted and then dereference the freed iio->pollfunc here.

Would it be safer to ensure devm_iio_triggered_buffer_setup() is called
before the IRQ is requested?

[ ... ]
> +static int __veml6031x00_set_trigger_state(struct veml6031x00_data *data, bool state)
> +	__must_hold(&data->irq_lock)
> +{
[ ... ]
> +	data->trig_en = state;
> +
> +	return 0;
> +
> +clear_interrupt:
> +	veml6031x00_set_interrupt(data, !state);
> +
> +	return ret;
> +}

[Severity: Medium]
Can data->interrupt_users permanently underflow here during error recovery?

If an I2C error occurs in __veml6031x00_set_trigger_state() while disabling
the trigger, this error recovery path calls veml6031x00_set_interrupt() to
revert the counter, but fails to update the software state (data->trig_en).

If a subsequent successful disable happens later, the counter will be
decremented again, potentially underflowing to UINT_MAX and silently skipping
all future hardware writes.

[ ... ]
> +static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	struct regmap *map = data->regmap;
> +	IIO_DECLARE_BUFFER_WITH_TS(__le16, scan, 2) = { };
> +	unsigned int i;
> +	int ch, ret;
> +
> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {
> +		ret = regmap_bulk_read(map, VEML6031X00_REG_ALS_L,
> +				       scan, 2 * sizeof(*scan));
> +		if (ret)
> +			goto done;
> +	} else {
> +		i = 0;
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(map, iio->channels[ch].address,
> +					       &scan[i++], sizeof(*scan));
> +			if (ret)
> +				goto done;
> +		}
> +	}

[Severity: Medium]
Does this loop unintentionally perform an I2C read of the configuration
register when pushing the timestamp to the buffer?

The iio_for_each_active_channel() iteration includes the IIO_TIMESTAMP
channel. Since its address is 0, this causes an unintended I2C read of the
configuration register (0x00) on every trigger event when the IR channel is
disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-veml6031x00-v8-0-532cb4f2168a@gmail.com?part=4