* Re: [PATCH] hwmon: (pmbus) Validate number of phases per page
2026-09-12 14:50 [PATCH] hwmon: (pmbus) Validate number of phases per page Guenter Roeck
@ 2026-09-12 15:07 ` sashiko-bot
2026-09-14 12:21 ` Nuno Sá
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-09-12 15:07 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] hwmon: (pmbus) Validate number of phases per page
2026-09-12 14:50 [PATCH] hwmon: (pmbus) Validate number of phases per page Guenter Roeck
2026-09-12 15:07 ` sashiko-bot
@ 2026-09-14 12:21 ` Nuno Sá
1 sibling, 0 replies; 3+ messages in thread
From: Nuno Sá @ 2026-09-14 12:21 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Hardware Monitoring
On Sat, Sep 12, 2026 at 07:50:38AM -0700, Guenter Roeck wrote:
> It is possible for a PMBus driver to configure a phase count larger than
> the PMBUS_PHASES limit and cause an out-of-bounds read.
>
> Add a bounds check for info->phases against PMBUS_PHASES in the core API
> to prevent the potential out-of-bounds read.
>
> Cc: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
Looks like sashiko comment does not apply with the patch on the max20826
series. Hence:
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> drivers/hwmon/pmbus/pmbus_core.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 5104fd29307b..d69f1bde1795 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2982,6 +2982,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;
> + }
> ret = pmbus_identify_common(client, data, page);
> if (ret < 0) {
> dev_err(dev, "Failed to identify chip capabilities\n");
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 3+ messages in thread