Re: [PATCH v4 3/4] iio: light: veml6031x00: add support for triggered buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Spurious I2C read of configuration register when the soft timestamp channel is enabled without both ALS and IR.
- [High] Uninitialized padding bytes in the `scan` structure leak kernel stack memory to user-space.

Pre-existing issues:
- [High] Mixed devm-managed PM runtime GET and manual autosuspend PUT causes the usage counter to underflow on driver unbind.
--

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

iio: light: veml6031x00: add support for triggered buffers

Add triggered buffer functionality for the two channels the device
provides (ALS and IR).

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 6f9a7bad44d44..facb1b8e42418 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c
[ ... ]
> @@ -469,6 +500,82 @@ static const struct iio_info veml6031x00_info = {
[ ... ]
> +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 = { };

[Severity: High]
Does this initialization leave compiler padding bytes uninitialized?

The scan struct has 4 bytes of compiler padding between the chans array (4
bytes) and the timestamp field (aligned to 8 bytes). Using = { }
zero-initializes the defined fields but leaves the padding bytes uninitialized
in C.

Since iio_push_to_buffers_with_ts() copies the entire 16-byte struct to the
IIO ring buffer, could this leak uninitialized kernel stack padding to
user-space? An explicit memset(&scan, 0, sizeof(scan)) is typically used to
prevent this.

> +
> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {
> +		ret = regmap_bulk_read(data->regmap,
> +				       VEML6031X00_REG_ALS_L,
> +				       &scan.chans, sizeof(scan.chans));
> +		if (ret)
> +			goto done;
> +	} else {
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(data->regmap,
> +					       iio->channels[ch].address,
> +					       &scan.chans[i++],
> +					       sizeof(*scan.chans));

[Severity: High]
Does this loop fail to skip the software-only timestamp channel?

The iio_for_each_active_channel() macro iterates over all set bits in
active_scan_mask, including the timestamp channel bit
(VEML6031X00_SCAN_TIMESTAMP). 

Because the timestamp channel doesn't have an address,
iio->channels[ch].address will default to 0x00 (VEML6031X00_REG_CONF0).

Will this result in a spurious I2C read from the configuration register when
the soft timestamp channel is enabled without both ALS and IR, wasting I2C
bandwidth and populating the data array with unintended hardware data?

> +			if (ret)
> +				goto done;
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> @@ -576,6 +683,13 @@ static int veml6031x00_probe(struct i2c_client *i2c)
[ ... ]
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> +					      veml6031x00_trig_handler,
> +					      &veml6031x00_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register triggered buffer\n");
> +
>  	pm_runtime_put_autosuspend(dev);

[Severity: High]
This is a pre-existing issue, but does mixing devm-managed PM runtime GET and
manual autosuspend PUT cause the usage counter to underflow on driver unbind?

Earlier in veml6031x00_probe(), devm_pm_runtime_get_noresume(dev)
increments the PM usage counter and queues a devres action to decrement it on
module unbind.

At the end of the probe function, the driver manually calls
pm_runtime_put_autosuspend(dev), decrementing the usage counter back to 0.

Upon driver unbind, the queued devres action will execute, dropping the usage
counter a second time. Could this underflow the PM usage counter to -1,
causing a WARN splat and corrupting the device's PM state? Should this be
using the non-devm pm_runtime_get_noresume(dev) if it manually drops the
reference in probe?

>  
>  	ret = devm_iio_device_register(dev, iio);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d@gmail.com?part=3