Re: [PATCH] hwmon: (pmbus/adm1266) serialize firmware_revision debugfs read with pmbus_lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Out-of-bounds memory read/write and logic bug in `adm1266_gpio_get_multiple()`.
- [High] Direct device accessors lack the `pmbus_lock` required for serialization, contradicting the commit message.
- [High] GPIO and NVMEM devices are exposed to userspace before PMBus probe is complete, creating a race condition.
--

commit 93431d6d47217063e4487061d81fc6df727bcdb0
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) serialize firmware_revision debugfs read with pmbus_lock

adm1266_firmware_revision_read() backs the firmware_revision debugfs
entry and issues an i2c_smbus_read_block_data() without taking pmbus_lock.
This patch adds guard(pmbus_lock)(client) to serialize the read.

>     Take pmbus_lock at the top of adm1266_firmware_revision_read()
>     via the scope-based guard(), matching the pattern just applied to
>     adm1266_state_read() and the GPIO/NVMEM accessors.

[Severity: High]
This isn't a bug introduced by this patch, but the commit message asserts that
the guard(pmbus_lock) pattern was "just applied to adm1266_state_read() and the
GPIO/NVMEM accessors."

Does the current code actually do this? Looking at adm1266_state_read(),
adm1266_gpio_get(), adm1266_gpio_get_multiple(), and
adm1266_nvmem_read_blackbox(), they do not appear to use pmbus_lock. This
omission leaves direct I2C accesses unprotected and vulnerable to interleaving
with pmbus_core's multi-transaction sequences, which can corrupt hardware state
or interrupt PMBus transactions.

[Severity: High]
This is a pre-existing issue, but are GPIO and NVMEM devices exposed to
userspace before PMBus probe is complete?

In adm1266_probe(), adm1266_config_gpio() and adm1266_config_nvmem() are called
before pmbus_do_probe(). These functions register their respective devices,
immediately exposing them to userspace.

If userspace accesses these interfaces early, do the accessors run before PMBus
core structures are fully initialized? If the missing pmbus_lock were added to
these accessors (as the commit message assumes it was), wouldn't it retrieve a
NULL pointer from i2c_get_clientdata() and dereference it during pmbus_lock(),
causing a kernel crash?

[Severity: Critical]
This is a pre-existing issue, but is there an out-of-bounds memory read/write
and logic bug in adm1266_gpio_get_multiple()?

In adm1266_gpio_get_multiple(), the second bit-iteration loop incorrectly uses
the PMBus command byte ADM1266_PDIO_STATUS (0xE9 = 233) instead of the correct
pin count macro ADM1266_PDIO_NR (16) as the loop's upper limit.

Does this cause for_each_set_bit_from() to iterate up to bit 242? Since the
mask and bits variables from the GPIO subsystem are sized only for ngpio (25
bits, which fits in 1 unsigned long), does this result in out-of-bounds reads
on mask and out-of-bounds writes on bits via set_bit(), leading to severe
memory corruption?

Furthermore, doesn't executing *bits = 0; immediately before the second loop
inadvertently wipe out all the valid pin states gathered during the first loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-adm1266-fwrev-fix-v1-1-8a78c94a53ef@nexthop.ai?part=1