Re: [PATCH v3 4/6] iio: light: ltr501: Power down chip if request irq fails

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

Pre-existing issues:
- [High] Uninitialized kernel stack memory is leaked to userspace and functional behavior is broken on Big Endian architectures due to `regmap_bulk_read()` reading into a 4-byte `int`.
- [Medium] Data race (TOCTOU) on `data->als_contr` and `data->ps_contr` between raw sysfs reads and writes.
- [Medium] Data race on `data->als_period` and `data->ps_period` between sysfs event writes and sysfs raw writes.
--

--- Patch [4]: [PATCH v3 4/6] iio: light: ltr501: Power down chip if request irq fails ---
commit 9f4c589a7c5ec63bde9127853783856ae3e751dd
Author: Esben Haabendal <[email protected]>

iio: light: ltr501: Power down chip if request irq fails

This patch updates the error handling in ltr501_probe() when
devm_request_threaded_irq() fails. It ensures the chip is powered down by
jumping to the powerdown_on_error label and removes a duplicate error message.

> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 7d045be78c6df..b36bd10a02624 100644
> --- a/drivers/iio/light/ltr501.c
> +++ b/drivers/iio/light/ltr501.c
> @@ -1537,11 +1537,8 @@ static int ltr501_probe(struct i2c_client *client)
>  						IRQF_ONESHOT,
>  						"ltr501_thresh_event",
>  						indio_dev);
> -		if (ret) {
> -			dev_err(&client->dev, "request irq (%d) failed\n",
> -				client->irq);
> -			return ret;
> -		}
> +		if (ret)
> +			goto powerdown_on_error;
>  	} else {
>  		indio_dev->info = data->chip_info->info_no_irq;
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with
uninitialized memory when reading data on Big Endian architectures. 

In ltr501_read_thresh():

        int ret, thresh_data;
        ...
        ret = regmap_bulk_read(data->regmap, LTR501_ALS_THRESH_UP,
                               &thresh_data, 2);
        if (ret < 0)
                return ret;
        *val = thresh_data & LTR501_ALS_THRESH_MASK;

And in ltr501_trigger_handler():

        int ret, psdata;
        ...
        ret = regmap_bulk_read(data->regmap, LTR501_PS_DATA,
                               &psdata, 2);
        if (ret < 0)
                goto done;
        scan.channels[j++] = psdata & LTR501_PS_DATA_MASK;

Could this leak up to 2 bytes of uninitialized kernel stack memory to userspace
and result in data loss on Big Endian systems? The regmap_bulk_read() call
reads 2 bytes into the MSB of the 32-bit int variables and then the code masks
the uninitialized LSBs.

Similarly, in ltr501_write_thresh():

        ret = regmap_bulk_write(data->regmap, LTR501_ALS_THRESH_UP,
                                &val, 2);

Would this write the upper 2 bytes of the 32-bit integer on Big Endian systems,
silently passing incorrect zeroes to the hardware?

[Severity: Medium]
This is a pre-existing issue, but is there a data race on data->als_contr
between raw sysfs reads and writes?

In ltr501_read_raw(), data->als_contr is read locklessly:

        i = (data->als_contr & data->chip_info->als_gain_mask)
             >> data->chip_info->als_gain_shift;

While in __ltr501_write_raw(), it performs a non-atomic read-modify-write:

        data->als_contr &= ~info->als_gain_mask;
        data->als_contr |= i << info->als_gain_shift;

Can a concurrent read (via sysfs) during a write observe an intermediate
configuration state, leading to the driver temporarily reporting an incorrect
scale factor to userspace?

[Severity: Medium]
This isn't a bug introduced by this patch, but is there a data race on
data->als_period between sysfs event writes and sysfs raw writes?

In ltr501_write_intr_prst():

        mutex_unlock(&data->lock_als);
        if (ret >= 0)
                data->als_period = period;

And in __ltr501_write_raw():

        ret = ltr501_write_intr_prst(data, chan->type,
                                     0, data->als_period);

Does updating data->als_period outside of the mutex lock in
ltr501_write_intr_prst() leave it unprotected against concurrent event writers
or lockless readers like __ltr501_write_raw()?

Could this lead to stale or inconsistent values being used to configure the
hardware, potentially setting incorrect sampling frequencies or interrupt
persistence counts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4