Re: [PATCH v6 2/2] hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] Denial of Service due to holding hardware lock while copying from user space
- [High] Use-After-Free of `psu` context due to incorrect devres initialization order
- [High] Hardware write protection is permanently disabled upon debugfs write
- [Medium] Guideline Violation: Use of incorrect lock for debugfs attributes
--

commit d199d1aceab7c176fa10e4adf36411621ba1481b
Author: Jack Cheng <cheng.jackhy@inventec.com>

hwmon: (pmbus/q54sj108a2) Add support for q50sn12072 and q54sn120a1

This commit adds support for the Q50SN12072 and Q54SN120A1 power modules
to the q54sj108a2 driver. It introduces new driver information structures,
device match tables, and debugfs entries for the supported hardware.

> diff --git a/drivers/hwmon/pmbus/q54sj108a2.c b/drivers/hwmon/pmbus/q54sj108a2.c
> index d5d60a9af8c5..b59cfee861f2 100644
> --- a/drivers/hwmon/pmbus/q54sj108a2.c
> +++ b/drivers/hwmon/pmbus/q54sj108a2.c

[ ... ]

> @@ -83,73 +113,77 @@ static ssize_t q54sj108a2_debugfs_read(struct file *file, char __user *buf,
>  	char *out = data;
>  	char *res;
>  
> +	rc = pmbus_lock_interruptible(psu->client);
> +	if (rc)
> +		return rc;

The hardware monitoring subsystem guidelines state that drivers must implement
locking for attributes registered by other means (such as debugfs) using
hwmon_lock() and hwmon_unlock().

Is pmbus_lock_interruptible() the correct lock to use here, or should this be
using the standard hwmon subsystem locks?

[ ... ]

> @@ -184,25 +224,29 @@ static ssize_t q54sj108a2_debugfs_write(struct file *file, const char __user *bu
>  	int idx = *idxp;
>  	struct q54sj108a2_data *psu = to_psu(idxp, idx);
>  
> -	rc = i2c_smbus_write_byte_data(psu->client, PMBUS_WRITE_PROTECT, 0);
> +	rc = pmbus_lock_interruptible(psu->client);
>  	if (rc)
>  		return rc;
>  
> +	rc = i2c_smbus_write_byte_data(psu->client, PMBUS_WRITE_PROTECT, 0);
> +	if (rc)
> +		goto unlock;

This isn't introduced by this patch, but this code unconditionally disables
hardware write protection.

Does the driver ever restore the PMBUS_WRITE_PROTECT register to its
original state? It looks like the power supply module might be left
permanently unprotected against unintended I2C writes.

>  	switch (idx) {
>  	case Q54SJ108A2_DEBUGFS_OPERATION:
>  		rc = kstrtou8_from_user(buf, count, 0, &dst_data);
>  		if (rc < 0)
> -			return rc;
> +			goto unlock;

Could calling kstrtou8_from_user() while holding pmbus_lock_interruptible()
introduce a denial of service risk?

If a user provides a buffer managed by userfaultfd, they might be able to
indefinitely pause the page fault. Because the PMBus mutex is held, other
tasks attempting to acquire the lock for this device could hang.

Can the user input be copied and parsed into local variables before acquiring
the hardware lock?

[ ... ]

> @@ -341,6 +395,7 @@ static int q54sj108a2_probe(struct i2c_client *client)
>  	if (!psu)
>  		return 0;
>  
> +	psu->chip = chip_id;
>  	psu->client = client;

This isn't a new issue, but there appears to be an existing use-after-free
risk during device unbind.

Looking earlier in q54sj108a2_probe(), pmbus_do_probe() is called before
allocating the psu context:

	ret = pmbus_do_probe(client, &q54sj108a2_info[chip_id]);
	if (ret)
		return ret;

	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);

Since devres actions execute in reverse order of addition, psu will be
freed before the PMBus core cleans up the debugfs directory.

Does this leave a window during removal where debugfs files are accessible,
but any read or write will dereference the freed psu pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260427-add-support-for-q50sn12072-and-q54sn120a1-v6-0-ffa77bfa65e7@inventec.com?part=2