Re: [PATCH v2 3/5] hwmon: (pmbus/adm1266) reject short block-read responses in the GPIO accessors

[sashiko-bot@kernel.org]

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