Re: [PATCH v2 5/5] hwmon: (pmbus/adm1266) serialize GPIO PMBus accesses with pmbus_lock

[sashiko-bot@kernel.org]

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