Re: [PATCH v2 4/4] iio: light: add support for veml6031x00 ALS series

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Race Condition and API Violation in `veml6031x00_write_raw`
- [High] Out-of-Bounds Array Access in Trigger Handler
- [High] PM Runtime Usage Counter Underflow and Race Condition in `veml6031x00_write_event_config`
- [High] TOCTOU Race and Lockless Hardware State Rollback in `veml6031x00_set_trigger_state`
- [High] Sensor Left in Hardware Shutdown During Initial Active PM State
- [Medium] PM Runtime Usage Counter Leak on Probe Error Paths
- [Medium] Negative Input Values Bypass Period Validation
- [Medium] `iio_device_claim_direct` Blocks Needlessly Due to Long Sleep in `veml6031x00_single_read`
--

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

iio: light: add support for veml6031x00 ALS series

These sensors provide two light channels (ALS and IR), I2C communication
and a multiplexed interrupt line to signal data ready and configurable
threshold alarms.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> new file mode 100644
> index 0000000000000..c7808768f45ad
> --- /dev/null
> +++ b/drivers/iio/light/veml6031x00.c

[ ... ]

> +static int veml6031x00_write_period(struct iio_dev *iio, int val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +
> +	if (val > 8 || hweight8(val) != 1)
> +		return -EINVAL;
> +
> +	return regmap_field_write(data->rf.pers, ffs(val) - 1);
> +}

Is it possible for negative values to bypass this validation?

If val is a negative number such as -128, val > 8 is false, and hweight8(-128)
checks the bottom 8 bits, which evaluates to exactly 1 bit set. This would
bypass the check and allow malformed inputs to be written to the hardware.

[ ... ]

> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
> +				   int *val)
> +{
[ ... ]
> +	ret = veml6031x00_get_it(data, &it_sec, &it_usec);
> +	if (ret < 0)
> +		goto put_autosuspend;
> +
> +	/* integration time + 10 % to ensure completion */
> +	fsleep((it_sec * MICRO) + it_usec + (it_usec / 10));
> +
> +	if (!iio_device_claim_direct(iio)) {
> +		ret = -EBUSY;
> +		goto put_autosuspend;
> +	}

Could iio_device_claim_direct() be called before sleeping?

Currently, if a triggered buffer is actively capturing data, this will sleep
for the entire integration delay (which could be up to 400ms) before returning
-EBUSY and rejecting the direct read.

[ ... ]

> +static int veml6031x00_write_raw(struct iio_dev *iio,
> +				 struct iio_chan_spec const *chan,
> +				 int val, int val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return veml6031x00_set_it(iio, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return veml6031x00_set_scale(iio, val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +}

Does this need to hold a lock or claim direct mode?

The helpers veml6031x00_set_it() and veml6031x00_set_scale() perform multiple
regmap operations.

Without holding a driver-level mutex or claiming direct mode with
iio_device_claim_direct_mode(), concurrent sysfs writes could interleave and
corrupt the configuration. This also allows userspace to alter hardware
settings while a triggered buffer is actively capturing data.

[ ... ]

> +static int veml6031x00_write_event_config(struct iio_dev *iio,
> +					  const struct iio_chan_spec *chan,
> +					  enum iio_event_type type,
> +					  enum iio_event_direction dir,
> +					  bool state)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	scoped_guard(mutex, &data->lock) {
> +		/* avoid multiple increments/decrements from one source */
> +		if (state == data->ev_en)
> +			return 0;
> +
> +		ret = veml6031x00_set_interrupt(data, state);
> +		if (ret)
> +			return ret;
> +
> +		data->ev_en = state;
> +	}
> +
> +	if (state)
> +		return pm_runtime_resume_and_get(data->dev);
> +
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +	return 0;
> +}

Could this lead to a PM runtime usage counter underflow or a race condition?

If state is true and pm_runtime_resume_and_get() fails, the function returns
an error but leaves data->ev_en as true and the interrupt enabled. A
subsequent call to disable the event would unconditionally call
pm_runtime_put_autosuspend(), underflowing the usage counter.

Additionally, because the PM runtime calls are outside the mutex, concurrent
calls could execute the PM runtime updates out-of-order, leading to reference
count corruption.

[ ... ]

> +static int veml6031x00_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *iio = iio_trigger_get_drvdata(trig);
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	scoped_guard(mutex, &data->lock) {
> +		if (state == data->trig_en)
> +			return 0;
> +
> +		ret = veml6031x00_set_interrupt(data, state);
> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> +	ret = veml6031x00_set_af_trig(data, state);
> +	if (ret)
> +		goto err_clear_af;
> +
> +	scoped_guard(mutex, &data->lock)
> +		data->trig_en = state;
> +
> +	return 0;
> +
> +err_clear_af:
> +	regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +			   VEML6031X00_CONF0_AF,
> +			   FIELD_PREP(VEML6031X00_CONF0_AF, !state));
> +err_disable_interrupt:
> +	veml6031x00_set_interrupt(data, !state);
> +	return ret;
> +}

Does this sequence create a race condition with the lock handling?

The function checks state == data->trig_en and increments an internal counter
in veml6031x00_set_interrupt() inside the first scoped_guard, but data->trig_en
isn't updated until the second scoped_guard. Concurrent callers could bypass
the state check before data->trig_en is updated.

Also, if an error occurs, the err_disable_interrupt path calls
veml6031x00_set_interrupt() outside the lock. This could race with other event
configurations and desynchronize the software state from the hardware state.

[ ... ]

> +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);
> +	int ch, ret, i = 0;
> +	struct {
> +		__le16 chans[2];
> +		aligned_s64 timestamp;
> +	} scan;
> +
> +	memset(&scan, 0, sizeof(scan));
> +
> +	if (*iio->active_scan_mask == VEML6031X00_ALL_CH_MASK) {
[ ... ]
> +	} else {
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(data->regmap,
> +					       iio->channels[ch].address,
> +					       &scan.chans[i++], 2);
> +			if (ret)
> +				goto done;
> +		}
> +	}

Can this loop overflow scan.chans[]?

If the user enables the IIO timestamp channel alongside ALS and IR, the active
scan mask includes the timestamp bit, so execution falls into the else branch.

The iio_for_each_active_channel() loop doesn't skip the timestamp channel, so
it would execute an I2C read into scan.chans[i++] when i reaches 2, overflowing
the 2-element array.

[ ... ]

> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{
[ ... ]
> +	ret = pm_runtime_set_active(dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to activate PM runtime\n");
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
> +
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_autosuspend_delay(dev, VEML6031X00_AUTOSUSPEND_MS);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = veml6031x00_validate_part_id(data);
> +	if (ret)
> +		return ret;

Should the sensor be powered on before marking it active?

The hardware powers up in a shutdown state, and veml6031x00_als_power_on() is
not called here before pm_runtime_set_active(). During the initial autosuspend
delay, reads will assume the device is awake and attempt to read from a
powered-down sensor.

Also, does this code leak the PM runtime reference on probe errors?

If an error occurs during veml6031x00_validate_part_id() or subsequent
initialization steps, the function returns without dropping the reference
acquired by pm_runtime_get_noresume(dev).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-veml6031x00-v2-0-4703ca661a1d@gmail.com?part=4