[PATCH 0/2] hwmon: (pmbus/adm1266) adm1266_gpio_get_multiple() fixes

[PATCH 0/2] hwmon: (pmbus/adm1266) adm1266_gpio_get_multiple() fixes

[Abdurrahman Hussain]

Two pre-existing bugs in adm1266_gpio_get_multiple() that landed
together when GPIO support was first added (commit d98dfad35c38).
Both are reachable any time userspace queries multiple ADM1266 GPIO
or PDIO lines at once via the gpiolib char-dev or sysfs interfaces.

Patch 1 caps the PDIO scan loop 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.  Flagged by sashiko in review of an unrelated
fix series [1].

Patch 2 drops a redundant "*bits = 0" reset that sits between the
GPIO and PDIO halves of the function.  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.

[1] https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai

Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Abdurrahman Hussain (2):
      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

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

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

[PATCH 1/2] 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 1/2] 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:
- [Critical] Stack buffer overflow in `adm1266_nvmem_read_blackbox()` due to an undersized receive buffer.
- [Critical] Heap buffer overflow in `adm1266_nvmem_read_blackbox()` caused by unbounded `record_count`.
- [Critical] Off-by-one heap buffer overflow in `adm1266_pmbus_block_xfer()`.
- [High] Uninitialized kernel stack memory use in `adm1266_gpio_get()` and `adm1266_gpio_get_multiple()`.
- [High] Data race in `adm1266_nvmem_read()` and violation of hwmon locking rules.
--

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

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

This commit fixes an out-of-bounds stack read and write in
adm1266_gpio_get_multiple() by changing the loop upper bound from
ADM1266_PDIO_STATUS to ADM1266_PDIO_NR.

> 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);

Earlier in this function, i2c_smbus_read_block_data() is called. It returns
the number of bytes read on success. If it returns a short read (e.g., 0 or
1 byte), could read_buf[0] and read_buf[1] contain uninitialized stack memory?

Does this leak kernel stack memory to user space via the GPIO interface?

>  
>  	*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);
>  	}

I also noticed a few other potential regressions in this file:

In adm1266_nvmem_read_blackbox(), i2c_smbus_read_block_data() can copy up to
32 bytes (I2C_SMBUS_BLOCK_MAX) into the target buffer. Since buf is only 5
bytes:

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

Could a malfunctioning or malicious I2C device returning a large block length
cause a stack buffer overflow here?


Additionally, record_count is read from the I2C device and can be up to 255.
The target buffer data->dev_mem is allocated with a fixed size of 2048 bytes:

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

If the hardware returns a record_count greater than 32 (32 * 64 = 2048), does
this loop overflow the data->dev_mem heap buffer?


In adm1266_pmbus_block_xfer(), msgs[1].len is set to
ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes):

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

However, data->read_buf is only sized for ADM1266_PMBUS_BLOCK_MAX + 1
(256 bytes):

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

Since i2c_transfer() instructs the controller to read exactly 257 bytes,
does this write past the end of the read_buf array?


Finally, the hwmon subsystem guidelines state that drivers must implement
locking for attributes registered by any other means, and should use
hwmon_lock(). Since NVMEM sysfs reads are not serialized:

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

If multiple userspace readers access offset == 0 concurrently, could they
simultaneously execute memset(data->dev_mem, 0, ...) and interleave I2C
block reads?

Does this lockless concurrent mutation of data->dev_mem cause data corruption?

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

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

Re: [PATCH 2/2] 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 1 potential issue(s) to consider:
- [High] The GPIO attribute callback performs I2C bus operations without acquiring the required hardware monitoring subsystem lock.
--

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

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

This commit removes a redundant clearing of the bitmask in the GPIO
get_multiple callback, ensuring that both GPIO and PDIO bits are preserved
in the result.

> 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

Does this GPIO attribute callback need to acquire the hardware monitoring
subsystem lock?

The hardware monitoring subsystem guidelines require that drivers implement
locking for attributes registered outside the core, to serialize with thermal
and sysfs operations.

Looking at adm1266_gpio_get_multiple(), it issues i2c_smbus_read_block_data()
without acquiring a lock. Could this cause a race condition against concurrent
PMBus accesses from the core?

>  
>  	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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-gpio-fixes-v1-0-38d9dd39b905@nexthop.ai?part=2

Re: [PATCH 0/2] hwmon: (pmbus/adm1266) adm1266_gpio_get_multiple() fixes

[Linus Walleij]

On Sat, May 16, 2026 at 10:45 PM Abdurrahman Hussain
<abdurrahman@nexthop.ai> wrote:

> Two pre-existing bugs in adm1266_gpio_get_multiple() that landed
> together when GPIO support was first added (commit d98dfad35c38).
> Both are reachable any time userspace queries multiple ADM1266 GPIO
> or PDIO lines at once via the gpiolib char-dev or sysfs interfaces.
>
> Patch 1 caps the PDIO scan loop 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.  Flagged by sashiko in review of an unrelated
> fix series [1].
>
> Patch 2 drops a redundant "*bits = 0" reset that sits between the
> GPIO and PDIO halves of the function.  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.
>
> [1] https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai
>
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>

Reviewed-by: Linus Walleij <linusw@kernel.org>

The better option would be to:

1. Convert this driver to use regmap
2. Extend gpio-regmap with get/set_multiple()
3. Convert the driver to use gpio-regmap

So if you feel adventurous and have time on your hands,
consider it! :)

Yours,
Linus Walleij

Re: [PATCH 0/2] hwmon: (pmbus/adm1266) adm1266_gpio_get_multiple() fixes

[Guenter Roeck]

On 5/17/26 03:44, Linus Walleij wrote:
> On Sat, May 16, 2026 at 10:45 PM Abdurrahman Hussain
> <abdurrahman@nexthop.ai> wrote:
> 
>> Two pre-existing bugs in adm1266_gpio_get_multiple() that landed
>> together when GPIO support was first added (commit d98dfad35c38).
>> Both are reachable any time userspace queries multiple ADM1266 GPIO
>> or PDIO lines at once via the gpiolib char-dev or sysfs interfaces.
>>
>> Patch 1 caps the PDIO scan loop 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.  Flagged by sashiko in review of an unrelated
>> fix series [1].
>>
>> Patch 2 drops a redundant "*bits = 0" reset that sits between the
>> GPIO and PDIO halves of the function.  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.
>>
>> [1] https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai
>>
>> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> 
> Reviewed-by: Linus Walleij <linusw@kernel.org>
> 
> The better option would be to:
> 
> 1. Convert this driver to use regmap

That would mean to convert the pmbus core code to regmap,
plus all the pmbus client drivers.

PMBus uses a mix of registers/command with different size, plus some
block commands. That would be a difficult task. Byte registers can be
mapped to word size, but for block registers that is difficult,
and then there are commands with zero data length. Maybe someone
managed to do this somewhere. I tried some time ago and could not get
it to work.

Guenter

> 2. Extend gpio-regmap with get/set_multiple()
> 3. Convert the driver to use gpio-regmap
> 
> So if you feel adventurous and have time on your hands,
> consider it! :)
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH 0/2] hwmon: (pmbus/adm1266) adm1266_gpio_get_multiple() fixes

[Linus Walleij]

On Tue, May 19, 2026 at 12:25 AM Guenter Roeck <linux@roeck-us.net> wrote:

> > 1. Convert this driver to use regmap
>
> That would mean to convert the pmbus core code to regmap,
> plus all the pmbus client drivers.
>
> PMBus uses a mix of registers/command with different size, plus some
> block commands. That would be a difficult task. Byte registers can be
> mapped to word size, but for block registers that is difficult,
> and then there are commands with zero data length. Maybe someone
> managed to do this somewhere. I tried some time ago and could not get
> it to work.

I didn't know PMBus was that complex and honestly thought it
was something simple that had been regmapp:ed ages ago ...
allright, definitely an exercise for another day.

Yours,
Linus Walleij