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

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

Pre-existing issues:
- [High] The driver incorrectly uses pointers to local `int` variables for `regmap_bulk_read()` and `regmap_bulk_write()` when accessing 16-bit registers. On big-endian systems, this accesses the most significant bytes, leaving the least significant bytes uninitialized. This results in returning kernel stack memory to userspace (info leak) or writing invalid zeroes to the device.
--

--- Patch [4]: [PATCH v5 4/8] iio: light: ltr501: Power down chip if request irq fails ---
commit b2aeaca1f2b62b80b38b3ac186edef1f0457ea0a
Author: Esben Haabendal <[email protected]>

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

In case devm_request_threaded_irq() failed, we were leaving the chip
powered instead of powering it down again. Power down the chip switching
ALS and PS modes to standby.

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

[Severity: High]
This isn't a bug introduced by this patch, but there are a few pre-existing
endianness issues in this file related to regmap_bulk_read() and
regmap_bulk_write().

In ltr501_trigger_handler():

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

Since psdata is an int, regmap_bulk_read() will populate the upper 16 bits of
the integer on big-endian systems, leaving the lower 16 bits uninitialized.

Could this cause the bitwise AND to operate on uninitialized stack memory,
which is then pushed to the IIO buffer?

A similar issue appears in ltr501_read_thresh():

		case IIO_EV_DIR_RISING:
			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;

Here, 2 bytes are read into the local int variable thresh_data.

Will this cause the function to return uninitialized stack memory to userspace
on big-endian architectures?

And in ltr501_write_thresh():

		case IIO_EV_DIR_RISING:
			mutex_lock(&data->lock_als);
			ret = regmap_bulk_write(data->regmap,
						LTR501_ALS_THRESH_UP,
						&val, 2);

The val parameter is an int.

Might writing 2 bytes directly from it on a big-endian system send 0x0000
instead of the actual 16-bit threshold values?

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