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

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

[Abdurrahman Hussain]

adm1266_firmware_revision_read() backs the firmware_revision debugfs
entry and issues an i2c_smbus_read_block_data(client,
ADM1266_IC_DEVICE_REV, buf) without taking pmbus_lock.  pmbus_core
holds pmbus_lock around its own multi-transaction sequences
(notably the "set PAGE, then read paged register" pattern used by
hwmon attributes), so an unlocked debugfs reader can land between
a PAGE write and the subsequent paged read in another thread.
IC_DEVICE_REV itself is not paged, so it cannot corrupt PAGE in
flight, but the same defensive serialisation applied to the other
adm1266 direct-device accessors applies here: any direct device
access from outside pmbus_core should be ordered with respect to
pmbus_core's own.

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.

Fixes: 7c99762af5c1 ("hwmon: (pmbus/adm1266) add firmware_revision debugfs entry")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Assisted-by: Claude-Code:claude-opus-4-7
---
The previous "GPIO, NVMEM, and debugfs accessor fixes" series [1]
locked all the adm1266 direct-device accessors except this one,
which slipped through because firmware_revision was already in
hwmon-next when the fixes were written.  Same defensive-locking
reason as adm1266_state_read() got there; same Fixes: shape
(stable backport candidate against the original firmware_revision
patch).

[1] https://lore.kernel.org/r/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai
---
 drivers/hwmon/pmbus/adm1266.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index aadca716fe7f..7f4dbc98d92a 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -359,6 +359,7 @@ static int adm1266_firmware_revision_read(struct seq_file *s, void *pdata)
 	u8 buf[I2C_SMBUS_BLOCK_MAX];
 	int ret;
 
+	guard(pmbus_lock)(client);
 	ret = i2c_smbus_read_block_data(client, ADM1266_IC_DEVICE_REV, buf);
 	if (ret < 0)
 		return ret;

---
base-commit: 7e63dac55e2de42a7947613c01e3d3c0fb9c15fc
change-id: 20260520-adm1266-fwrev-fix-a011f9be598a

Best regards,
--  
Abdurrahman Hussain <abdurrahman@nexthop.ai>

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

[Guenter Roeck]

On Wed, May 20, 2026 at 10:38:28AM -0700, Abdurrahman Hussain wrote:
> adm1266_firmware_revision_read() backs the firmware_revision debugfs
> entry and issues an i2c_smbus_read_block_data(client,
> ADM1266_IC_DEVICE_REV, buf) without taking pmbus_lock.  pmbus_core
> holds pmbus_lock around its own multi-transaction sequences
> (notably the "set PAGE, then read paged register" pattern used by
> hwmon attributes), so an unlocked debugfs reader can land between
> a PAGE write and the subsequent paged read in another thread.
> IC_DEVICE_REV itself is not paged, so it cannot corrupt PAGE in
> flight, but the same defensive serialisation applied to the other
> adm1266 direct-device accessors applies here: any direct device
> access from outside pmbus_core should be ordered with respect to
> pmbus_core's own.
> 
> 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.
> 
> Fixes: 7c99762af5c1 ("hwmon: (pmbus/adm1266) add firmware_revision debugfs entry")
> Cc: stable@vger.kernel.org
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> Assisted-by: Claude-Code:claude-opus-4-7
> ---
> The previous "GPIO, NVMEM, and debugfs accessor fixes" series [1]
> locked all the adm1266 direct-device accessors except this one,
> which slipped through because firmware_revision was already in
> hwmon-next when the fixes were written.  Same defensive-locking
> reason as adm1266_state_read() got there; same Fixes: shape
> (stable backport candidate against the original firmware_revision
> patch).

I completely forgot about this one. Thanks for remembering.
I squashed this patch into the firmware_revision patch.

Thanks!

Guenter

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

[sashiko-bot]

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