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

[sashiko-bot@kernel.org]

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