Re: [PATCH 1/2] 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 5 potential issue(s) to consider:
- [Critical] Stack buffer overflow in `adm1266_nvmem_read_blackbox()` due to an undersized receive buffer.
- [Critical] Heap buffer overflow in `adm1266_nvmem_read_blackbox()` caused by unbounded `record_count`.
- [Critical] Off-by-one heap buffer overflow in `adm1266_pmbus_block_xfer()`.
- [High] Uninitialized kernel stack memory use in `adm1266_gpio_get()` and `adm1266_gpio_get_multiple()`.
- [High] Data race in `adm1266_nvmem_read()` and violation of hwmon locking rules.
--

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

hwmon: (pmbus/adm1266) cap PDIO scan in get_multiple at ADM1266_PDIO_NR

This commit fixes an out-of-bounds stack read and write in
adm1266_gpio_get_multiple() by changing the loop upper bound from
ADM1266_PDIO_STATUS to ADM1266_PDIO_NR.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index d90f8f80be8e0..11f9a44f4361a 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -211,7 +211,7 @@ static int adm1266_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask
>  	status = read_buf[0] + (read_buf[1] << 8);

Earlier in this function, i2c_smbus_read_block_data() is called. It returns
the number of bytes read on success. If it returns a short read (e.g., 0 or
1 byte), could read_buf[0] and read_buf[1] contain uninitialized stack memory?

Does this leak kernel stack memory to user space via the GPIO interface?

>  
>  	*bits = 0;
> -	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
> +	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_NR) {
>  		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
>  			set_bit(gpio_nr, bits);
>  	}

I also noticed a few other potential regressions in this file:

In adm1266_nvmem_read_blackbox(), i2c_smbus_read_block_data() can copy up to
32 bytes (I2C_SMBUS_BLOCK_MAX) into the target buffer. Since buf is only 5
bytes:

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

Could a malfunctioning or malicious I2C device returning a large block length
cause a stack buffer overflow here?


Additionally, record_count is read from the I2C device and can be up to 255.
The target buffer data->dev_mem is allocated with a fixed size of 2048 bytes:

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

If the hardware returns a record_count greater than 32 (32 * 64 = 2048), does
this loop overflow the data->dev_mem heap buffer?


In adm1266_pmbus_block_xfer(), msgs[1].len is set to
ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes):

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

However, data->read_buf is only sized for ADM1266_PMBUS_BLOCK_MAX + 1
(256 bytes):

drivers/hwmon/pmbus/adm1266.c {
    ...
	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
    ...
}

Since i2c_transfer() instructs the controller to read exactly 257 bytes,
does this write past the end of the read_buf array?


Finally, the hwmon subsystem guidelines state that drivers must implement
locking for attributes registered by any other means, and should use
hwmon_lock(). Since NVMEM sysfs reads are not serialized:

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read() {
    ...
	if (offset == 0) {
		memset(data->dev_mem, 0, data->nvmem_config.size);

		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);
    ...
}

If multiple userspace readers access offset == 0 concurrently, could they
simultaneously execute memset(data->dev_mem, 0, ...) and interleave I2C
block reads?

Does this lockless concurrent mutation of data->dev_mem cause data corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-gpio-fixes-v1-0-38d9dd39b905@nexthop.ai?part=1