[PATCH v2 0/5] hwmon: (pmbus/adm1266) GPIO accessor fixes

[PATCH v2 0/5] hwmon: (pmbus/adm1266) GPIO accessor fixes

[Abdurrahman Hussain]

Five pre-existing bugs in the adm1266 GPIO path that all landed when
GPIO support was first added (commit d98dfad35c38).  Each is
reachable any time userspace queries an ADM1266 GPIO/PDIO line via
the gpiolib char-dev or sysfs interfaces, or reads
debugfs/gpio-<chip>.

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 5.

Patch 5 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.

Signed-off-by: Abdurrahman Hussain <abdurrahman@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 (5):
      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) serialize GPIO PMBus accesses with pmbus_lock

 drivers/hwmon/pmbus/adm1266.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)
---
base-commit: 70eda68668d1476b459b64e69b8f36659fa9dfa8
change-id: 20260516-adm1266-gpio-fixes-dbdb9c10a4c2

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

[PATCH v2 1/5] 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 27
extra unsigned-long words 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>
---
 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 v2 1/5] hwmon: (pmbus/adm1266) cap PDIO scan in get_multiple at ADM1266_PDIO_NR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] This problem wasn't introduced by this patch, but the adm1266_nvmem_read_blackbox function allocates an undersized 5-byte stack buffer and passes it to i2c_smbus_read_block_data, which can copy up to 32 bytes.
- [Critical] This problem wasn't introduced by this patch, but a missing bounds check on the device-controlled record_count leads to a severe heap buffer overflow in adm1266_nvmem_read_blackbox.
- [High] This problem wasn't introduced by this patch, but there is an off-by-one heap buffer overflow and out-of-bounds read in adm1266_pmbus_block_xfer due to an undersized read_buf array.
- [Medium] The commit message inaccurately exaggerates the out-of-bounds access size.
--

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

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

This commit modifies the PDIO mask iteration in adm1266_gpio_get_multiple()
to use the correct upper bound constant to prevent an out-of-bounds read.

> 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 27
> extra unsigned-long words of whatever lives past the end of the mask

This isn't a bug, but is this calculation accurate?

The difference between 242 bits and 25 bits is 217 bits, which is roughly
27 bytes.

Wouldn't this result in reading only 3 extra unsigned-long words on a 64-bit
architecture (or 7 on a 32-bit architecture), rather than 27?

While reviewing this file, I noticed a few pre-existing regressions in the
surrounding code.

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

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

Does this overflow the buf[] array on the stack?

The i2c_smbus_read_block_data() API expects a destination buffer of at least
I2C_SMBUS_BLOCK_MAX (32 bytes) and can copy up to 32 bytes into the buffer.

The buf array is only 5 bytes long.

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;
	}
    ...
}

Does this loop overflow the data->dev_mem buffer?

Since record_count is read directly from the I2C device (buf[3]), it could
be up to 255.

The read_buff pointer targets data->dev_mem, which is allocated to 2048 bytes.

A record_count greater than 32 writes up to 16,320 bytes into the buffer.

Should we bounds-check record_count?

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,
		},
    ...
}

Does this overflow the data->read_buf array?

The data->read_buf array is sized at ADM1266_PMBUS_BLOCK_MAX + 1 (256 bytes).

The length passed to the I2C transfer is ADM1266_PMBUS_BLOCK_MAX + 2
(257 bytes).

Additionally, if the device returns a block length of 255, wouldn't the PEC
validation logic access the 257th byte, causing an out-of-bounds read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-gpio-fixes-v2-0-801f13debcb2@nexthop.ai?part=1

[PATCH v2 2/5] 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>
---
 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

[PATCH v2 3/5] 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>
---
 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 v2 3/5] 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] Missing bounds check on device-provided `record_count` causes a kernel buffer overflow in `adm1266_nvmem_read_blackbox`.
- [Critical] Unbounded memcpy using device-provided length in `adm1266_pmbus_block_xfer()`.
- [High] Off-by-one out-of-bounds write in `data->read_buf` during I2C transfer.
--

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

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

This commit adds a length check for block-read responses in the
adm1266_gpio_get() and adm1266_gpio_get_multiple() functions. It addresses
an issue where short block-read responses of 0 or 1 bytes could result in
reading uninitialized stack memory.

> 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

[ ... ]

While reviewing this code, I noticed a few pre-existing issues in this file.

Could this cause a kernel buffer overflow? In adm1266_nvmem_read_blackbox(),
record_count is read directly from the I2C device and never validated against
safe bounds:

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);
		if (ret < 0)
			return ret;

		if (ret != ADM1266_BLACKBOX_SIZE)
			return -EIO;

		read_buff += ADM1266_BLACKBOX_SIZE;
	}
    ...
}

The loop advances read_buff (which points to the dynamically allocated
2048-byte data->dev_mem buffer) by 64 bytes on each iteration. If a
malfunctioning or malicious device returns a record_count greater than 32,
it will write past the end of the data->dev_mem buffer.

Additionally, because the loop variable index is a char, on architectures
where char is signed, a record_count of 128 or more will cause index to
wrap to -128, resulting in an infinite loop of out-of-bounds writes until
the kernel panics.

Does adm1266_pmbus_block_xfer() perform an unbounded memcpy() using a
device-provided length?

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

	ret = msgs[1].buf[0];
	mutex_unlock(&data->buf_mutex);

	return ret;
}

The length parameter msgs[1].buf[0] is controlled by the I2C device and can
be up to 255. While this is safe for callers providing a 256-byte buffer,
adm1266_nvmem_read_blackbox() passes a pointer inside the 2048-byte
data->dev_mem buffer and only expects 64 bytes per transaction.

A device returning a length greater than 64 will cause an immediate
out-of-bounds write beyond the expected 64-byte block, and if it occurs near
the end of the 2048-byte buffer, it will overflow the kernel heap.

Can this result in an off-by-one out-of-bounds write during an I2C transfer?

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() {
    ...
	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,
		}
	};
    ...
}

The read_buf array in struct adm1266_data is declared with a size of
ADM1266_PMBUS_BLOCK_MAX + 1 (256 bytes). However, the I2C read message
length is set to ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes) to accommodate
the length byte and an optional PEC byte.

If the hardware responds with the maximum block size and PEC is enabled,
the I2C controller will write 257 bytes into the 256-byte read_buf.
Because read_buf is the last member of the dynamically allocated struct
adm1266_data and no trailing padding exists to absorb it, this would write
1 byte out-of-bounds on the kernel heap.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-gpio-fixes-v2-0-801f13debcb2@nexthop.ai?part=3

[PATCH v2 4/5] 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>
---
 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 v2 4/5] hwmon: (pmbus/adm1266) register the gpio_chip after pmbus_do_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Out-of-bounds write of 1 byte in adm1266_pmbus_block_xfer() due to I2C transfer length exceeding buffer size.
- [High] Missing PMBus core locking in NVMEM accessors violates hwmon subsystem guidelines.
- [High] Incomplete fix: NVMEM device is exposed before PMBus initialization completes, preserving a probe race condition.
--

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

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

adm1266_probe() calls adm1266_config_gpio() before pmbus_do_probe() has
initialised the per-client PMBus state. This latent hazard allows a sysfs
read from a user space agent to race pmbus_do_probe()'s own device
accesses. Move adm1266_config_gpio() down past pmbus_do_probe() to avoid
this race.

Fixes: d98dfad35c38 ("hwmon: (pmbus/adm1266) Add support for GPIOs")
Cc: stable@vger.kernel.org

> 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
> @@ -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;
> +

Does this code leave the nvmem device exposed before PMBus initialization
completes?

While adm1266_config_gpio() is moved down past pmbus_do_probe(),
adm1266_config_nvmem() remains above it:

static int adm1266_probe(struct i2c_client *client)
{
	...
	ret = adm1266_config_nvmem(data);
	if (ret < 0)
		return ret;

	ret = pmbus_do_probe(client, &data->info);
	...
}

This means the nvmem device is immediately exposed to userspace before the
PMBus core is fully initialized. A concurrent nvmem read could trigger
adm1266_nvmem_read() and perform I2C accesses without serialization,
recreating the identical hazard this patch aims to fix for GPIOs.

Additionally, do the nvmem accessors implement the required locking?

The hwmon subsystem guidelines state that drivers must implement locking
for attributes registered outside the core. adm1266_nvmem_read() triggers
I2C transactions independently via adm1266_pmbus_block_xfer() without
taking pmbus_lock(). This could allow concurrent nvmem reads to interleave
with the PMBus core's multi-step sequences, potentially breaking device
state.

Could this lead to a memory corruption issue during I2C transfers?

While not introduced by this patch, it appears the read_buf array in
struct adm1266_data is allocated with 256 bytes and is exactly cacheline
aligned:

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

If adm1266_pmbus_block_xfer() sets the read message length to
ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes), wouldn't the I2C master controller
clock out and read exactly 257 bytes from the bus, resulting in an
out-of-bounds write of 1 byte past the end of the heap-allocated structure?

>  	adm1266_init_debugfs(data);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-gpio-fixes-v2-0-801f13debcb2@nexthop.ai?part=4

[PATCH v2 5/5] 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>
---
 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 b91dcf067fa6..2e9ae03ab64b 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 v2 5/5] hwmon: (pmbus/adm1266) serialize GPIO PMBus accesses with pmbus_lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Pre-existing Stack and Heap buffer overflows in `adm1266_nvmem_read_blackbox` from untrusted device input.
- [High] Incomplete Fix: NVMEM and debugfs accessors are left unlocked, and probe ordering prevents safely locking them.
- [High] Pre-existing out-of-bounds write and read in `adm1266_pmbus_block_xfer` due to off-by-one buffer size.
--

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

hwmon: (pmbus/adm1266) serialize GPIO PMBus accesses with pmbus_lock

This commit takes pmbus_lock at the top of GPIO accessors via the scope-based
guard() to serialize PMBus accesses and avoid corrupting the device state
machine.

> 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.

Are adm1266_nvmem_read() and adm1266_state_read() also vulnerable to this
interleaving race condition? They issue PMBus reads directly against the device
and can be triggered concurrently by userspace via debugfs, so it seems they
might also corrupt the device state machine if left unlocked.

If pmbus_lock is added to the NVMEM path, is the driver's probe sequence safe?
Currently, adm1266_probe() registers the NVMEM device before calling
pmbus_do_probe() which allocates the clientdata. If an early NVMEM read occurs
before pmbus_do_probe() completes, it seems i2c_get_clientdata() would return
NULL, which could cause a panic when dereferencing the lock.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index b91dcf067fa6d..2e9ae03ab64b0 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);

Does adm1266_pmbus_block_xfer() write past the end of the data->read_buf array?

The read_buf array in struct adm1266_data is declared as 256 bytes:

	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;

But adm1266_pmbus_block_xfer() initializes the I2C read message with a larger
length:

static int 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,
		}
	};
	...

Could this cause i2c_transfer() to write 257 bytes into the 256-byte array?

Additionally, if I2C_CLIENT_PEC is enabled and the device returns a max length
byte of 255, could the PEC validation logic perform an out-of-bounds read at
index 256?

While reviewing this file, I also noticed two other regressions in
adm1266_nvmem_read_blackbox().

First, it passes a local 5-byte array to i2c_smbus_read_block_data():

	u8 buf[5];
	...
	ret = i2c_smbus_read_block_data(client, ADM1266_READ_BLACKBOX_INFO, buf);

Since i2c_smbus_read_block_data() copies up to I2C_SMBUS_BLOCK_MAX (32) bytes
into the destination array based on the length returned by the device, could a
device returning a length greater than 5 overflow the buf stack array?

Second, it reads record_count from the device response:

	record_count = buf[3];

Then it uses this in a loop:

	for (i = 0; i < record_count; i++) {
		...
		read_buff += ADM1266_BLACKBOX_SIZE;
	}

Since data->dev_mem is allocated with a fixed size of 2048 bytes, and
ADM1266_BLACKBOX_SIZE is 64 bytes, if the device returns a record_count greater
than 32, could this loop write past the end of the data->dev_mem array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-gpio-fixes-v2-0-801f13debcb2@nexthop.ai?part=5

Re: [PATCH v2 0/5] hwmon: (pmbus/adm1266) GPIO accessor fixes

[Bartosz Golaszewski]

On Sun, 17 May 2026 01:18:46 +0200, Abdurrahman Hussain
<abdurrahman@nexthop.ai> said:
> Five pre-existing bugs in the adm1266 GPIO path that all landed when
> GPIO support was first added (commit d98dfad35c38).  Each is
> reachable any time userspace queries an ADM1266 GPIO/PDIO line via
> the gpiolib char-dev or sysfs interfaces, or reads
> debugfs/gpio-<chip>.
>
> 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 5.
>
> Patch 5 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.
>
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> ---

All these make sense to me.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH v2 0/5] hwmon: (pmbus/adm1266) GPIO accessor fixes

[Guenter Roeck]

Hi,

On 5/16/26 16:18, Abdurrahman Hussain wrote:
> Five pre-existing bugs in the adm1266 GPIO path that all landed when
> GPIO support was first added (commit d98dfad35c38).  Each is
> reachable any time userspace queries an ADM1266 GPIO/PDIO line via
> the gpiolib char-dev or sysfs interfaces, or reads
> debugfs/gpio-<chip>.
> 
> 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 5.
> 
> Patch 5 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.
> 
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>

Sashiko reported a number of additional problems. As far as I can
see those are real. Would you mind fixing those issues as well
as part of this series ?

Thanks,
Guenter

Re: [PATCH v2 0/5] hwmon: (pmbus/adm1266) GPIO accessor fixes

[Abdurrahman Hussain]

On Mon May 18, 2026 at 3:08 PM PDT, Guenter Roeck wrote:
> Hi,
>
> On 5/16/26 16:18, Abdurrahman Hussain wrote:
>> Five pre-existing bugs in the adm1266 GPIO path that all landed when
>> GPIO support was first added (commit d98dfad35c38).  Each is
>> reachable any time userspace queries an ADM1266 GPIO/PDIO line via
>> the gpiolib char-dev or sysfs interfaces, or reads
>> debugfs/gpio-<chip>.
>> 
>> 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 5.
>> 
>> Patch 5 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.
>> 
>> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> Sashiko reported a number of additional problems. As far as I can
> see those are real. Would you mind fixing those issues as well
> as part of this series ?
>
> Thanks,
> Guenter

Sure -- v3 (sending shortly) folds in everything Sashiko flagged on
v2 that isn't already covered by the "buffer-bound and timestamp
fixes" series you applied to hwmon-next:

  - New patch 5: register the nvmem device after pmbus_do_probe();
    same probe-ordering hazard v2 patch 4 fixed for the gpio_chip.
  - New patch 7: take pmbus_lock in adm1266_nvmem_read().
  - New patch 8: take pmbus_lock in adm1266_state_read().
  - Patch 1 commit-message wording fix (Sashiko corrected the
    "27 unsigned-long words" arithmetic; no code change).
  - Reviewed-by tags from Linus Walleij (patches 1, 2) and
    Bartosz Golaszewski (the rest).

Thanks,
Abdurrahman