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

["Javier Carrasco" <javier.carrasco.cruz@gmail.com>]

On Tue Sep 1, 2026 at 3:22 PM CEST, sashiko-bot wrote:
> 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?
>

I will register the IRQ right after calling
devm_iio_triggered_buffer_setup(), removing it from
veml6031x00_init_iiodev(). It was there not to check i2c->irq twice,
because that is also necessary to properly initialize the IIO device
with the right data, but it is true that the tear down is not clean at
the moment.

> [ ... ]
>> +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.
>
> [ ... ]

That is not true because the error path will *increment* the counter to
re-enable the IRQ and keept it consistent with data->trig_en. If an I2C
operation fails, the trigger will stay enabled in the device registers,
and therefore data-trig_en should stay enabled.

I tested it by adding some hacks to force that situation, and what may
actually happen is that if *on top* of that failure, the I2C operation
in veml6031x00_set_interrupt() also fails, interrupt_users will be
decremented (as it should, because the interrupt could not be
re-enabled), and *then* a second call to disable the trigger will set
interrupt_users to UINT_MAX.

Although this is very unlikely, the fix seems to be trivial:

 if (state) {
 	data->interrupt_users++;
 	if (data->interrupt_users > 1)
 		return 0;
 } else {
 	+	if (!data->interrupt_users)
 	+		return 0;

 	data->interrupt_users--;
 	if (data->interrupt_users > 0)
 		return 0;
 }

Note that this fix is not a hack to just get rid of reaching UINT_MAX, it
reflects the fact that the previous failed operations lead to a disabled
interrupt and an enabled trigger in the hardware, and there is no need
to act on the interrupt again when trying to disable the trigger a
second time.

I will add it for V9 after testing it with some hacks to force that
scenario.

Best regards,
Javier