Re: [PATCH v3 5/8] hwmon: (pmbus/adm1266) register the nvmem device after pmbus_do_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unbounded loop in adm1266_nvmem_read_blackbox allows a heap buffer overflow triggered by an untrusted I2C device response.
- [High] Length validation in adm1266_nvmem_read_blackbox occurs after memcpy, leading to heap corruption.
- [High] 1-byte static out-of-bounds read/write on the heap during every adm1266_pmbus_block_xfer I2C transaction.
- [High] NVMEM reads at non-zero offsets skip device reads and return uninitialized or stale data.
--

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

hwmon: (pmbus/adm1266) register the nvmem device after pmbus_do_probe()

adm1266_probe() calls adm1266_config_nvmem() before pmbus_do_probe() has
initialised the per-client PMBus state. This moves adm1266_config_nvmem()
down past pmbus_do_probe() so the nvmem device isn't reachable from userspace
until the PMBus state is fully initialised.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index b91dcf067fa6d..8b9fbb99a4bda 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -471,14 +471,14 @@ static int adm1266_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = adm1266_config_nvmem(data);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = pmbus_do_probe(client, &data->info);
>  	if (ret)
>  		return ret;
>  
> +	ret = adm1266_config_nvmem(data);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = adm1266_config_gpio(data);
>  	if (ret < 0)
>  		return ret;

While reviewing this patch, I noticed some pre-existing issues in the NVMEM
handling and I2C transfers.

Could this loop in adm1266_nvmem_read_blackbox() cause a heap buffer overflow?

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);
		if (ret < 0)
			return ret;

		if (ret != ADM1266_BLACKBOX_SIZE)
			return -EIO;

		read_buff += ADM1266_BLACKBOX_SIZE;
	}
    ...
}

The function reads record_count directly from the device response, which can be
up to 255.

It then loops record_count times, appending 64 bytes into data->dev_mem.

Since data->dev_mem is allocated with a fixed size of 2048 bytes, could a
record_count greater than 32 linearly write past the end of the buffer and
corrupt kernel heap memory?

Additionally, does the length validation happen too late?

Inside the loop, adm1266_pmbus_block_xfer() copies up to 255 bytes into the
current position of read_buff.

The length check (ret != ADM1266_BLACKBOX_SIZE) is performed after the copy.

If read_buff is near the end of the 2048-byte buffer, could copying up to 255
bytes silently overflow the heap before the error is caught?

There also appears to be a 1-byte out-of-bounds access in
adm1266_pmbus_block_xfer():

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,
		}
	};
    ...
		if (crc != msgs[1].buf[msgs[1].buf[0] + 1]) {
			mutex_unlock(&data->buf_mutex);
			return -EBADMSG;
		}
    ...
}

The read_buf in struct adm1266_data is allocated with a size of 256 bytes
(ADM1266_PMBUS_BLOCK_MAX + 1).

However, the i2c_msg requests a length of 257 bytes. Doesn't this instruct
the I2C master to write up to 257 bytes into the 256-byte buffer,
overflowing it by 1 byte?

Also, if the device returns a length of 255, the PEC validation logic
accesses msgs[1].buf[msgs[1].buf[0] + 1], which reads index 256. Does this
result in an out-of-bounds read?

Finally, could adm1266_nvmem_read() return uninitialized or stale data?

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

	memcpy(val, data->dev_mem + offset, bytes);

	return 0;
}

If a user performs a read starting at an offset greater than 0, the device
read is skipped entirely.

Since data->dev_mem is allocated with devm_kzalloc, doesn't this return pure
zeros if no prior read at offset 0 has occurred, breaking standard
random-access file semantics?

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