Re: [PATCH v3 3/8] 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] Stack buffer overflow in adm1266_nvmem_read_blackbox() caused by untrusted device response.
- [Critical] Heap buffer overflow in adm1266_nvmem_read_blackbox() due to missing bounds check on record_count.
- [High] Mismatched buffer sizes in adm1266_pmbus_block_xfer() cause an out-of-bounds heap read and write.
--

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

hwmon: (pmbus/adm1266) reject short block-read responses in the GPIO accessors

This commit adds length checks to adm1266_gpio_get() and
adm1266_gpio_get_multiple() after calling i2c_smbus_read_block_data(). This
prevents uninitialized stack memory from being returned to userspace via
gpiolib when a device returns a short block-read response.

> 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

[ ... ]

This isn't a bug introduced by this patch, but while addressing short
block-read responses, there appears to be a related stack buffer overflow
caused by untrusted device responses:

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

The SMBus block read function can copy up to 32 bytes into the buffer before
returning the actual length. If a malfunctioning I2C device returns a block
length greater than 5, could this overflow the 5-byte stack buffer buf?

This isn't a bug introduced by this patch, but there might also be a heap
buffer overflow due to a missing bounds check on record_count:

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

The record_count is read directly from buf[3] without validation and used as
the loop upper limit to iteratively read 64-byte records into read_buff.

Since read_buff points to data->dev_mem, which is allocated with a fixed size
of 2048 bytes (allowing a maximum of 32 records), what happens if a device
returns a record_count larger than 32? Could the loop overflow the heap
buffer by writing up to 16,320 bytes?

This isn't a bug introduced by this patch, but there is a mismatch in buffer
sizes in adm1266_pmbus_block_xfer():

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() {
    ...
		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		},
    ...
	return i2c_transfer(client->adapter, msgs, 2);
}

The array data->read_buf is 256 bytes, but the read message defines its length
as 257 bytes. Will i2c_transfer() read up to 257 bytes into the 256-byte
buffer and cause a heap out-of-bounds write?

Additionally, if the PEC flag is enabled and the device returns a maximum
length of 255:

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
	if (client->flags & I2C_CLIENT_PEC) {
		if (i2c_smbus_msg_pec(0, &msgs[0], &msgs[1]) !=
		    msgs[1].buf[msgs[1].buf[0] + 1])
			return -EBADMSG;
    ...
}

Would the CRC validation access msgs[1].buf[256], performing an out-of-bounds
read past the end of the 256-byte array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-adm1266-gpio-fixes-v3-0-e425e4f88139@nexthop.ai?part=3