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

["Abdurrahman Hussain" <abdurrahman@nexthop.ai>]

On Wed May 13, 2026 at 5:47 PM PDT, sashiko-bot wrote:
> 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?
>

Yes, this is a real race -- the nvmem core (nvmem_reg_read() in
drivers/nvmem/core.c) does not serialize reg_read callbacks, so two
concurrent reads of the nvmem device can interleave the memset and
memcpy on data->dev_mem.  For v4 I'll lift the guard from
adm1266_nvmem_read_blackbox() up into adm1266_nvmem_read() so it
covers the memset, the blackbox refill, and the memcpy as a single
critical section.

>>  	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?
>

Yes -- a malformed length byte from the device would overflow the
5-byte buf onto the stack.  This is pre-existing in the original
driver; I'll send a separate fix patch with a Fixes: tag sizing buf
at I2C_SMBUS_BLOCK_MAX, and rebase the feature series on top.

>>  	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?
>

Yes; record_count comes straight from the device.  Pre-existing as
well -- same fix path.  v4 will clamp against the declared blackbox
size (32 records) before entering the loop and return -EIO if the
device reports more.

> 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?
>

Yes.  adm1266_pmbus_block_xfer() memcpy()s up to 255 bytes into a
caller-supplied buffer whose size the function has no knowledge of.
Pre-existing; same fix path.  The fix will add a max-length
parameter to the helper and update the three call sites to pass
their actual buffer size.

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

Yes -- read_buf needs ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes) to hold
the length byte plus up to 255 payload bytes plus the PEC byte.  The
subsequent PEC compare,

  if (crc != msgs[1].buf[msgs[1].buf[0] + 1])

also reads past the end of the current array for a max-length
response.  Pre-existing; same fix path.

In summary, the four pre-existing buffer-bound bugs (points 2-5
above) will go into a separate fix series with Fixes: tags sent
first; the nvmem-read race (point 1) is folded into this patch in
v4.

Thanks,
Abdurrahman