Re: [PATCH v2 3/5] iio: light: stk3310: move the data registers into the channel address

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Possible Use-After-Free of `event_interface` in `iio_push_event` on the `iio_device_register` error path.
- [High] Missing locking in `stk3310_write_event` leading to state mismatch between hardware and software.
--

commit 0d9aa45c0bb9cd3c7c737938875870bc51a8c0be
Author: Jorijn van der Graaf <jorijnvdgraaf@catcrafts.net>

iio: light: stk3310: move the data registers into the channel address

The RAW read selects its data register with a per-channel-type branch.
Record each channel's data register in its .address field and read
from there, so adding a channel does not grow the branch.

> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 5b5b6812edc75..156888969366c 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c

[ ... ]

> @@ -370,25 +372,20 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
>  {
> -	u8 reg;
>  	__be16 buf;
>  	int ret;
>  	unsigned int index;
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  	struct i2c_client *client = data->client;
> +	struct regmap *map = data->regmap;
>  
>  	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
>  		return -EINVAL;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		if (chan->type == IIO_LIGHT)
> -			reg = STK3310_REG_ALS_DATA_MSB;
> -		else
> -			reg = STK3310_REG_PS_DATA_MSB;
> -
>  		mutex_lock(&data->lock);
> -		ret = regmap_bulk_read(data->regmap, reg, &buf, sizeof(buf));
> +		ret = regmap_bulk_read(map, chan->address, &buf, sizeof(buf));
>  		if (ret < 0) {
>  			dev_err(&client->dev, "register read failed\n");
>  			mutex_unlock(&data->lock);

[Severity: High]
This is a pre-existing issue, but is there missing locking in
stk3310_write_event()?

drivers/iio/light/stk3310.c:stk3310_write_event() {
	...
	buf = cpu_to_be16(val);
	ret = regmap_bulk_write(data->regmap, reg, &buf, sizeof(buf));
	if (ret < 0) {
		dev_err(&client->dev, "failed to set PS threshold!\n");
		return ret;
	}

	if (reg == STK3310_REG_THDH_PS)
		data->ps_thdh = val;
	else
		data->ps_thdl = val;
	...
}

Can concurrent sysfs writes to the event threshold file cause the hardware
configuration and software state to diverge?

If two threads call stk3310_write_event() concurrently without data->lock
being held, could they interleave their regmap_bulk_write() and software
state updates? 

For example, if Thread A writes 10 and Thread B writes 20, can the hardware
be set to 20 while the software state incorrectly records 10, leading to
stk3310_resume() restoring the wrong state later?

[Severity: High]
This isn't a bug introduced by this patch, but could there be a
use-after-free in the probe error path?

drivers/iio/light/stk3310.c:stk3310_probe() {
	...
	if (client->irq > 0) {
		ret = devm_request_threaded_irq(&client->dev, client->irq,
						stk3310_irq_handler,
						stk3310_irq_event_handler,
						IRQF_TRIGGER_FALLING |
						IRQF_ONESHOT,
						"stk3310_event", indio_dev);
		if (ret)
			goto err_standby;
	}

	ret = iio_device_register(indio_dev);
	if (ret < 0) {
		dev_err(&client->dev, "device_register failed\n");
		goto err_standby;
	}
	...
}

If iio_device_register() fails, does this code leave the IRQ active?

Since devm_request_threaded_irq() is called before iio_device_register(), a
failure in iio_device_register() will clean up the event interface and
free it. 

Because the IRQ is still active and not disabled on this error path, an
interrupt could fire concurrently.

If that happens, would stk3310_irq_event_handler() call iio_push_event()
and dereference the freed event interface pointer, resulting in memory
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826175409.326131-1-jorijnvdgraaf@catcrafts.net?part=3