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

[sashiko-bot@kernel.org]

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