Re: [PATCH v3 3/3] iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] iio: max34417: silent truncation of 64-bit software accumulator
- [High] iio: max34417: sysfs reads violate 1ms hardware UPDATE delay constraint
- [Medium] iio: max34417: u32 acc_count overflows causing corrupted power calculations
- [Medium] iio: max34417: device registered before hardware initialization completes
- [Medium] iio: max34417: unprivileged log-spam DoS via non-rate-limited dev_warn
- [Medium] iio: adc: max34417: data race and TOCTOU on cam_mode causes incorrect statistics
--

commit ebd13d02bb2cfb7ea4951f2b1c27883dde2b138e
Author: Neil Armstrong <neil.armstrong@linaro.org>

iio: adc: add driver for the MAX34417 Four-Channel High Dynamic Range Power Accumulator

The MAX34417 is a specialized current and voltage monitor used to determine
power consumption of portable systems. The driver supports getting the channels
voltage, average power and accumulated average power and energy over an
I2C/SMBUS serial interface.

> diff --git a/drivers/iio/adc/max34417.c b/drivers/iio/adc/max34417.c
> new file mode 100644
> index 0000000000000..00d7f20cd9531
> --- /dev/null
> +++ b/drivers/iio/adc/max34417.c
> @@ -0,0 +1,777 @@

[ ... ]

> +struct max34417_data {
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct mutex lock;
> +	u32 input_correction[MAX34417_CHANNEL_COUNT];
> +	const char *input_label[MAX34417_CHANNEL_COUNT];
> +	bool cam_mode;
> +	u64 acc_power[MAX34417_CHANNEL_COUNT];
> +	u32 acc_count[MAX34417_CHANNEL_COUNT];
> +};

[Severity: Medium]
Will this 32-bit counter overflow during continuous long-term accumulation?

In Continuous Accumulation Mode (CAM), the hardware sample counter is repeatedly
added to this variable across sysfs reads. At the maximum sample rate of 1024
SPS, a 32-bit counter will overflow in approximately 48 days. When this
wrap-around occurs, the subsequent average power calculation
(div_u64(acc_power, acc_count)) will divide the large 64-bit acc_power by a
small or zero acc_count, resulting in a corrupted power reading or a division
by zero panic. Should acc_count be promoted to u64?

[ ... ]

> +static int max34417_read_voltage(struct max34417_data *max34417,
> +				 const struct iio_chan_spec *chan, int *val)
> +{
> +	u8 buf[MAX34417_VOLTAGE_READ_LENGTH];
> +	u16 voltage;
> +	int rc;
> +
> +	guard(mutex)(&max34417->lock);
> +
> +	/* In SMM mode, trigger a measurement cycle manually */
> +	if (!max34417->cam_mode) {
> +		rc = max34417_trigger_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* Move the new UPDATE data into the SMBus read registers */
> +	rc = regmap_write(max34417->regmap, MAX34417_UPDATE_REG, 1);

[Severity: High]
Does this bypass the hardware's 1ms UPDATE delay constraint?

The driver notes that "UPDATE commands should be no less than 1msec apart for
reliable measurements." While max34417_trigger_smm() properly enforces this
delay, the sysfs read handlers for CAM mode unconditionally write to
MAX34417_UPDATE_REG. If an unprivileged userspace process polls the sysfs
attributes faster than 1ms, it might violate this constraint, potentially
leading to unreliable measurements, missing samples, or bus lockups.

[ ... ]

> +static int max34417_read_power(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{

[ ... ]

> +	if (max34417->cam_mode) {
> +		u32 acc_count;
> +		u64 acc_power;
> +
> +		/* Retrieve all channels to avoid missing data for other channels */
> +		rc = max34417_save_accumulators(max34417);
> +		if (rc)
> +			return rc;
> +
> +		/* Get current channel power, counter and reset them */
> +		acc_power = max34417->acc_power[chan->channel];
> +		acc_count = max34417->acc_count[chan->channel];
> +		max34417->acc_power[chan->channel] = 0;
> +		max34417->acc_count[chan->channel] = 0;
> +
> +		if (unlikely(acc_count == 0)) {
> +			dev_warn(max34417->dev, "unexpected zero accumulator count\n");

[Severity: Medium]
Could this non-rate-limited warning lead to a log-spam denial of service?

If an unprivileged userspace process repeatedly reads the attribute in a tight
loop, it will extract data faster than the hardware's sample rate, causing the
counter to evaluate to 0. This blindly prints a warning using dev_warn()
without any rate limiting, which could flood the kernel log buffer and mask
legitimate kernel messages. Should dev_warn_ratelimited() be used instead?

> +			*val = 0;
> +			*val2 = 0;
> +
> +			return IIO_VAL_INT_64;
> +		}
> +
> +		power = div_u64(acc_power, acc_count);
> +	} else {
> +		rc = max34417_read_acc_power(max34417, chan->address, &power);
> +		if (rc)
> +			return rc;
> +
> +		rc = max34417_reset_smm(max34417);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	*val = FIELD_GET(GENMASK_ULL(31, 0), power);
> +	*val2 = FIELD_GET(GENMASK_ULL(55, 32), power);

[Severity: High]
Does this silently truncate the upper 8 bits of the 64-bit software
accumulator?

GENMASK_ULL(55, 32) explicitly masks off bits 56-63, meaning any accumulated
value exceeding 56 bits is silently truncated and dropped before being
exported to IIO userspace. Since CAM mode continuously accumulates readings,
the u64 can easily exceed 56 bits in sustained operation, causing the
userspace reading to wrap around prematurely. Should this use
GENMASK_ULL(63, 32) or upper_32_bits(power)?

> +
> +	return IIO_VAL_INT_64;
> +}
> +
> +static int max34417_read_energy(struct max34417_data *max34417,
> +			       const struct iio_chan_spec *chan,
> +			       int *val, int *val2)
> +{

[ ... ]

> +	/* Get current channel power and reset them */
> +	acc_power = max34417->acc_power[chan->channel];
> +	max34417->acc_power[chan->channel] = 0;
> +	max34417->acc_count[chan->channel] = 0;
> +
> +	*val = FIELD_GET(GENMASK_ULL(31, 0), acc_power);
> +	*val2 = FIELD_GET(GENMASK_ULL(55, 32), acc_power);

[Severity: High]
Does this also silently truncate the upper 8 bits of the 64-bit accumulator?

Just as in max34417_read_power(), this GENMASK_ULL(55, 32) truncates bits
56-63 of acc_power.

[ ... ]

> +static int max34417_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max34417_data *max34417 = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			return max34417_read_voltage(max34417, chan, val);
> +		case IIO_POWER:
> +			/* In default mode, we return a single measurement */
> +			if (max34417->cam_mode)
> +				return -EINVAL;
> +
> +			return max34417_read_power(max34417, chan, val, val2);

[Severity: Medium]
Could this lockless check of cam_mode lead to a TOCTOU race and return
incorrect statistics?

Here, cam_mode is evaluated without holding the mutex. Concurrently, a sysfs
write to in_energy_enable could call max34417_setup_cam_mode(), taking the
mutex and setting cam_mode to true. When max34417_read_power() is then
called, it acquires the mutex, sees cam_mode is true, and executes the CAM
mode logic. This would unexpectedly return an averaged measurement instead of
a single-shot reading, violating the expected IIO channel behavior.

[ ... ]

> +static int max34417_probe(struct i2c_client *client)
> +{

[ ... ]

> +	indio_dev->channels = max34417_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max34417_channels);
> +	indio_dev->name = "max34417";
> +	indio_dev->info = &max34417_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	rc = devm_iio_device_register(dev, indio_dev);
> +	if (rc)
> +		return rc;
> +
> +	/* Reset mode and set accumulator width to Wide */
> +	rc = regmap_write(max34417->regmap, MAX34417_CONTROL_REG, MAX34417_CONTROL_MODE);

[Severity: Medium]
Is it safe to register the IIO device before the hardware initialization
completes?

devm_iio_device_register() exposes the device and its sysfs attributes to
userspace immediately. A userspace process or udev rule could open the device
and trigger read_raw or write_raw callbacks before the hardware is placed
into the correct mode (MAX34417_CONTROL_MODE) or its internal accumulators
are reset. Should the hardware configuration happen prior to registration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-topic-sm8x50-iio-max34417-adc-v3-0-d3604ae4a229@linaro.org?part=3