[PATCH] hwmon: Fix potential UAF in pec_store

[PATCH] hwmon: Fix potential UAF in pec_store

[Guenter Roeck]

Sashiko reports:

In pec_store(), a guard(mutex)(&hwdev->lock) is taken. If the chip write
operation returns an error other than -EOPNOTSUPP, the code jumps to the
put label, which calls put_device(hdev). If this drops the final reference,
the device is freed. When the function then returns, the guard cleanup
function runs and attempts to unlock the freed mutex.

Use scoped_guard() instead of guard() to avoid the problem.

Fixes: 3ad2a7b9b15d5 ("hwmon: Serialize accesses in hwmon core")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 41755910a25a..3e65fc6d25eb 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -371,18 +371,17 @@ static ssize_t pec_store(struct device *dev, const struct device_attribute *deva
 	 * handling is not required.
 	 */
 	hwdev = to_hwmon_device(hdev);
-	guard(mutex)(&hwdev->lock);
-	if (hwdev->chip->ops->write) {
-		err = hwdev->chip->ops->write(hdev, hwmon_chip, hwmon_chip_pec, 0, val);
-		if (err && err != -EOPNOTSUPP)
-			goto put;
+	scoped_guard(mutex, &hwdev->lock) {
+		if (hwdev->chip->ops->write) {
+			err = hwdev->chip->ops->write(hdev, hwmon_chip, hwmon_chip_pec, 0, val);
+			if (err && err != -EOPNOTSUPP)
+				goto put;
+		}
+		if (!val)
+			client->flags &= ~I2C_CLIENT_PEC;
+		else
+			client->flags |= I2C_CLIENT_PEC;
 	}
-
-	if (!val)
-		client->flags &= ~I2C_CLIENT_PEC;
-	else
-		client->flags |= I2C_CLIENT_PEC;
-
 	err = count;
 put:
 	put_device(hdev);
-- 
2.45.2

Re: [PATCH] hwmon: Fix potential UAF in pec_store

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The function `pec_store()` mixes `goto`-based error handling (`goto put;`) with scope-based cleanup (`scoped_guard`), which violates subsystem guidelines.
--

--- Patch [1]: [PATCH] hwmon: Fix potential UAF in pec_store ---
commit faaeb6b5ca43335968fb5512ec894ab84e5edc9c
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: Fix potential UAF in pec_store

This patch fixes a potential use-after-free in pec_store() by replacing
guard() with scoped_guard(). The previous guard() usage attempted to unlock
the mutex after dropping the final device reference when jumping to the 'put'
label.

> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 29dc90a2c3fe7..8cf717ae8b1dd 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -371,18 +371,17 @@ static ssize_t pec_store(struct device *dev, struct device_attribute *devattr,
>  	 * handling is not required.
>  	 */
>  	hwdev = to_hwmon_device(hdev);
> -	guard(mutex)(&hwdev->lock);
> -	if (hwdev->chip->ops->write) {
> -		err = hwdev->chip->ops->write(hdev, hwmon_chip, hwmon_chip_pec, 0, val);
> -		if (err && err != -EOPNOTSUPP)
> -			goto put;
> +	scoped_guard(mutex, &hwdev->lock) {
> +		if (hwdev->chip->ops->write) {
> +			err = hwdev->chip->ops->write(hdev, hwmon_chip, hwmon_chip_pec, 0, val);
> +			if (err && err != -EOPNOTSUPP)
> +				goto put;

[Severity: Medium]
This is a pre-existing issue, but does this code mix goto-based error
handling with scope-based cleanup helpers?

According to the subsystem guidelines in include/linux/cleanup.h, mixing goto
and cleanup helpers in the same function creates confusing ownership semantics
and should be avoided. Would it be better to also convert the 'hdev' reference
management to use __free(put_device) to fully eliminate the goto pattern?

> +		}
> +		if (!val)
> +			client->flags &= ~I2C_CLIENT_PEC;
> +		else
> +			client->flags |= I2C_CLIENT_PEC;
>  	}
> -
> -	if (!val)
> -		client->flags &= ~I2C_CLIENT_PEC;
> -	else
> -		client->flags |= I2C_CLIENT_PEC;
> -
>  	err = count;
>  put:
>  	put_device(hdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821050808.667204-1-linux@roeck-us.net?part=1