Re: [PATCH 5/5] hwmon: (pmbus/adm1266) bounce blackbox records through a protocol-sized buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] This problem wasn't introduced by this patch, but the NVMEM read callback `adm1266_nvmem_read` lacks required locking, violating hwmon subsystem rules and causing a data race on `data->dev_mem`.
--

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

hwmon: (pmbus/adm1266) bounce blackbox records through a protocol-sized buffer

This commit modifies adm1266_nvmem_read_blackbox() to copy records into a
local bounce buffer before writing them to the destination. This prevents
an overflow since the device can return up to 255 bytes while the helper
previously wrote directly into 64-byte strides of the buffer.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 5c68e3177f64e..ea1edb89d2a0c 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -348,6 +348,7 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
>  
>  static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  {
> +	u8 record[ADM1266_PMBUS_BLOCK_MAX];
>  	int record_count;
>  	char index;
>  	u8 buf[I2C_SMBUS_BLOCK_MAX];
> @@ -365,13 +366,14 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  		return -EIO;
>  
>  	for (index = 0; index < record_count; index++) {
> -		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
> +		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, record);
>  		if (ret < 0)
>  			return ret;
>  
>  		if (ret != ADM1266_BLACKBOX_SIZE)
>  			return -EIO;
>  
> +		memcpy(read_buff, record, ADM1266_BLACKBOX_SIZE);

This isn't a bug introduced by this patch, but does the NVMEM read callback
adm1266_nvmem_read() lack required locking around data->dev_mem?

The hwmon subsystem guidelines state that drivers must implement locking
for attributes registered outside the hwmon core, such as using hwmon_lock()
and hwmon_unlock().

Since the NVMEM core does not serialize reads, could concurrent user space
reads race in adm1266_nvmem_read():

static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t bytes)
{
	...
	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 Thread A reads at offset == 0, it zeroes data->dev_mem and repopulates
it via adm1266_nvmem_read_blackbox().

If Thread B concurrently reads at a non-zero offset, it might execute the
memcpy() and return torn, zeroed, or corrupted data to user space.

Additionally, if both threads read at offset == 0, they could simultaneously
clear and write to the same buffer, destroying the integrity of the cached
blackbox data.

>  		read_buff += ADM1266_BLACKBOX_SIZE;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai?part=5