Re: [PATCH v3 7/8] hwmon: (pmbus/adm1266) serialize NVMEM blackbox read with pmbus_lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing bounds check on hardware-provided `record_count` causes massive out-of-bounds write and potential infinite loop.
- [Critical] Unvalidated hardware-controlled length byte causes out-of-bounds memcpy.
- [High] 1-byte Out-of-Bounds write in I2C DMA buffer and out-of-bounds PEC array access.
--

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

hwmon: (pmbus/adm1266) serialize NVMEM blackbox read with pmbus_lock

adm1266_nvmem_read() is the reg_read callback the NVMEM core invokes
when userspace reads /sys/bus/nvmem/devices/.../nvmem on this chip.
[ ... ]

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index a80fb2ea73bdb..051f4f188ec52 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -394,6 +394,8 @@ static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t
>  	if (offset + bytes > data->nvmem_config.size)
>  		return -EINVAL;
>  
> +	guard(pmbus_lock)(data->client);
> +
>  	if (offset == 0) {
>  		memset(data->dev_mem, 0, data->nvmem_config.size);
>  
> 		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);

This isn't a bug introduced by this patch, but does this missing bounds check
on the hardware-provided record_count cause an out-of-bounds write?

In adm1266_nvmem_read_blackbox(), record_count is read directly from the
device and can be up to 255:

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read_blackbox() {
    ...
    ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);
    ...
    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;
    }
    ...
}

Since read_buff points to data->dev_mem, which is fixed at 2048 bytes, a
record_count > 32 will write out of bounds since each block is 64 bytes
(ADM1266_BLACKBOX_SIZE).

Additionally, if the architecture has a signed char, could a record_count > 127
cause index to wrap around to -128, creating an infinite loop?


This isn't a bug introduced by this patch, but could this memcpy() overflow
the buffer?

In adm1266_pmbus_block_xfer(), the length of the read block is determined by
msgs[1].buf[0], which can be up to 255:

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

When called from adm1266_nvmem_read_blackbox(), data_r points to a position
in data->dev_mem expecting only 64 bytes. If the device returns a length > 64,
this will corrupt memory before the safeguard check in the caller is reached.


This isn't a bug introduced by this patch, but could this I2C message length
cause an out-of-bounds write?

In adm1266_data, read_buf is 256 bytes (ADM1266_PMBUS_BLOCK_MAX + 1):

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

However, adm1266_pmbus_block_xfer() configures the read length to 257 bytes
(ADM1266_PMBUS_BLOCK_MAX + 2):

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

This allows the I2C controller to write up to 257 bytes into the 256-byte
array.

Furthermore, if PEC is enabled and the device returns a block length of 255
(msgs[1].buf[0] == 255), does the PEC validation access index 256, which is
past the end of the array?

drivers/hwmon/pmbus/adm1266.c:adm1266_pmbus_block_xfer() {
    ...
        crc = crc8(pmbus_crc_table, msgs[1].buf,  msgs[1].buf[0] + 1, crc);

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

> 		if (ret) {
> 			dev_err(&data->client->dev, "Could not read blackbox!");
> 			return ret;
> 		}

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