[PATCH v3 0/8] hwmon: (pmbus/adm1266) GPIO, NVMEM, and debugfs accessor fixes

[PATCH v3 0/8] hwmon: (pmbus/adm1266) GPIO, NVMEM, and debugfs accessor fixes

[Abdurrahman Hussain]

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>

[PATCH v3 1/8] hwmon: (pmbus/adm1266) cap PDIO scan in get_multiple at ADM1266_PDIO_NR

[Abdurrahman Hussain]

adm1266_gpio_get_multiple() iterates the PDIO portion of the
caller-supplied mask using

	for_each_set_bit_from(gpio_nr, mask,
			      ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
		...
	}

where ADM1266_PDIO_STATUS is the PMBus command code (0xE9, i.e. 233),
not the number of PDIO pins.  The intended upper bound is
ADM1266_GPIO_NR + ADM1266_PDIO_NR = 25.

gpiolib hands in a mask sized for gc.ngpio (= 25 bits on this chip),
so the iteration walks find_next_bit() up to 242, reading up to 217
extra bits (a handful of unsigned-long words: four on 64-bit, seven
on 32-bit) of whatever lives past the end of the mask in the
caller's stack.  Any incidental set bit in that range then drives a
set_bit(gpio_nr, bits) call that writes past the end of the
caller-supplied bits array too -- both out-of-bounds.

Substitute ADM1266_PDIO_NR for the constant so the scan stops at the
last real PDIO bit.

Fixes: d98dfad35c38 ("hwmon: (pmbus/adm1266) Add support for GPIOs")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Reviewed-by: Linus Walleij <linusw@kernel.org>
---
 drivers/hwmon/pmbus/adm1266.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index d90f8f80be8e..11f9a44f4361 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -211,7 +211,7 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
 	status = read_buf[0] + (read_buf[1] << 8);
 
 	*bits = 0;
-	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
+	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_NR) {
 		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
 			set_bit(gpio_nr, bits);
 	}

-- 
2.53.0

Re: [PATCH v3 1/8] hwmon: (pmbus/adm1266) cap PDIO scan in get_multiple at ADM1266_PDIO_NR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Overwriting previously set GPIO state in `*bits`.
- [High] Uninitialized stack memory use due to insufficient validation of `i2c_smbus_read_block_data`.
- [High] Heap buffer overflow in `adm1266_pmbus_block_xfer`.
- [High] Heap buffer overflow in `adm1266_nvmem_read_blackbox` due to a missing bounds check.
- [High] Data race and unprotected shared state modification in `adm1266_nvmem_read`.
--

commit 850ac6b65baa3669e7d4f1e67c46fb676ed9a96c
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) cap PDIO scan in get_multiple at ADM1266_PDIO_NR

This commit modifies the GPIO get_multiple accessor to stop the PDIO scan
at the correct upper bound. While reviewing this change, a few other issues
were noticed in the surrounding code, some of which are addressed by later
commits in the series.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index d90f8f80be8e0..11f9a44f4361a 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -211,7 +211,7 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
>  	status = read_buf[0] + (read_buf[1] << 8);
>  
>  	*bits = 0;

This isn't a bug introduced by this patch, but does this second assignment
to *bits unconditionally wipe out the GPIO states successfully collected in
the first loop for indices 0-8? 

I notice this is fixed later in the series by the commit "hwmon:
(pmbus/adm1266) don't clobber GPIO bits before PDIO read in get_multiple".

Also, is it possible for i2c_smbus_read_block_data() to return a short read
here?

drivers/hwmon/pmbus/adm1266.c:adm1266_gpio_get_multiple() {
    ...
	ret = i2c_smbus_read_block_data(data->client, ADM1266_PDIO_STATUS, read_buf);
	if (ret < 0)
		return ret;

	status = read_buf[0] + (read_buf[1] << 8);
    ...
}

If a short read occurs (returning 0 or 1 byte), does the status calculation
process uninitialized stack memory from read_buf? I see this is addressed
later in the series by "hwmon: (pmbus/adm1266) reject short block-read
responses in the GPIO accessors".

> -	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
> +	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_NR) {
>  		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
>  			set_bit(gpio_nr, bits);
>  	}

This isn't a bug introduced by this patch, but does this loop lack bounds
checking against the destination buffer size?

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
    ...
	record_count = buf[3];

	for (index = 0; index < record_count; index++) {
		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
		if (ret < 0)
			return ret;

		if (ret != ADM1266_BLACKBOX_SIZE)
			return -EIO;

		read_buff += ADM1266_BLACKBOX_SIZE;
	}
    ...
}

Since data->dev_mem is only 2048 bytes, if the hardware returns a
record_count greater than 32, does this overflow data->dev_mem?

Looking at the block transfer implementation, is there a potential heap buffer
overflow?

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
	struct i2c_msg msgs[2] = {
        ...
		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		}
	};
    ...
	ret = i2c_transfer(client->adapter, msgs, 2);
    ...
}

Since data->read_buf is allocated as ADM1266_PMBUS_BLOCK_MAX + 1 (256 bytes),
if a full block is read (or PEC is enabled and max data is received), could
i2c_transfer() overflow data->read_buf by writing 257 bytes into it?

Lastly, is there a missing lock protecting the shared data->dev_mem buffer
during reads?

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read() {
    ...
	if (offset == 0) {
		memset(data->dev_mem, 0, data->nvmem_config.size);

		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);
        ...
	}

	memcpy(val, data->dev_mem + offset, bytes);
    ...
}

Can a concurrent read at offset > 0 bypass initialization and read partially
updated or zeroed data while another thread executes memset and reads the
blackbox at offset == 0? 

I notice this is fixed later in the series by the commit "hwmon:
(pmbus/adm1266) serialize NVMEM blackbox read with pmbus_lock".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=1

[PATCH v3 2/8] hwmon: (pmbus/adm1266) don't clobber GPIO bits before PDIO read in get_multiple

[Abdurrahman Hussain]

adm1266_gpio_get_multiple() zeroes *bits before the GPIO_STATUS loop
and then a second time before the PDIO_STATUS loop:

	*bits = 0;
	for_each_set_bit(gpio_nr, mask, ADM1266_GPIO_NR) {
		...
		set_bit(gpio_nr, bits);
	}

	ret = i2c_smbus_read_block_data(data->client, ADM1266_PDIO_STATUS, ...);
	...
	*bits = 0;
	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_NR) {
		...
		set_bit(gpio_nr, bits);
	}

The second *bits = 0 throws away every GPIO bit the first loop just
populated, so callers asking for any combination of GPIO and PDIO
pins always see the GPIO portion of the returned bits as zero.

Drop the redundant second assignment so both halves of the result
survive.

Fixes: d98dfad35c38 ("hwmon: (pmbus/adm1266) Add support for GPIOs")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Reviewed-by: Linus Walleij <linusw@kernel.org>
---
 drivers/hwmon/pmbus/adm1266.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 11f9a44f4361..4dd67c02b412 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -210,7 +210,6 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
 
 	status = read_buf[0] + (read_buf[1] << 8);
 
-	*bits = 0;
 	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_NR) {
 		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
 			set_bit(gpio_nr, bits);

-- 
2.53.0

Re: [PATCH v3 2/8] hwmon: (pmbus/adm1266) don't clobber GPIO bits before PDIO read in get_multiple

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized stack memory use on short I2C block reads
- [High] Missing required locking for PMBus hardware accesses
--

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

hwmon: (pmbus/adm1266) don't clobber GPIO bits before PDIO read in get_multiple

This commit prevents the pdio loop in adm1266_gpio_get_multiple() from
overwriting previously populated gpio bits by dropping a redundant assignment.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 11f9a44f4361a..4dd67c02b412e 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -210,7 +210,6 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
>  
>  	status = read_buf[0] + (read_buf[1] << 8);

i2c_smbus_read_block_data() returns the number of bytes read on success.

If a short read occurs and fewer than two bytes are returned, does this
result in using uninitialized stack data for read_buf[1]?

I note this regression is fixed later in the series by a commit regarding
short block-read responses in the gpio accessors.

Additionally, the hardware monitoring subsystem guidelines require drivers
to implement locking for attributes registered by other means.

Can these unlocked pmbus reads in adm1266_gpio_get_multiple() interleave
with multi-message transactions from other threads, potentially corrupting
the active page state?

I see this regression is also resolved later in the series by a commit
that serializes gpio pmbus accesses with pmbus_lock.

> -	*bits = 0;
>  	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_NR) {
>  		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
>  			set_bit(gpio_nr, bits);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=2

[PATCH v3 3/8] hwmon: (pmbus/adm1266) reject short block-read responses in the GPIO accessors

[Abdurrahman Hussain]

adm1266_gpio_get() and adm1266_gpio_get_multiple() both compose the
pin-status word as

	pins_status = read_buf[0] + (read_buf[1] << 8);

right after i2c_smbus_read_block_data(), guarding only against an
error return.  A well-behaved device returns 2 bytes for
GPIO_STATUS/PDIO_STATUS, but the helper happily reports a 0- or
1-byte response too.  If the device returns 0 bytes, both read_buf
slots are uninitialized stack memory; if it returns 1 byte, read_buf[1]
is.

The composed value then flows through set_bit() into the caller's
*bits in adm1266_gpio_get_multiple(), or into the return value of
adm1266_gpio_get(), and ends up in userspace via gpiolib (sysfs and
the char-dev ioctls).  That leaks a few bits of kernel stack per
request on any device whose firmware glitch, bus error, or hostile
slave produces a short block-read response.

Add the missing length check to both call sites and surface a short
response as -EIO.

Fixes: d98dfad35c38 ("hwmon: (pmbus/adm1266) Add support for GPIOs")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/hwmon/pmbus/adm1266.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 4dd67c02b412..57cb7d302cdd 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -175,6 +175,8 @@ static int adm1266_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	ret = i2c_smbus_read_block_data(data->client, pmbus_cmd, read_buf);
 	if (ret < 0)
 		return ret;
+	if (ret < 2)
+		return -EIO;
 
 	pins_status = read_buf[0] + (read_buf[1] << 8);
 	if (offset < ADM1266_GPIO_NR)
@@ -195,6 +197,8 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
 	ret = i2c_smbus_read_block_data(data->client, ADM1266_GPIO_STATUS, read_buf);
 	if (ret < 0)
 		return ret;
+	if (ret < 2)
+		return -EIO;
 
 	status = read_buf[0] + (read_buf[1] << 8);
 
@@ -207,6 +211,8 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
 	ret = i2c_smbus_read_block_data(data->client, ADM1266_PDIO_STATUS, read_buf);
 	if (ret < 0)
 		return ret;
+	if (ret < 2)
+		return -EIO;
 
 	status = read_buf[0] + (read_buf[1] << 8);
 

-- 
2.53.0

Re: [PATCH v3 3/8] hwmon: (pmbus/adm1266) reject short block-read responses in the GPIO accessors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Stack buffer overflow in adm1266_nvmem_read_blackbox() caused by untrusted device response.
- [Critical] Heap buffer overflow in adm1266_nvmem_read_blackbox() due to missing bounds check on record_count.
- [High] Mismatched buffer sizes in adm1266_pmbus_block_xfer() cause an out-of-bounds heap read and write.
--

commit 54bf8f3eaa13fb13d4eb4adf5a3d15d39e6a5e7a
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) reject short block-read responses in the GPIO accessors

This commit adds length checks to adm1266_gpio_get() and
adm1266_gpio_get_multiple() after calling i2c_smbus_read_block_data(). This
prevents uninitialized stack memory from being returned to userspace via
gpiolib when a device returns a short block-read response.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 4dd67c02b412e..57cb7d302cddd 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c

[ ... ]

This isn't a bug introduced by this patch, but while addressing short
block-read responses, there appears to be a related stack buffer overflow
caused by untrusted device responses:

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
    ...
	u8 buf[5];
	int ret;

	ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);
    ...
}

The SMBus block read function can copy up to 32 bytes into the buffer before
returning the actual length. If a malfunctioning I2C device returns a block
length greater than 5, could this overflow the 5-byte stack buffer buf?

This isn't a bug introduced by this patch, but there might also be a heap
buffer overflow due to a missing bounds check on record_count:

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
    ...
	record_count = buf[3];

	for (index = 0; index < record_count; index++) {
		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
        ...
		read_buff += ADM1266_BLACKBOX_SIZE;
	}
    ...
}

The record_count is read directly from buf[3] without validation and used as
the loop upper limit to iteratively read 64-byte records into read_buff.

Since read_buff points to data->dev_mem, which is allocated with a fixed size
of 2048 bytes (allowing a maximum of 32 records), what happens if a device
returns a record_count larger than 32? Could the loop overflow the heap
buffer by writing up to 16,320 bytes?

This isn't a bug introduced by this patch, but there is a mismatch in buffer
sizes in adm1266_pmbus_block_xfer():

drivers/hwmon/pmbus/adm1266.c:
struct adm1266_data {
    ...
	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
};

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		},
    ...
	return i2c_transfer(client->adapter, msgs, 2);
}

The array data->read_buf is 256 bytes, but the read message defines its length
as 257 bytes. Will i2c_transfer() read up to 257 bytes into the 256-byte
buffer and cause a heap out-of-bounds write?

Additionally, if the PEC flag is enabled and the device returns a maximum
length of 255:

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
	if (client->flags & I2C_CLIENT_PEC) {
		if (i2c_smbus_msg_pec(0, &msgs[0], &msgs[1]) !=
		    msgs[1].buf[msgs[1].buf[0] + 1])
			return -EBADMSG;
    ...
}

Would the CRC validation access msgs[1].buf[256], performing an out-of-bounds
read past the end of the 256-byte array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=3

[PATCH v3 4/8] hwmon: (pmbus/adm1266) register the gpio_chip after pmbus_do_probe()

[Abdurrahman Hussain]

adm1266_probe() calls adm1266_config_gpio() -- which goes on to
devm_gpiochip_add_data() and exposes the gpio_chip callbacks to
gpiolib -- before pmbus_do_probe() has initialised the per-client
PMBus state (notably the pmbus_lock mutex the core hands out via
pmbus_get_data()).

That ordering is already a latent hazard: any GPIO access that lands
between adm1266_config_gpio() and the end of pmbus_do_probe() (for
example a sysfs read from a user space agent that opens the gpiochip
the instant gpiolib advertises it) races pmbus_do_probe()'s own
device accesses with no serialisation.

Move adm1266_config_gpio() down past pmbus_do_probe() so the chip
isn't reachable from userspace until the PMBus state it depends on
is fully initialised.

Fixes: d98dfad35c38 ("hwmon: (pmbus/adm1266) Add support for GPIOs")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/hwmon/pmbus/adm1266.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 57cb7d302cdd..b91dcf067fa6 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -467,10 +467,6 @@ static int adm1266_probe(struct i2c_client *client)
 	crc8_populate_msb(pmbus_crc_table, 0x7);
 	mutex_init(&data->buf_mutex);
 
-	ret = adm1266_config_gpio(data);
-	if (ret < 0)
-		return ret;
-
 	ret = adm1266_set_rtc(data);
 	if (ret < 0)
 		return ret;
@@ -483,6 +479,10 @@ static int adm1266_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	ret = adm1266_config_gpio(data);
+	if (ret < 0)
+		return ret;
+
 	adm1266_init_debugfs(data);
 
 	return 0;

-- 
2.53.0

Re: [PATCH v3 4/8] hwmon: (pmbus/adm1266) register the gpio_chip after pmbus_do_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Massive heap buffer overflow in adm1266_nvmem_read_blackbox() via untrusted record_count.
- [High] Deterministic 1-byte heap out-of-bounds write in adm1266_pmbus_block_xfer() due to mismatched buffer sizes.
--

commit 1aa8682477a4237d507fab67a50b6b1b23c803b4
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) register the gpio_chip after pmbus_do_probe()

This commit changes the order of operations in adm1266_probe() to register
the gpio_chip only after pmbus_do_probe() has fully initialized the PMBus
state. This prevents a race condition where userspace could access the
uninitialized PMBus state via gpiolib.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 57cb7d302cddd..b91dcf067fa6d 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c

[ ... ]

This problem wasn't introduced by this patch, but can a malfunctioning or
malicious I2C device returning a large record_count cause a heap buffer
overflow here?

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
...
	record_count = buf[3];

	for (index = 0; index < record_count; index++) {
		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
		if (ret < 0)
			return ret;

		if (ret != ADM1266_BLACKBOX_SIZE)
			return -EIO;

		read_buff += ADM1266_BLACKBOX_SIZE;
	}
...
}

The record_count is read directly from the I2C device into buf[3] (an 8-bit
unsigned value, up to 255). The function iterates record_count times without
an upper bound validation, advancing and writing 64 bytes to read_buff on each
iteration.

Since read_buff points to data->dev_mem which is exactly 2048 bytes,
could this write up to 16,320 bytes and overflow the buffer?


This problem wasn't introduced by this patch, but is it possible for this I2C
transfer to write past the end of data->read_buf?

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
...
	struct i2c_msg msgs[2] = {
		{
...
		},
		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		}
	};
...
	ret = i2c_transfer(client->adapter, msgs, 2);
...
}

data->read_buf is defined with size ADM1266_PMBUS_BLOCK_MAX + 1 (256 bytes).
However, msgs[1].len is hardcoded to ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes).

Because this is a raw I2C_M_RD transfer, the I2C adapter will blindly write
exactly 257 bytes into data->read_buf.

Since read_buf is the last member of the devm_kzalloc-allocated adm1266_data
struct, could this unconditionally corrupt adjacent heap memory on every
block read transfer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=4

[PATCH v3 5/8] hwmon: (pmbus/adm1266) register the nvmem device after pmbus_do_probe()

[Abdurrahman Hussain]

adm1266_probe() calls adm1266_config_nvmem() -- which goes on to
devm_nvmem_register() and exposes adm1266_nvmem_read() to userspace --
before pmbus_do_probe() has initialised the per-client PMBus state.

Same latent hazard as the gpio_chip one fixed in the previous patch:
once the nvmem device is registered, gpiolib's nvmem char-dev / sysfs
interface is reachable, and any concurrent read triggers
adm1266_nvmem_read() -> adm1266_nvmem_read_blackbox(), which issues
PMBus traffic that races pmbus_do_probe()'s own device accesses with
no serialisation.

Move adm1266_config_nvmem() down past pmbus_do_probe() so the nvmem
device isn't reachable from userspace until the PMBus state the
nvmem accessors depend on is fully initialised.

Fixes: 15609d189302 ("hwmon: (pmbus/adm1266) read blackbox")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index b91dcf067fa6..8b9fbb99a4bd 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -471,14 +471,14 @@ static int adm1266_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	ret = adm1266_config_nvmem(data);
-	if (ret < 0)
-		return ret;
-
 	ret = pmbus_do_probe(client, &data->info);
 	if (ret)
 		return ret;
 
+	ret = adm1266_config_nvmem(data);
+	if (ret < 0)
+		return ret;
+
 	ret = adm1266_config_gpio(data);
 	if (ret < 0)
 		return ret;

-- 
2.53.0

Re: [PATCH v3 5/8] hwmon: (pmbus/adm1266) register the nvmem device after pmbus_do_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unbounded loop in adm1266_nvmem_read_blackbox allows a heap buffer overflow triggered by an untrusted I2C device response.
- [High] Length validation in adm1266_nvmem_read_blackbox occurs after memcpy, leading to heap corruption.
- [High] 1-byte static out-of-bounds read/write on the heap during every adm1266_pmbus_block_xfer I2C transaction.
- [High] NVMEM reads at non-zero offsets skip device reads and return uninitialized or stale data.
--

commit 1d749e51ad745af23cd77d7854a264e1e6247407
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) register the nvmem device after pmbus_do_probe()

adm1266_probe() calls adm1266_config_nvmem() before pmbus_do_probe() has
initialised the per-client PMBus state. This moves adm1266_config_nvmem()
down past pmbus_do_probe() so the nvmem device isn't reachable from userspace
until the PMBus state is fully initialised.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index b91dcf067fa6d..8b9fbb99a4bda 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -471,14 +471,14 @@ static int adm1266_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = adm1266_config_nvmem(data);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = pmbus_do_probe(client, &data->info);
>  	if (ret)
>  		return ret;
>  
> +	ret = adm1266_config_nvmem(data);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = adm1266_config_gpio(data);
>  	if (ret < 0)
>  		return ret;

While reviewing this patch, I noticed some pre-existing issues in the NVMEM
handling and I2C transfers.

Could this loop in adm1266_nvmem_read_blackbox() cause a heap buffer overflow?

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
    ...
	record_count = buf[3];

	for (index = 0; index < record_count; index++) {
		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
		if (ret < 0)
			return ret;

		if (ret != ADM1266_BLACKBOX_SIZE)
			return -EIO;

		read_buff += ADM1266_BLACKBOX_SIZE;
	}
    ...
}

The function reads record_count directly from the device response, which can be
up to 255.

It then loops record_count times, appending 64 bytes into data->dev_mem.

Since data->dev_mem is allocated with a fixed size of 2048 bytes, could a
record_count greater than 32 linearly write past the end of the buffer and
corrupt kernel heap memory?

Additionally, does the length validation happen too late?

Inside the loop, adm1266_pmbus_block_xfer() copies up to 255 bytes into the
current position of read_buff.

The length check (ret != ADM1266_BLACKBOX_SIZE) is performed after the copy.

If read_buff is near the end of the 2048-byte buffer, could copying up to 255
bytes silently overflow the heap before the error is caught?

There also appears to be a 1-byte out-of-bounds access in
adm1266_pmbus_block_xfer():

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
	struct i2c_msg msgs[2] = {
        ...
		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		}
	};
    ...
		if (crc != msgs[1].buf[msgs[1].buf[0] + 1]) {
			mutex_unlock(&data->buf_mutex);
			return -EBADMSG;
		}
    ...
}

The read_buf in struct adm1266_data is allocated with a size of 256 bytes
(ADM1266_PMBUS_BLOCK_MAX + 1).

However, the i2c_msg requests a length of 257 bytes. Doesn't this instruct
the I2C master to write up to 257 bytes into the 256-byte buffer,
overflowing it by 1 byte?

Also, if the device returns a length of 255, the PEC validation logic
accesses msgs[1].buf[msgs[1].buf[0] + 1], which reads index 256. Does this
result in an out-of-bounds read?

Finally, could adm1266_nvmem_read() return uninitialized or stale data?

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read() {
    ...
	if (offset == 0) {
		memset(data->dev_mem, 0, data->nvmem_config.size);

		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);
        ...
	}

	memcpy(val, data->dev_mem + offset, bytes);

	return 0;
}

If a user performs a read starting at an offset greater than 0, the device
read is skipped entirely.

Since data->dev_mem is allocated with devm_kzalloc, doesn't this return pure
zeros if no prior read at offset 0 has occurred, breaking standard
random-access file semantics?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=5

[PATCH v3 6/8] hwmon: (pmbus/adm1266) serialize GPIO PMBus accesses with pmbus_lock

[Abdurrahman Hussain]

adm1266_gpio_get(), adm1266_gpio_get_multiple(), and
adm1266_gpio_dbg_show() all issue PMBus reads against the device but
none of them take pmbus_lock.  The pmbus_core framework 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 GPIO accessor can land between a PAGE
write and the subsequent paged read in another thread and corrupt
either side's view of the device state machine.

Take pmbus_lock at the top of each of the three accessors via the
scope-based guard().  The lock is uncontended in the common case and
adds only a single mutex round-trip per call.

Fixes: d98dfad35c38 ("hwmon: (pmbus/adm1266) Add support for GPIOs")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/hwmon/pmbus/adm1266.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 8b9fbb99a4bd..a80fb2ea73bd 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -172,6 +172,8 @@ static int adm1266_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	else
 		pmbus_cmd = ADM1266_PDIO_STATUS;
 
+	guard(pmbus_lock)(data->client);
+
 	ret = i2c_smbus_read_block_data(data->client, pmbus_cmd, read_buf);
 	if (ret < 0)
 		return ret;
@@ -194,6 +196,8 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
 	unsigned int gpio_nr;
 	int ret;
 
+	guard(pmbus_lock)(data->client);
+
 	ret = i2c_smbus_read_block_data(data->client, ADM1266_GPIO_STATUS, read_buf);
 	if (ret < 0)
 		return ret;
@@ -235,6 +239,8 @@ static void adm1266_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	int ret;
 	int i;
 
+	guard(pmbus_lock)(data->client);
+
 	for (i = 0; i < ADM1266_GPIO_NR; i++) {
 		write_cmd = adm1266_gpio_mapping[i][1];
 		ret = adm1266_pmbus_block_xfer(data, ADM1266_GPIO_CONFIG, 1, &write_cmd, read_buf);

-- 
2.53.0

Re: [PATCH v3 6/8] hwmon: (pmbus/adm1266) serialize GPIO PMBus accesses with pmbus_lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Heap buffer overflow and infinite loop in adm1266_nvmem_read_blackbox() due to unvalidated device input
- [Critical] Out-of-bounds read and write in adm1266_pmbus_block_xfer() due to undersized read_buf
--

commit 72c9d4f76b145894590481f2c71edfe0ff2e57c9
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
hwmon: (pmbus/adm1266) serialize GPIO PMBus accesses with pmbus_lock

This commit adds guard(pmbus_lock) to GPIO accessors in the adm1266 driver.
This ensures the pmbus_lock is held during multi-transaction sequences,
preventing other threads from corrupting the device state machine.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 8b9fbb99a4bda..a80fb2ea73bdb 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> @@ -235,6 +239,8 @@ static void adm1266_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  	int ret;
>  	int i;
>  
> +	guard(pmbus_lock)(data->client);
> +
>  	for (i = 0; i < ADM1266_GPIO_NR; i++) {
>  		write_cmd = adm1266_gpio_mapping[i][1];
>  		ret = adm1266_pmbus_block_xfer(data, ADM1266_GPIO_CONFIG, 1, &write_cmd, read_buf);

This isn't a bug introduced by this patch, but could this call to
adm1266_pmbus_block_xfer() trigger an out-of-bounds read and write due
to an undersized read_buf?

In struct adm1266_data, read_buf is sized at ADM1266_PMBUS_BLOCK_MAX + 1
(256 bytes):

struct adm1266_data {
	...
	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
};

However, adm1266_pmbus_block_xfer() constructs an I2C read message with a length
of ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes):

	struct i2c_msg msgs[2] = {
		...
		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		}
	};

If the I2C device returns the maximum allowable payload, will the I2C controller
write 257 bytes into the 256-byte buffer and corrupt adjacent memory?

Additionally, during PEC verification, the code evaluates the length byte:

	if (crc != msgs[1].buf[msgs[1].buf[0] + 1]) {

If the device returns a length byte of 255, this calculates an index of 256.
Does this cause an out-of-bounds read of the 257th byte?

Also not introduced by this patch, but does adm1266_nvmem_read_blackbox() have
a potential heap buffer overflow and infinite loop?

The function reads record_count from the I2C peripheral without bounds checking:

static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
{
	int record_count;
	char index;
	u8 buf[5];
	...
	record_count = buf[3];

	for (index = 0; index < record_count; index++) {
		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
		...
		read_buff += ADM1266_BLACKBOX_SIZE;
	}
	return 0;
}

Since record_count can be up to 255 and each iteration writes 64 bytes
(ADM1266_BLACKBOX_SIZE), wouldn't a record_count greater than 32 overflow
the 2048-byte data->dev_mem destination buffer?

Furthermore, index is declared as a char. On architectures where char is signed,
if the device returns a record_count between 128 and 255, index will increment
to 127 and then wrap around to -128.

Since -128 is less than the positive record_count, does the loop condition
remain true forever and lead to an infinite loop of sequential out-of-bounds
memory writes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=6

[PATCH v3 7/8] hwmon: (pmbus/adm1266) serialize NVMEM blackbox read with pmbus_lock

[Abdurrahman Hussain]

adm1266_nvmem_read() is the reg_read callback the NVMEM core invokes
when userspace reads /sys/bus/nvmem/devices/.../nvmem on this chip.
On the first byte of every read it does a memset of data->dev_mem,
walks the device blackbox through adm1266_nvmem_read_blackbox()
(which issues a chain of PMBus block transactions), and then memcpys
the refreshed buffer out to userspace.  None of that runs under
pmbus_lock today.

Two consequences:

  - The PMBus traffic the refresh issues is not serialised against
    pmbus_core's own multi-step PAGE+register sequences.  A paged
    hwmon attribute read from another thread can land between a
    PAGE write and the paged read in either direction and corrupt
    one side's view of the device state machine.

  - The NVMEM core does not serialise concurrent reg_read calls, so
    two userspace readers racing at offset 0 can interleave the
    memset of data->dev_mem with another reader's
    adm1266_nvmem_read_blackbox() refill or memcpy out, returning
    torn data to userspace.

Take pmbus_lock at the top of adm1266_nvmem_read() via the
scope-based guard().  Patch 5 of this series moves
adm1266_config_nvmem() past pmbus_do_probe() so the lock is
guaranteed to be live before the callback is reachable from
userspace.

Fixes: 15609d189302 ("hwmon: (pmbus/adm1266) read blackbox")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index a80fb2ea73bd..051f4f188ec5 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -394,6 +394,8 @@ static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t
 	if (offset + bytes > data->nvmem_config.size)
 		return -EINVAL;
 
+	guard(pmbus_lock)(data->client);
+
 	if (offset == 0) {
 		memset(data->dev_mem, 0, data->nvmem_config.size);
 

-- 
2.53.0

Re: [PATCH v3 7/8] hwmon: (pmbus/adm1266) serialize NVMEM blackbox read with pmbus_lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing bounds check on hardware-provided `record_count` causes massive out-of-bounds write and potential infinite loop.
- [Critical] Unvalidated hardware-controlled length byte causes out-of-bounds memcpy.
- [High] 1-byte Out-of-Bounds write in I2C DMA buffer and out-of-bounds PEC array access.
--

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

hwmon: (pmbus/adm1266) serialize NVMEM blackbox read with pmbus_lock

adm1266_nvmem_read() is the reg_read callback the NVMEM core invokes
when userspace reads /sys/bus/nvmem/devices/.../nvmem on this chip.
[ ... ]

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index a80fb2ea73bdb..051f4f188ec52 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -394,6 +394,8 @@ static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t
>  	if (offset + bytes > data->nvmem_config.size)
>  		return -EINVAL;
>  
> +	guard(pmbus_lock)(data->client);
> +
>  	if (offset == 0) {
>  		memset(data->dev_mem, 0, data->nvmem_config.size);
>  
> 		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);

This isn't a bug introduced by this patch, but does this missing bounds check
on the hardware-provided record_count cause an out-of-bounds write?

In adm1266_nvmem_read_blackbox(), record_count is read directly from the
device and can be up to 255:

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
    ...
    ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);
    ...
    record_count = buf[3];

    for (index = 0; index < record_count; index++) {
        ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
        ...
        read_buff += ADM1266_BLACKBOX_SIZE;
    }
    ...
}

Since read_buff points to data->dev_mem, which is fixed at 2048 bytes, a
record_count > 32 will write out of bounds since each block is 64 bytes
(ADM1266_BLACKBOX_SIZE).

Additionally, if the architecture has a signed char, could a record_count > 127
cause index to wrap around to -128, creating an infinite loop?


This isn't a bug introduced by this patch, but could this memcpy() overflow
the buffer?

In adm1266_pmbus_block_xfer(), the length of the read block is determined by
msgs[1].buf[0], which can be up to 255:

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
    memcpy(data_r, &msgs[1].buf[1], msgs[1].buf[0]);
    ...
}

When called from adm1266_nvmem_read_blackbox(), data_r points to a position
in data->dev_mem expecting only 64 bytes. If the device returns a length > 64,
this will corrupt memory before the safeguard check in the caller is reached.


This isn't a bug introduced by this patch, but could this I2C message length
cause an out-of-bounds write?

In adm1266_data, read_buf is 256 bytes (ADM1266_PMBUS_BLOCK_MAX + 1):

drivers/hwmon/pmbus/adm1266.c: {
    ...
    u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
    ...
}

However, adm1266_pmbus_block_xfer() configures the read length to 257 bytes
(ADM1266_PMBUS_BLOCK_MAX + 2):

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
        {
            .addr = client->addr,
            .flags = I2C_M_RD | I2C_M_DMA_SAFE,
            .buf = data->read_buf,
            .len = ADM1266_PMBUS_BLOCK_MAX + 2,
        }
    ...
}

This allows the I2C controller to write up to 257 bytes into the 256-byte
array.

Furthermore, if PEC is enabled and the device returns a block length of 255
(msgs[1].buf[0] == 255), does the PEC validation access index 256, which is
past the end of the array?

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
        crc = crc8(pmbus_crc_table, msgs[1].buf,  msgs[1].buf[0] + 1, crc);

        if (crc != msgs[1].buf[msgs[1].buf[0] + 1]) {
    ...
}

> 		if (ret) {
> 			dev_err(&data->client->dev, "Could not read blackbox!");
> 			return ret;
> 		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=7

[PATCH v3 8/8] hwmon: (pmbus/adm1266) serialize sequencer_state debugfs read with pmbus_lock

[Abdurrahman Hussain]

adm1266_state_read() backs the sequencer_state debugfs entry and
issues an i2c_smbus_read_word_data(client, ADM1266_READ_STATE)
against the device 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.  READ_STATE
itself is not paged, so it cannot corrupt PAGE in flight, but the
same defensive serialisation that applies to the GPIO 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_state_read() via the
scope-based guard().

Fixes: ed1ff457e187 ("hwmon: (pmbus/adm1266) add debugfs for states")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@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 051f4f188ec5..605db086236c 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -333,6 +333,7 @@ static int adm1266_state_read(struct seq_file *s, void *pdata)
 	struct i2c_client *client = to_i2c_client(dev);
 	int ret;
 
+	guard(pmbus_lock)(client);
 	ret = i2c_smbus_read_word_data(client, ADM1266_READ_STATE);
 	if (ret < 0)
 		return ret;

-- 
2.53.0

Re: [PATCH v3 0/8] hwmon: (pmbus/adm1266) GPIO, NVMEM, and debugfs accessor fixes

[Guenter Roeck]

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