Re: [PATCH v4 2/2] iio: adc: add support for PAC194X

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Makefile typo prevents driver compilation.
- [High] Unvalidated shunt resistor allows divide-by-zero kernel panic.
- [High] Missing type check on ACPI object returned by `acpi_evaluate_dsm`.
- [High] Incomplete check of `i2c_transfer` return value leads to uninitialized memory use.
- [High] Register parsing misalignment when channels are disabled.
- [High] TOCTOU race in `pac1944_retrieve_data()` causes data loss.
- [High] Unprotected lockless reads of shared state cause data corruption and torn reads.
- [High] Unconditional re-arming of delayed work causes Use-After-Free.
- [High] Passing stack memory to `i2c_transfer()` violates DMA mapping rules.
- [High] Accumulator saturation applies in the wrong direction upon overflow.
- [Medium] Power scale calculation overflows 32-bit signed integers.
- [Medium] ACPI package object leaked on error paths during probe.
--

commit f9f3f02e24d9b1eec599682a12c6ee4c2e8e0b5f
Author: Marius Cristea <marius.cristea@microchip.com>
Subject: iio: adc: add support for PAC194X

This commit adds the IIO driver for the Microchip PAC194X and PAC195X series
of Power Monitors. It supports multiple voltage and current channels.

> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 097357d146baf..0e1cc47b68a13 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -113,6 +113,7 @@ obj-$(CONFIG_NPCM_ADC) += npcm_adc.o
>  obj-$(CONFIG_NXP_SAR_ADC) += nxp-sar-adc.o
>  obj-$(CONFIG_PAC1921) += pac1921.o
>  obj-$(CONFIG_PAC1934) += pac1934.o
> +obj-$(CONFIG_PAC1934) += pac1944.o

This isn't a bug, but there is a typo in the config name. Should this be
CONFIG_PAC1944?

>  obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o

[ ... ]

> diff --git a/drivers/iio/adc/pac1944.c b/drivers/iio/adc/pac1944.c
> new file mode 100644
> index 0000000000000..3938bbcd7e6bc
> --- /dev/null
> +++ b/drivers/iio/adc/pac1944.c

[ ... ]

> +static int pac1944_i2c_read(struct i2c_client *client, u8 reg_addr,
> +			    void *databuf, u8 len)
> +{
> +	struct i2c_msg msgs[2] = {
> +		{
> +		 .addr = client->addr,
> +		 .len = 1,
> +		 .buf = (u8 *)&reg_addr,

Can this cause a panic when CONFIG_VMAP_STACK is enabled?

Since reg_addr is on the stack, it might not be DMA-safe for I2C host
controllers that use DMA. Should this be placed in a bounce buffer?

> +		 .flags = 0
> +		},

[ ... ]

> +static ssize_t pac1944_shunt_value_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct pac1944_chip_info *info = iio_priv(indio_dev);
> +	int sh_val;
> +
> +	if (kstrtouint(buf, 10, &sh_val)) {
> +		dev_err(dev, "Shunt value is not valid\n");
> +		return -EINVAL;
> +	}
> +
> +	scoped_guard(mutex, &info->lock)
> +		info->shunts[this_attr->address] = sh_val;

Does this allow writing a value of 0?

If info->shunts[idx] is set to 0, will the division operations in
pac1944_in_power_acc_scale_show() and pac1944_read_raw() trigger a
divide-by-zero hardware exception?

> +
> +	return count;
> +}

[ ... ]

> +static int pac1944_reg_snapshot(struct pac1944_chip_info *info, bool do_refresh,
> +				u8 refresh_addr, u32 wait_time)
> +{

[ ... ]

> +	/* Read the data registers. */
> +	ret = pac1944_i2c_read(client, PAC1944_ACC_COUNT_REG_ADDR,
> +			       (u8 *)info->chip_reg_data.meas_regs,
> +			       PAC1944_MEAS_REG_SNAPSHOT_LEN);
> +	if (ret < 0) {

What happens if i2c_transfer() executes only the first message and returns 1?

The condition ret < 0 will pass, but the meas_regs buffer might remain
uninitialized. Should this check that ret equals the number of messages
instead?

> +		dev_err(dev, "cannot read PAC1944 regs from 0x%02X\n", PAC1944_ACC_COUNT_REG_ADDR);
> +		return ret;
> +	}
> +
> +	offset_reg_data_p = &info->chip_reg_data.meas_regs[0];
> +
> +	info->chip_reg_data.acc_count = get_unaligned_be32(offset_reg_data_p);
> +
> +	offset_reg_data_p += PAC1944_ACC_REG_LEN;
> +
> +	/*
> +	 * Check if the channel is active (within the data read from the chip),
> +	 * skip all fields if disabled.
> +	 */
> +	for_each_set_bit(cnt, &info->active_channels_mask, info->phys_channels) {
> +		/* skip if the energy accumulation is disabled */
> +		if (!info->enable_acc[cnt]) {
> +			offset_reg_data_p += PAC1944_VACC_REG_LEN;
> +			continue;
> +		}

If a channel is entirely inactive (its bit is not set in active_channels_mask),
the loop skips it completely.

Will this fail to advance offset_reg_data_p past the inactive channel's fixed
slot in the buffer, causing subsequent reads to be structurally misaligned?

> +
> +		stored_value = info->chip_reg_data.acc_val[cnt];

[ ... ]

> +		if (smpl_mode <= PAC1944_SAMP_8SPS) {
> +			tmp_s64 = info->chip_reg_data.vacc[cnt];
> +			/*
> +			 * Find how much shift is required by the sample rate.
> +			 * The chip's sampling rate is 2^shift samples/sec.
> +			 */
> +			shift = shift_map_tbl[smpl_mode];
> +			inc = tmp_s64 >> shift;
> +		} else {
> +			dev_err(dev, "Invalid sample rate index: %d!\n", smpl_mode);
> +			return -EINVAL;
> +		}
> +
> +		if (check_add_overflow(stored_value, inc, &stored_value)) {
> +			if (stored_value < 0)
> +				info->chip_reg_data.acc_val[cnt] = S64_MIN;
> +			else
> +				info->chip_reg_data.acc_val[cnt] = S64_MAX;

Does this clamp in the wrong direction?

When two large positive values overflow, check_add_overflow() places the
wrapped negative result into stored_value.

Will the condition stored_value < 0 evaluate to true and clamp the positive
overflow to S64_MIN instead of S64_MAX?

> +
> +			dev_err(dev, "Overflow detected on channel [%d]!\n", cnt + 1);
> +		} else {

[ ... ]

> +static int pac1944_retrieve_data(struct pac1944_chip_info *info, u32 wait_time)
> +{
> +	int ret;
> +
> +	/*
> +	 * Check if the minimal elapsed time has passed and if so,
> +	 * re-read the chip, otherwise the cached info is just fine.
> +	 */
> +	if (!time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
> +		       msecs_to_jiffies(PAC1944_MIN_POLLING_TIME_MS)))
> +		return 0;
> +
> +	/* We need to re-read the chip values */
> +	ret = pac1944_reg_snapshot(info, true, PAC1944_REFRESH_REG_ADDR, wait_time);

Is there a race condition here if multiple threads read sysfs concurrently?

If the cache is stale, both threads could pass the time_after() check
locklessly, serialize on the lock in pac1944_reg_snapshot(), and issue
sequential REFRESH commands.

Since REFRESH resets the chip accumulators, will the second thread wipe out
the data and read near-zero values? Should the expiration check be
re-evaluated inside the mutex lock?

> +
> +	/*

[ ... ]

> +static int pac1944_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct pac1944_chip_info *info = iio_priv(indio_dev);
> +	int ret, idx;
> +	u64 tmp;
> +
> +	ret = pac1944_retrieve_data(info, PAC1944_MIN_UPDATE_WAIT_TIME_US);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * In the datasheet channels are referred to as 1 to 4. For AVG the index
> +	 * should be between 5 to 8. To calculate PAC1944_CH_VOLTAGE_AVERAGE and
> +	 * PAC1944_CH_CURRENT_AVERAGE real index, we need to remove the added
> +	 * offset (PAC1944_MAX_CH).
> +	 */
> +	idx = (chan->channel - 1) % PAC1944_MAX_CH;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = info->chip_reg_data.vbus[idx];

Could this result in torn reads or corrupted data?

The fields in info->chip_reg_data are read locklessly if the cache is deemed
fresh.

At the same time, pac1944_work_periodic_rfsh() can execute
pac1944_reg_snapshot() and modify these fields under info->lock. Could a
reader capture an intermediate state?

> +			return IIO_VAL_INT;

[ ... ]

> +		case PAC1944_VPOWER_1_ADDR:
> +		case PAC1944_VPOWER_2_ADDR:
> +		case PAC1944_VPOWER_3_ADDR:
> +		case PAC1944_VPOWER_4_ADDR:
> +			if (info->is_pac195x_family)
> +				tmp = PAC195X_PRODUCT_VOLTAGE_PV_FSR;
> +			else
> +				tmp = PAC194X_PRODUCT_VOLTAGE_PV_FSR;
> +
> +			do_div(tmp, info->shunts[idx]);
> +			*val = (int)tmp;

Can this overflow the 32-bit signed integer?

For a typical 1 mOhm shunt, PAC195X_PRODUCT_VOLTAGE_PV_FSR (3.2 trillion)
divided by 1000 is 3.2 billion, which exceeds the maximum value of a
signed 32-bit integer. Will this expose a negative scale to userspace?

> +			if ((info->chip_reg_data.vbus_mode[idx] == PAC1944_UNIPOLAR_FSR_CFG &&

[ ... ]

> +static void pac1944_work_periodic_rfsh(struct work_struct *work)
> +{
> +	struct pac1944_chip_info *info = to_pac1944_chip_info(to_delayed_work(work));
> +	struct i2c_client *client = info->client;
> +
> +	dev_dbg(&client->dev, "Periodic refresh\n");
> +
> +	pac1944_reg_snapshot(info, true, PAC1944_REFRESH_REG_ADDR,
> +			     PAC1944_MIN_UPDATE_WAIT_TIME_US);
> +
> +	mod_delayed_work(system_percpu_wq, &info->work_chip_rfsh,
> +			 msecs_to_jiffies(PAC1944_MAX_RFSH_LIMIT_MS));
> +}

Can this re-arm the work after the device is unbound?

The driver uses devm_add_action_or_reset() to call cancel_delayed_work_sync(),
but if that runs concurrently with this periodic refresh, could
mod_delayed_work() schedule the work again on freed memory?

> +
> +/*
> + * Universal Unique Identifier (UUID),

[ ... ]

> +static int pac1944_acpi_parse_channel_config(struct i2c_client *client,
> +					     struct pac1944_chip_info *info)
> +{
> +	struct device *dev = &client->dev;
> +	const struct acpi_device_id *id;
> +	unsigned short bi_dir_mask;
> +	union acpi_object *rez;
> +	acpi_handle handle;
> +	guid_t guid;
> +	int i;
> +
> +	handle = ACPI_HANDLE(dev);
> +
> +	id = acpi_match_device(dev->driver->acpi_match_table, dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	guid_parse(PAC1944_DSM_UUID, &guid);
> +
> +	rez = acpi_evaluate_dsm(handle, &guid, 0, PAC1944_ACPI_GET_NAMES, NULL);
> +	if (!rez)
> +		return -EINVAL;
> +
> +	if (rez->package.count > PAC1944_MAX_CH) {

What happens if a buggy firmware returns an ACPI_TYPE_INTEGER instead of
ACPI_TYPE_PACKAGE?

The code directly accesses rez->package.count without verifying
rez->type == ACPI_TYPE_PACKAGE. Could this read overlapping union members and
cause an out-of-bounds dereference?

> +		ACPI_FREE(rez);
> +		return -EINVAL;
> +	}

[ ... ]

> +	for_each_set_bit(i, &info->active_channels_mask, info->phys_channels) {
> +		if (rez->package.elements[i].type != ACPI_TYPE_INTEGER) {
> +			ACPI_FREE(rez);
> +			return -EINVAL;
> +		}
> +
> +		bi_dir_mask = rez->package.elements[i].integer.value;
> +
> +		if (bi_dir_mask == PAC1944_UNIPOLAR_FSR_CFG ||
> +		    bi_dir_mask == PAC1944_BIPOLAR_FSR_CFG  ||
> +		    bi_dir_mask == PAC1944_BIPOLAR_HALF_FSR_CFG) {
> +			dev_dbg(dev, "VBUS{%d} mode set to: %d\n", i, bi_dir_mask);
> +			info->chip_reg_data.vbus_mode[i] = bi_dir_mask;
> +		} else {
> +			return dev_err_probe(dev, -EINVAL, "invalid vbus-mode value on %i\n", i);
> +		}

Does this leak the memory for the rez object?

It looks like ACPI_FREE(rez) is bypassed when returning an error here.

> +
> +		if (rez->package.elements[i + PAC1944_MAX_CH].type != ACPI_TYPE_INTEGER) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515092946.10791-1-marius.cristea@microchip.com?part=2