drivers/hwmon/pmbus/adm1266.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)
Eight pre-existing bugs in the adm1266 driver's userspace-facing
accessors and probe ordering. Each is reachable any time userspace
touches an ADM1266 GPIO/PDIO line via the gpiolib char-dev or sysfs
interfaces, opens the nvmem device, or reads the sequencer_state
debugfs entry. Five landed when GPIO support was added (commit
d98dfad35c38), one when blackbox/NVMEM was added (commit
15609d189302), and one when the sequencer_state debugfs entry was
added (commit ed1ff457e187).
Patch 1 caps the PDIO scan loop in adm1266_gpio_get_multiple() at
ADM1266_PDIO_NR (16) instead of ADM1266_PDIO_STATUS (0xE9 = 233, a
PMBus command code that ended up in the bound by mistake). As
written, the scan walks find_next_bit() up to bit 242 across a
25-bit caller mask, reading out of bounds and -- if any of that
incidental memory contains a set bit -- driving a corresponding
out-of-bounds write to the caller's bits array.
Patch 2 drops a redundant "*bits = 0" reset that sits between the
GPIO and PDIO halves of adm1266_gpio_get_multiple(). As written,
the GPIO bits the first loop populates are immediately discarded
before the PDIO loop runs, so any caller asking for a mix of GPIO
and PDIO lines sees the GPIO half always reported as 0.
Patch 3 adds the missing "ret < 2" length check after the three
i2c_smbus_read_block_data() calls in adm1266_gpio_get() and
adm1266_gpio_get_multiple(). A device returning a 0- or 1-byte
response would otherwise compose pin status from uninitialised
stack memory and leak it to userspace via gpiolib.
Patch 4 moves adm1266_config_gpio() past pmbus_do_probe() in
adm1266_probe() so the gpio_chip isn't registered (and reachable
from userspace) until the PMBus state the GPIO accessors depend
on is initialised. This is a prerequisite for patch 6.
Patch 5 does the same for adm1266_config_nvmem(): the nvmem
device is now also registered after pmbus_do_probe(), so the
nvmem callback (adm1266_nvmem_read) cannot race
pmbus_do_probe()'s own device accesses.
Patch 6 takes pmbus_lock at the top of adm1266_gpio_get(),
adm1266_gpio_get_multiple(), and adm1266_gpio_dbg_show() so the
GPIO PMBus reads can't land between a PAGE write and the paged
read pmbus_core does in another thread.
Patch 7 takes pmbus_lock in adm1266_nvmem_read() so its memset /
blackbox refill / memcpy run as a single critical section. The
NVMEM core does not serialise concurrent reg_read invocations, so
without this two readers racing at offset 0 can interleave the
memset on data->dev_mem with another reader's memcpy to userspace
and return torn data. The same lock also serialises the refill's
PMBus traffic against pmbus_core's own PAGE+register sequences.
Patch 8 takes pmbus_lock in adm1266_state_read() (the
sequencer_state debugfs handler) for the same defensive-locking
reason: any direct device access from outside pmbus_core should
be ordered with respect to pmbus_core's own.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Changes in v3:
- New patch 5: register the nvmem device after pmbus_do_probe()
in adm1266_probe(). Same probe-ordering hazard the v2 patch 4
fixed for the gpio_chip, just for nvmem; flagged by Sashiko on
v2 and seconded by Guenter on-list.
- Patches 1 and 2: pick up Linus Walleij's Reviewed-by from his
reply to v1 [1].
- All previously-reviewed patches (1-4 of v2, now 1-4 of v3, plus
6 which is the v2 patch 5 renumbered): pick up Bartosz
Golaszewski's Reviewed-by from his reply to v2 [2].
- Patch 1: tighten the commit-message wording about how far the
unbounded scan reads past the end of the caller mask. v2 said
"up to 27 extra unsigned-long words", but the actual extent is
217 bits past gc.ngpio = 25, which works out to a handful of
long-word reads (four on 64-bit, seven on 32-bit) -- Sashiko
caught the arithmetic on v2. No code change.
- New patch 7: take pmbus_lock in adm1266_nvmem_read() so the
memset / blackbox refill / memcpy on data->dev_mem run as a
single critical section, and so the refill's PMBus traffic
serialises against pmbus_core's PAGE+register sequences.
Depends on the new patch 5 for the probe-ordering invariant:
without it, an early NVMEM read could fire before pmbus_do_probe()
has wired up the pmbus_core data, and the lock acquisition would
deref a NULL i2c_get_clientdata(). Flagged by Sashiko on v2.
- New patch 8: take pmbus_lock in adm1266_state_read() (the
sequencer_state debugfs handler) for the same defensive-locking
reason that motivates patch 6. adm1266_init_debugfs() already
runs after pmbus_do_probe() so no extra probe-ordering work is
needed. Flagged by Sashiko on v2.
[1] https://lore.kernel.org/r/CAD++jL=rasuYTot3M8u75PXRgrhbCzpue=pY2Yxx7ymVwhgGGQ@mail.gmail.com
[2] https://lore.kernel.org/r/CAMRc=Md_kjwH5v1JkQz5DxnzhK9yk1t+kjNcHG7P75bdg_16xA@mail.gmail.com
- Link to v2: https://patch.msgid.link/20260516-adm1266-gpio-fixes-v2-0-801f13debcb2@nexthop.ai
Changes in v2:
- New patch 3: reject short block-read responses in adm1266_gpio_get()
and adm1266_gpio_get_multiple(), so a 0- or 1-byte response from
the device cannot leak uninitialised stack memory to userspace
through the gpiolib interfaces (Sashiko review of v1).
- New patch 4: move adm1266_config_gpio() down past pmbus_do_probe()
in adm1266_probe() so the gpio_chip isn't reachable from userspace
before the PMBus state the GPIO accessors depend on is initialised.
Prerequisite for the new patch 5; without it the lock acquired by
the GPIO accessors would race adm1266_config_gpio() / pmbus_do_probe()
setup.
- New patch 5: take pmbus_lock in adm1266_gpio_get(),
adm1266_gpio_get_multiple(), and adm1266_gpio_dbg_show() so the
GPIO PMBus reads serialise against pmbus_core's PAGE+register
sequences (Sashiko review of v1).
- Patches 1 and 2 are unchanged from v1.
- Link to v1: https://patch.msgid.link/20260516-adm1266-gpio-fixes-v1-0-38d9dd39b905@nexthop.ai
To: Guenter Roeck <linux@roeck-us.net>
To: Linus Walleij <linusw@kernel.org>
To: Bartosz Golaszewski <brgl@kernel.org>
To: Alexandru Tachici <alexandru.tachici@analog.com>
Cc: linux-hwmon@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-gpio@vger.kernel.org
---
Abdurrahman Hussain (8):
hwmon: (pmbus/adm1266) cap PDIO scan in get_multiple at ADM1266_PDIO_NR
hwmon: (pmbus/adm1266) don't clobber GPIO bits before PDIO read in get_multiple
hwmon: (pmbus/adm1266) reject short block-read responses in the GPIO accessors
hwmon: (pmbus/adm1266) register the gpio_chip after pmbus_do_probe()
hwmon: (pmbus/adm1266) register the nvmem device after pmbus_do_probe()
hwmon: (pmbus/adm1266) serialize GPIO PMBus accesses with pmbus_lock
hwmon: (pmbus/adm1266) serialize NVMEM blackbox read with pmbus_lock
hwmon: (pmbus/adm1266) serialize sequencer_state debugfs read with pmbus_lock
drivers/hwmon/pmbus/adm1266.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
---
base-commit: 70eda68668d1476b459b64e69b8f36659fa9dfa8
change-id: 20260516-adm1266-gpio-fixes-dbdb9c10a4c2
Best regards,
--
Abdurrahman Hussain <abdurrahman@nexthop.ai>
On 5/18/26 17:52, Abdurrahman Hussain wrote: > Eight pre-existing bugs in the adm1266 driver's userspace-facing > accessors and probe ordering. Each is reachable any time userspace > touches an ADM1266 GPIO/PDIO line via the gpiolib char-dev or sysfs > interfaces, opens the nvmem device, or reads the sequencer_state > debugfs entry. Five landed when GPIO support was added (commit > d98dfad35c38), one when blackbox/NVMEM was added (commit > 15609d189302), and one when the sequencer_state debugfs entry was > added (commit ed1ff457e187). > > Patch 1 caps the PDIO scan loop in adm1266_gpio_get_multiple() at > ADM1266_PDIO_NR (16) instead of ADM1266_PDIO_STATUS (0xE9 = 233, a > PMBus command code that ended up in the bound by mistake). As > written, the scan walks find_next_bit() up to bit 242 across a > 25-bit caller mask, reading out of bounds and -- if any of that > incidental memory contains a set bit -- driving a corresponding > out-of-bounds write to the caller's bits array. > > Patch 2 drops a redundant "*bits = 0" reset that sits between the > GPIO and PDIO halves of adm1266_gpio_get_multiple(). As written, > the GPIO bits the first loop populates are immediately discarded > before the PDIO loop runs, so any caller asking for a mix of GPIO > and PDIO lines sees the GPIO half always reported as 0. > > Patch 3 adds the missing "ret < 2" length check after the three > i2c_smbus_read_block_data() calls in adm1266_gpio_get() and > adm1266_gpio_get_multiple(). A device returning a 0- or 1-byte > response would otherwise compose pin status from uninitialised > stack memory and leak it to userspace via gpiolib. > > Patch 4 moves adm1266_config_gpio() past pmbus_do_probe() in > adm1266_probe() so the gpio_chip isn't registered (and reachable > from userspace) until the PMBus state the GPIO accessors depend > on is initialised. This is a prerequisite for patch 6. > > Patch 5 does the same for adm1266_config_nvmem(): the nvmem > device is now also registered after pmbus_do_probe(), so the > nvmem callback (adm1266_nvmem_read) cannot race > pmbus_do_probe()'s own device accesses. > > Patch 6 takes pmbus_lock at the top of adm1266_gpio_get(), > adm1266_gpio_get_multiple(), and adm1266_gpio_dbg_show() so the > GPIO PMBus reads can't land between a PAGE write and the paged > read pmbus_core does in another thread. > > Patch 7 takes pmbus_lock in adm1266_nvmem_read() so its memset / > blackbox refill / memcpy run as a single critical section. The > NVMEM core does not serialise concurrent reg_read invocations, so > without this two readers racing at offset 0 can interleave the > memset on data->dev_mem with another reader's memcpy to userspace > and return torn data. The same lock also serialises the refill's > PMBus traffic against pmbus_core's own PAGE+register sequences. > > Patch 8 takes pmbus_lock in adm1266_state_read() (the > sequencer_state debugfs handler) for the same defensive-locking > reason: any direct device access from outside pmbus_core should > be ordered with respect to pmbus_core's own. > > Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai> Series applied. Thanks, Guenter
© 2016 - 2026 Red Hat, Inc.