Re: [PATCH v7 2/2] iio: adc: add MAX40080 current-sense amplifier driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized stack memory disclosure due to insufficient length check on I2C block read
- [Medium] Data race and torn read risk due to missing READ_ONCE()/WRITE_ONCE() for concurrent lockless state access
--

--- Patch [2]: [PATCH v7 2/2] iio: adc: add MAX40080 current-sense amplifier driver ---
commit ea9754605e8a1fb9ab674845d433d119d507e471
Author: Stefan Popa <[email protected]>

iio: adc: add MAX40080 current-sense amplifier driver

This commit adds an IIO driver for the MAX40080 bidirectional
current-sense amplifier. It supports single-measurement mode via SMBus
Quick Command and exposes current and voltage channels.

> diff --git a/drivers/iio/adc/max40080.c b/drivers/iio/adc/max40080.c
> new file mode 100644
> index 0000000000000..4b397bcd145b2
> --- /dev/null
> +++ b/drivers/iio/adc/max40080.c

[ ... ]

> +static int max40080_read_iv_once(struct max40080_state *st, u32 *iv)
> +{
> +	u8 buf[4];
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(st->client, MAX40080_REG_IV,
> +					    sizeof(buf), buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	*iv = get_unaligned_le32(buf);

[Severity: High]
i2c_smbus_read_i2c_block_data() returns the number of bytes read on success.
If a short read occurs (e.g., returning 1 to 3 bytes), ret will be positive
and pass the if (ret < 0) check. 

Does this allow get_unaligned_le32() to read uninitialized stack memory from
buf, potentially exposing it to userspace or causing the driver's polling
loop to incorrectly evaluate the validity bit?

Should this check be if (ret != sizeof(buf))?

[ ... ]

> +static int max40080_set_range(struct max40080_state *st, unsigned int range)
> +{
> +	int ret;
> +
> +	ret = max40080_update_bits(st, MAX40080_REG_CFG, MAX40080_CFG_RANGE_MSK,
> +				   FIELD_PREP(MAX40080_CFG_RANGE_MSK, range));
> +	if (ret)
> +		return ret;
> +
> +	st->range = range;

[Severity: Medium]
This plain write updates the shared state without WRITE_ONCE(), which might
allow compiler reordering that exacerbates the race condition in
max40080_read_raw().

[ ... ]

> +static int max40080_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +	unsigned int range;
> +	int ret;
> +
> +	switch (mask) {

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_CURRENT) {
> +			/*
> +			 * The selectable current-sense range is exposed through
> +			 * scale: each RANGE setting has its own precomputed
> +			 * mA-per-code value. Userspace picks the range by writing
> +			 * the matching scale.
> +			 *
> +			 * Take a local copy of range to ensure val and val2 come
> +			 * from the same setting even if a concurrent write changes
> +			 * st->range between the two accesses.
> +			 */
> +			range = st->range;
> +			*val = st->current_scale[range][0];
> +			*val2 = st->current_scale[range][1];

[Severity: Medium]
The comment mentions taking a local copy to prevent issues if a concurrent
write changes st->range. However, since this is a plain C assignment, can the
compiler optimize out the local copy and reload st->range directly from memory
for each array access?

If max40080_write_raw() concurrently updates st->range, could the two scale
components be torn, defeating the explicit purpose of the local copy? Should
this use READ_ONCE() along with a matching WRITE_ONCE() in
max40080_set_range() to guarantee this behavior?

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