Re: [PATCH] hwmon: (pmbus) Validate number of phases per page

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added strict bounds check on `info->phases[page]` causes a functional regression for legitimate devices (e.g., mp2975, mp2856) that require more than 10 phases.

Pre-existing issues:
- [High] Preexisting out-of-bounds write in `mp2856_identify_multiphase_rail1` when probing the `mp2857` chip due to hardcoded bounds mismatch.
- [High] The bounds check for `info->pages` is performed after `pmbus_clear_faults()`, leading to out-of-bounds reads if a driver configures a pages count exceeding `PMBUS_PAGES`.
- [High] IEEE754 conversion functions use 32-bit 'long' types, causing integer overflow and truncation for power sensors on 32-bit architectures.
--

commit 039f346ee509dcc9fb57df126053cfcdf1e45d57
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (pmbus) Validate number of phases per page

This commit adds a bounds check to the PMBus core API to ensure that
the number of phases configured by a driver does not exceed the
PMBUS_PHASES limit. This prevents potential out-of-bounds reads.

> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 5f69c1420b4e1..c7b7bb7f175b2 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2962,6 +2962,11 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>  	}
>  
>  	for (page = 0; page < info->pages; page++) {
> +		if (info->phases[page] > PMBUS_PHASES) {
> +			dev_err(dev, "Bad number of PMBus phases for page %d: %d\n",
> +				page, info->phases[page]);
> +			return -ENODEV;
> +		}

[Severity: High]
Does this bounds check introduce a functional regression for devices that
legitimately require more than 10 phases?

Devices like the mp2975 configure up to 14 phases, and the mp2856 uses
up to 12 phases. If PMBUS_PHASES is currently 10, won't this check
unconditionally abort initialization and return -ENODEV for these valid
devices instead of properly increasing the PMBUS_PHASES limit?

>  		ret = pmbus_identify_common(client, data, page);
>  		if (ret < 0) {
>  			dev_err(dev, "Failed to identify chip capabilities\n");

[ ... ]

[Severity: High]
This is a pre-existing issue, but does mp2856_identify_multiphase_rail1()
cause an out-of-bounds write when probing the mp2857 chip?

drivers/hwmon/pmbus/mp2856.c:mp2856_identify_multiphase_rail1() {
	...
	for (i = 0 ; i < data->info.phases[0]; i++)
		data->info.pfunc[i] |= PMBUS_HAVE_IOUT;
	...
}

When probing the mp2857 chip (which has 12 phases), info->phases[0] is set
to 12. Since the pfunc array in struct pmbus_driver_info is bounded by
PMBUS_PHASES (10), won't writing up to index 11 corrupt memory, such as
the read_byte_data and read_word_data function pointers?

[Severity: High]
This is a pre-existing issue, but does pmbus_init_common() perform an
out-of-bounds read if a driver configures a pages count exceeding
PMBUS_PAGES?

drivers/hwmon/pmbus/pmbus_core.c:pmbus_init_common() {
	...
	if (data->info->pages)
		pmbus_clear_faults(client);
	...
	if (info->pages <= 0 || info->pages > PMBUS_PAGES) {
	...
}

Since pmbus_clear_faults() iterates up to info->pages and calls
pmbus_set_page(), which accesses the func and phases arrays:

drivers/hwmon/pmbus/pmbus_core.c:pmbus_set_page() {
	...
	if (data->info->phases[page] && data->currphase != phase &&
	    !(data->info->func[page] & PMBUS_PHASE_VIRTUAL)) {
	...
}

Won't the unchecked page index cause an out-of-bounds read on the func and
phases arrays before the pages count is validated?

[Severity: High]
This is a pre-existing issue, but do the IEEE754 conversion functions
truncate power sensors on 32-bit architectures?

drivers/hwmon/pmbus/pmbus_core.c:
static long pmbus_reg2data_ieee754(struct pmbus_data *data,
				   struct pmbus_sensor *sensor)
{
	int exponent;
	bool sign;
	long val;
	...
}

static u16 pmbus_data2reg_ieee754(struct pmbus_data *data,
				  struct pmbus_sensor *sensor, long val)
{
	...
}

Because the val parameter and variable are 32-bit long instead of s64 on
32-bit architectures, won't passing high user-space sysfs power limits
(e.g., 3000 W as 3,000,000,000 uW) immediately wrap to a negative number?

This truncation from 64-bit to 32-bit signed long could cause the driver
to configure inverted or zero limits in the hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912145038.2411447-1-linux@roeck-us.net?part=1