Re: [PATCH v3 2/5] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The shared buffer `data->dev_mem` is modified and accessed in `adm1266_nvmem_read()` outside of any lock, leading to data corruption on concurrent nvmem reads.
- [Critical] Stack buffer overflow in `adm1266_nvmem_read_blackbox()` caused by an undersized buffer passed to `i2c_smbus_read_block_data()`.
- [Critical] Heap buffer overflow in `adm1266_nvmem_read_blackbox()` due to blindly trusting the hardware-provided `record_count`.
- [Critical] Out-of-bounds write in `adm1266_pmbus_block_xfer()` due to unsafe `memcpy` using an untrusted length from the I2C device.
- [High] DMA read buffer bounds mismatch in `adm1266_pmbus_block_xfer()` exposes an out-of-bounds write vulnerability.
--

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

hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

This commit adds a write-only debugfs file to clear the ADM1266 blackbox
buffer. Writing to the file issues a 2-byte block-write command to the
device. It also adds a pmbus_lock guard to adm1266_nvmem_read_blackbox()
to prevent concurrent read/write operations from interleaving.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 7b8433226176f..bd3784d59a7c8 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> @@ -379,10 +415,11 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  	u8 buf[5];
>  	int ret;
>  
> +	guard(pmbus_lock)(data->client);
> +

While this patch adds a guard here, does adm1266_nvmem_read() also modify
and access data->dev_mem without taking this lock?

If two threads read the nvmem device concurrently, could one thread's
memset() execute during another thread's memcpy(), causing corrupted data
to be copied to userspace?

>  	ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);

Can this call overflow the buf array?

Looking at i2c_smbus_read_block_data(), it blindly copies up to
I2C_SMBUS_BLOCK_MAX (32) bytes into the provided buffer based on the
length byte returned by the hardware:

drivers/i2c/i2c-core-smbus.c:i2c_smbus_read_block_data() {
    ...
	status = i2c_smbus_xfer(...);
	if (status)
		return status;

	memcpy(values, &data.block[1], data.block[0]);
	return data.block[0];
}

If the I2C device returns a block length greater than 5, would this
memcpy() overflow the 5-byte buf array?

>  	if (ret < 0)
>  		return ret;
> -
>  	if (ret != 4)
>  		return -EIO;
>  
> @@ -392,7 +429,6 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);

Does this loop have a bounds checking issue?

The record_count loop limit is read directly from buf[3]. If the device
returns a record_count greater than 32, will the loop advance read_buff
past the end of the 2048-byte allocation for data->dev_mem?

Also, could this call to adm1266_pmbus_block_xfer() cause an
out-of-bounds write?

In adm1266_pmbus_block_xfer(), the memcpy() uses a length from the I2C
device:

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
	memcpy(data_r, &msgs[1].buf[1], msgs[1].buf[0]);
    ...
}

If the device returns a length up to 255, could this write past the
expected 64 bytes (ADM1266_BLACKBOX_SIZE) allocated for read_buff?

Additionally, does the DMA read buffer size in adm1266_pmbus_block_xfer()
expose an overflow regression?

The read_buf array in struct adm1266_data is sized
ADM1266_PMBUS_BLOCK_MAX + 1 (256 bytes). However, the i2c_msg length is
set to 257:

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

If the hardware sends a full 257-byte response, will the controller
write 1 byte past the end of data->read_buf?

>  		if (ret < 0)
>  			return ret;
> -
>  		if (ret != ADM1266_BLACKBOX_SIZE)
>  			return -EIO;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-adm1266-v3-0-a81a479b0bb0@nexthop.ai?part=2