Re: [PATCH v2 2/2] hwmon: (pmbus/max20830) add driver for max20830

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Missing validation for short reads from `i2c_smbus_read_i2c_block_data()` can lead to uninitialized stack memory access and information leak.
- [Low] Off-by-one error when null-terminating the device ID string truncates the last character of the payload.
--

commit 479738352902d3197058c3172cee31960249a6a5
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

hwmon: (pmbus/max20830) add driver for max20830

This adds support for the MAX20830 step-down DC-DC switching regulator with
a PMBus interface, allowing monitoring of input/output voltages, output
current, and temperature.

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> new file mode 100644
> index 0000000000000..21ea8b59150cb
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/max20830.c

[ ... ]

> +	ret = i2c_smbus_read_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
> +					    I2C_SMBUS_BLOCK_MAX, buf);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to read IC_DEVICE_ID\n");

i2c_smbus_read_i2c_block_data() returns the number of bytes read on success.
If the I2C controller performs a short read, returning fewer bytes than
expected, buf will contain uninitialized stack memory for the remaining
bytes.

If buf[0] coincidentally contains 9, the len != 9 check below will pass. Could
this lead to uninitialized stack memory being accessed and printed by
dev_err_probe() on a mismatch? Should we verify the number of bytes read
(e.g., ret >= 10) instead of just ret < 0?

> +
> +	/* First byte is the block length. */
> +	len = buf[0];
> +	if (len != 9)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "Unexpected IC_DEVICE_ID response\n");
> +
> +	buf[len] = '\0';

Since the payload of length len occupies buf[1] through buf[len], doesn't
setting buf[len] to '\0' overwrite the final byte of the device ID payload?

While this 9th byte is ignored by the 8-byte strncmp() call below, it will
cause the dev_err_probe() error log to print a truncated string. Should the
null terminator be placed at buf[len + 1] instead?

> +	if (strncmp(buf + 1, "MAX20830", 8))
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "Unsupported device: '%s'\n", buf + 1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416-dev_max20830-v2-0-2c7d676dc0bd@analog.com?part=2