Re: [PATCH v4 1/3] hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Heap buffer overflow in adm1266_nvmem_read_blackbox() via unvalidated record_count read from I2C.
- [High] Out-of-bounds write in adm1266_pmbus_block_xfer() due to blindly trusting I2C block read length.
- [High] Out-of-bounds write in adm1266_pmbus_block_xfer() caused by mismatch between buffer capacity and I2C transfer length.
--

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

hwmon: (pmbus/adm1266) add clear_blackbox debugfs entry

This commit introduces a clear_blackbox debugfs entry for the ADM1266
device to allow clearing the blackbox recording buffer. It issues a
2-byte block-write using the READ_BLACKBOX command under the pmbus_lock.
The commit also acquires the pmbus_lock in adm1266_nvmem_read() to
serialize blackbox reads and clear operations.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index d90f8f80be8e0..9f4709bc85afe 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c

[ ... ]

> @@ -355,7 +390,6 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  	ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);
>  	if (ret < 0)
>  		return ret;
> -
>  	if (ret != 4)
>  		return -EIO;
>  

This regression wasn't introduced by this patch, but pre-dates it. Can the
unvalidated record_count read from I2C cause a heap buffer overflow?

The function reads ADM1266_BLACKBOX_INFO into buf and uses buf[3] as
record_count:

	record_count = buf[3];

It then loops record_count times, appending ADM1266_BLACKBOX_SIZE
(64 bytes) to read_buff on each iteration. Since read_buff points to
data->dev_mem, which is allocated to 2048 bytes (32 records), could
an I2C device returning a record_count greater than 32 cause an
out-of-bounds write into adjacent heap memory?

> @@ -365,7 +399,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);
>  		if (ret < 0)
>  			return ret;
> -
>  		if (ret != ADM1266_BLACKBOX_SIZE)
>  			return -EIO;

There is another regression that wasn't introduced by this patch, but
pre-dates it. Does blindly trusting the I2C block read length in
adm1266_pmbus_block_xfer() lead to an out-of-bounds write?

In adm1266_pmbus_block_xfer(), the first byte of the I2C response is used
as the transfer length for memcpy():

	memcpy(data_r, &msgs[1].buf[1], msgs[1].buf[0]);

The caller adm1266_nvmem_read_blackbox() anticipates 64 bytes per iteration
and offsets the data_r pointer by 64 bytes per loop. If the I2C device
returns a block length greater than 64, could the memcpy overwrite
subsequent records in the buffer and write past the end of the 2048-byte
allocation?

Additionally, another regression pre-dates this patch. Does a mismatch
between the buffer capacity and I2C transfer length cause an
out-of-bounds write?

The data->read_buf array is declared with a size of
ADM1266_PMBUS_BLOCK_MAX + 1 (256 bytes):

	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;

However, when preparing the I2C read message in adm1266_pmbus_block_xfer(),
msgs[1].len is set to ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes):

		{
			.addr = client->addr,
			.flags = I2C_M_RD | I2C_M_DMA_SAFE,
			.buf = data->read_buf,
			.len = ADM1266_PMBUS_BLOCK_MAX + 2,
		}

Because the I2C_M_RECV_LEN flag is absent, could the I2C controller
unconditionally attempt to read 257 bytes into the 256-byte read_buf,
writing past the end of struct adm1266_data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-v4-0-1f8df4797258@nexthop.ai?part=1