Re: [PATCH v2 4/5] hwmon: (pmbus/adm1266) register the gpio_chip after pmbus_do_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Out-of-bounds write of 1 byte in adm1266_pmbus_block_xfer() due to I2C transfer length exceeding buffer size.
- [High] Missing PMBus core locking in NVMEM accessors violates hwmon subsystem guidelines.
- [High] Incomplete fix: NVMEM device is exposed before PMBus initialization completes, preserving a probe race condition.
--

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

hwmon: (pmbus/adm1266) register the gpio_chip after pmbus_do_probe()

adm1266_probe() calls adm1266_config_gpio() before pmbus_do_probe() has
initialised the per-client PMBus state. This latent hazard allows a sysfs
read from a user space agent to race pmbus_do_probe()'s own device
accesses. Move adm1266_config_gpio() down past pmbus_do_probe() to avoid
this race.

Fixes: d98dfad35c38 ("hwmon: (pmbus/adm1266) Add support for GPIOs")
Cc: stable@vger.kernel.org

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 57cb7d302cddd..b91dcf067fa6d 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -467,10 +467,6 @@ static int adm1266_probe(struct i2c_client *client)
>  	crc8_populate_msb(pmbus_crc_table, 0x7);
>  	mutex_init(&data->buf_mutex);
>  
> -	ret = adm1266_config_gpio(data);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = adm1266_set_rtc(data);
>  	if (ret < 0)
>  		return ret;

[ ... ]

> @@ -483,6 +479,10 @@ static int adm1266_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	ret = adm1266_config_gpio(data);
> +	if (ret < 0)
> +		return ret;
> +

Does this code leave the nvmem device exposed before PMBus initialization
completes?

While adm1266_config_gpio() is moved down past pmbus_do_probe(),
adm1266_config_nvmem() remains above it:

static int adm1266_probe(struct i2c_client *client)
{
	...
	ret = adm1266_config_nvmem(data);
	if (ret < 0)
		return ret;

	ret = pmbus_do_probe(client, &data->info);
	...
}

This means the nvmem device is immediately exposed to userspace before the
PMBus core is fully initialized. A concurrent nvmem read could trigger
adm1266_nvmem_read() and perform I2C accesses without serialization,
recreating the identical hazard this patch aims to fix for GPIOs.

Additionally, do the nvmem accessors implement the required locking?

The hwmon subsystem guidelines state that drivers must implement locking
for attributes registered outside the core. adm1266_nvmem_read() triggers
I2C transactions independently via adm1266_pmbus_block_xfer() without
taking pmbus_lock(). This could allow concurrent nvmem reads to interleave
with the PMBus core's multi-step sequences, potentially breaking device
state.

Could this lead to a memory corruption issue during I2C transfers?

While not introduced by this patch, it appears the read_buf array in
struct adm1266_data is allocated with 256 bytes and is exactly cacheline
aligned:

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

If adm1266_pmbus_block_xfer() sets the read message length to
ADM1266_PMBUS_BLOCK_MAX + 2 (257 bytes), wouldn't the I2C master controller
clock out and read exactly 257 bytes from the bus, resulting in an
out-of-bounds write of 1 byte past the end of the heap-allocated structure?

>  	adm1266_init_debugfs(data);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-adm1266-gpio-fixes-v2-0-801f13debcb2@nexthop.ai?part=4