Re: [PATCH 5/7] hwmon: (pmbus/adm1266) add powerup_counter debugfs entry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Use-after-free (UAF) during device removal due to conflicting devm and I2C core debugfs cleanup.
- [High] The debugfs attribute read function does not use hwmon_lock() to protect the hardware transaction.
--

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

hwmon: (pmbus/adm1266) add powerup_counter debugfs entry

The ADM1266 maintains a 16-bit non-volatile POWERUP_COUNTER register
that increments on every power cycle and cannot be reset by the host.
This patch exposes it as a read-only debugfs file.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> +static int adm1266_powerup_counter_read(struct seq_file *s, void *pdata)
> +{
> +	struct device *dev = s->private;
> +	struct i2c_client *client = to_i2c_client(dev);
> +	u8 buf[I2C_SMBUS_BLOCK_MAX];
> +	int ret;
> +
> +	ret = i2c_smbus_read_block_data(client, ADM1266_POWERUP_COUNTER, buf);

The hwmon subsystem guidelines mention that drivers should use hwmon_lock()
and hwmon_unlock() for attributes registered outside of the core hwmon sysfs
operations.

Since this debugfs read issues an I2C transaction, should hwmon_lock() be
acquired before calling i2c_smbus_read_block_data() to avoid racing with
concurrent sysfs or internal device operations?

> +	if (ret < 0)
> +		return ret;
> +	if (ret != 2)
> +		return -EIO;
> +
> +	seq_printf(s, "%u\n", buf[0] | (buf[1] << 8));
> +
> +	return 0;
> +}
[ ... ]
> @@ -400,6 +426,8 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
>  				    adm1266_state_read);
>  	debugfs_create_devm_seqfile(&data->client->dev, "firmware_revision", data->debugfs_dir,
>  				    adm1266_firmware_revision_read);
> +	debugfs_create_devm_seqfile(&data->client->dev, "powerup_counter", data->debugfs_dir,
> +				    adm1266_powerup_counter_read);

Can this sequence lead to a use-after-free during device removal?

According to the I2C subsystem rules, the I2C core cleans up debugfs entries
attached to the client after the driver's remove function finishes, but
before releasing devres resources.

If the I2C core removes the debugfs tree and frees the dentries first,
wouldn't the subsequent devm cleanup for this seqfile try to call
debugfs_remove() using an already-freed dentry pointer?

>  	debugfs_create_file("clear_blackbox", 0200, data->debugfs_dir, data->client,
>  			    &adm1266_clear_blackbox_fops);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-adm1266-v1-0-ec08bf29e0ce@nexthop.ai?part=5