Re: [PATCH 4/5] hwmon: (pmbus/adm1266) include PEC byte in pmbus_block_xfer read buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Out-of-bounds memory accesses in `adm1266_gpio_get_multiple` due to incorrect macro usage.
- [High] Data race and subsystem guideline violation in `adm1266_nvmem_read` due to lack of locking.
--

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

hwmon: (pmbus/adm1266) include PEC byte in pmbus_block_xfer read buffer

This commit increases the size of the read buffer in the adm1266 driver by
one byte. This allows it to hold the length byte, up to 255 payload bytes,
and the PEC byte, preventing an out-of-bounds write and a subsequent
out-of-bounds read during PEC comparison.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 43d9e74077954..5c68e3177f64e 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -61,7 +61,7 @@ struct adm1266_data {
>  	u8 *dev_mem;
>  	struct mutex buf_mutex;
>  	u8 write_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
> -	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
> +	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 2] ____cacheline_aligned;
>  };
>  
>  static const struct nvmem_cell_info adm1266_nvmem_cells[] = {

While reviewing this code, I noticed a couple of other issues in the file.
This isn't a bug introduced by this patch, but does the loop in
adm1266_gpio_get_multiple() read past the end of the mask array?

drivers/hwmon/pmbus/adm1266.c:adm1266_gpio_get_multiple() {
...
	*bits = 0;
	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
			set_bit(gpio_nr, bits);
	}
...
}

Because ADM1266_PDIO_STATUS is a PMBus command constant (0xE9 or 233), the
size evaluated is 242 instead of the expected ADM1266_GPIO_NR +
ADM1266_PDIO_NR (25).

Since the mask and bits arrays supplied by gpiolib are sized only for ngpio
(25 bits), find_next_bit() will read up to 242 bits from mask.

If any of that memory has a bit set, could the loop execute
set_bit(gpio_nr, bits) and overflow the bits array?

Additionally, this isn't a bug introduced by this patch, but is there a
missing lock in adm1266_nvmem_read() that could cause a data race on
data->dev_mem?

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

The hwmon subsystem guidelines require drivers to implement locking for
attributes registered by other means, such as the NVMEM core. However,
adm1266_nvmem_read() accesses and mutates the shared data->dev_mem buffer
without acquiring any driver-level lock.

When called with offset 0, it zeroes out the memory via memset() and then
repopulates it via multiple slow I2C reads.

Because the NVMEM subsystem does not serialize reg_read operations, will
concurrent reads via the sysfs NVMEM interface result in multiple threads
interleaving their memset operations and I2C block transactions, returning
corrupted or partially-populated data to userspace?

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