[PATCH] hwmon: (occ) unregister hwmon device outside occ lock

[PATCH] hwmon: (occ) unregister hwmon device outside occ lock

[Runyu Xiao]

occ_active(false) and occ_shutdown() call hwmon_device_unregister() while
occ->lock is held.  hwmon_device_unregister() waits for sysfs callbacks to
drain, and those callbacks can enter the OCC update path and try to take
occ->lock again.  That gives the unregister paths the lock ordering
occ->lock -> sysfs callback drain, while a callback has the opposite edge
sysfs callback -> occ->lock.

This issue was found by our static analysis tool and then manually
reviewed against the current tree.

The grounded PoC kept the real unregister and callback carrier:

  occ_shutdown()
  hwmon_device_unregister()
  occ_show_temp_1()
  occ_update_response()

Lockdep reported the circular dependency with occ_shutdown() already
holding the OCC mutex and hwmon_device_unregister() waiting on the sysfs
side:

  WARNING: possible circular locking dependency detected
  ... (sysfs_lock) ... at: hwmon_device_unregister+0x12/0x30 [vuln_msv]
  ... (&test_occ.lock) ... at: occ_shutdown.constprop.0+0xe/0x40 [vuln_msv]
  occ_update_response.isra.0+0xb/0x20 [vuln_msv]
  occ_show_temp_1.constprop.0.isra.0+0x23/0x40 [vuln_msv]
  *** DEADLOCK ***

Serialize hwmon registration and removal with a separate hwmon_lock.
Under that lock, detach occ->hwmon and update occ->active while occ->lock
is held so concurrent OCC state changes still see a stable state, then
drop occ->lock before calling hwmon_device_unregister().  This keeps the
double-unregister protection while avoiding the unregister-versus-callback
lock inversion.

Fixes: 849b0156d996 ("hwmon: (occ) Delay hwmon registration until user request")
Fixes: ac6888ac5a11 ("hwmon: (occ) Lock mutex in shutdown to prevent race with occ_active")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/hwmon/occ/common.c | 28 ++++++++++++++++++++++------
 drivers/hwmon/occ/common.h |  1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 89928d38831b..43dfea14d2ef 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -1106,11 +1106,16 @@ static void occ_parse_poll_response(struct occ *occ)
 
 int occ_active(struct occ *occ, bool active)
 {
-	int rc = mutex_lock_interruptible(&occ->lock);
+	struct device *hwmon = NULL;
+	int rc = mutex_lock_interruptible(&occ->hwmon_lock);
 
 	if (rc)
 		return rc;
 
+	rc = mutex_lock_interruptible(&occ->lock);
+	if (rc)
+		goto unlock_hwmon;
+
 	if (active) {
 		if (occ->active) {
 			rc = -EALREADY;
@@ -1155,14 +1160,17 @@ int occ_active(struct occ *occ, bool active)
 			goto unlock;
 		}
 
-		if (occ->hwmon)
-			hwmon_device_unregister(occ->hwmon);
+		hwmon = occ->hwmon;
 		occ->active = false;
 		occ->hwmon = NULL;
 	}
 
 unlock:
 	mutex_unlock(&occ->lock);
+	if (hwmon)
+		hwmon_device_unregister(hwmon);
+unlock_hwmon:
+	mutex_unlock(&occ->hwmon_lock);
 	return rc;
 }
 
@@ -1171,6 +1179,7 @@ int occ_setup(struct occ *occ)
 	int rc;
 
 	mutex_init(&occ->lock);
+	mutex_init(&occ->hwmon_lock);
 	occ->groups[0] = &occ->group;
 
 	rc = occ_setup_sysfs(occ);
@@ -1191,15 +1200,22 @@ EXPORT_SYMBOL_GPL(occ_setup);
 
 void occ_shutdown(struct occ *occ)
 {
-	mutex_lock(&occ->lock);
+	struct device *hwmon;
 
+	mutex_lock(&occ->hwmon_lock);
+	mutex_lock(&occ->lock);
+
 	occ_shutdown_sysfs(occ);
 
-	if (occ->hwmon)
-		hwmon_device_unregister(occ->hwmon);
+	hwmon = occ->hwmon;
+	occ->active = false;
 	occ->hwmon = NULL;
 
 	mutex_unlock(&occ->lock);
+
+	if (hwmon)
+		hwmon_device_unregister(hwmon);
+	mutex_unlock(&occ->hwmon_lock);
 }
 EXPORT_SYMBOL_GPL(occ_shutdown);
 
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index 7ac4b2febce6..82f600093c7f 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -101,6 +101,7 @@ struct occ {
 
 	unsigned long next_update;
 	struct mutex lock;		/* lock OCC access */
+	struct mutex hwmon_lock;		/* serialize hwmon registration/removal */
 
 	struct device *hwmon;
 	struct occ_attribute *attrs;
-- 
2.34.1

Re: [PATCH] hwmon: (occ) unregister hwmon device outside occ lock

[sashiko-bot]

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

New issues:
- [High] Sysfs callbacks can initiate hardware polling after the device has been deactivated.

Pre-existing issues:
- [High] The patch fixes an ABBA deadlock for hwmon sysfs attributes but leaves an identical deadlock for the driver's own occ_sysfs attributes.
- [High] Unbounded memory leak when toggling the OCC active state via sysfs.
- [High] A NULL pointer dereference crash occurs during sysfs group creation if the hardware reports multiple capability sensors.
- [High] Malformed device responses can cause an out-of-bounds kernel memory read when parsing sensor data blocks.
--

commit c52f29d2e70bdddb1a59be767e0322ae3c29585d
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

hwmon: (occ) unregister hwmon device outside occ lock

Serialize hwmon registration and removal with a separate hwmon_lock.
Under that lock, detach occ->hwmon and update occ->active while occ->lock
is held so concurrent OCC state changes still see a stable state, then
drop occ->lock before calling hwmon_device_unregister().

Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 42cc6068bb08b..b8461da29e36f 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -1105,11 +1105,16 @@ static void occ_parse_poll_response(struct occ *occ)
>  
>  int occ_active(struct occ *occ, bool active)
>  {
> -	int rc = mutex_lock_interruptible(&occ->lock);
> +	struct device *hwmon = NULL;
> +	int rc = mutex_lock_interruptible(&occ->hwmon_lock);
>  
>  	if (rc)
>  		return rc;
>  
> +	rc = mutex_lock_interruptible(&occ->lock);
> +	if (rc)
> +		goto unlock_hwmon;
> +

[Severity: High]
This is a pre-existing issue, but can this cause an unbounded memory leak when
toggling the OCC active state via sysfs?

When the device is activated, occ_setup_sensor_attrs() unconditionally
allocates the sensor attribute arrays using devm_kzalloc(). Because devres
allocations are bound to the device's lifetime, the memory is not freed when
occ_active() is called with active set to false.

Will repeatedly writing '1' and '0' to the occ_active sysfs file continuously
allocate new memory and overwrite the pointers?

[Severity: High]
This is a pre-existing issue, but does the hardware reporting multiple
capability sensors cause a NULL pointer dereference during sysfs group
creation?

In occ_setup_sensor_attrs(), the driver expects sensors->caps.num_sensors
attributes and allocates array space accordingly, but it only populates the
first capability sensor because it uses an
if (sensors->caps.num_sensors >= 1) condition instead of a loop:

occ_setup_sensor_attrs() {
    ...
    if (sensors->caps.num_sensors >= 1) {
        snprintf(attr->name, sizeof(attr->name), "power%d_label", s);
    ...
}

Are the remaining attributes left zero-initialized, causing
sysfs_create_group() to read a NULL attr->name and crash?

[Severity: High]
This is a pre-existing issue, but can malformed device responses cause an
out-of-bounds kernel memory read when parsing sensor data blocks?

In occ_parse_poll_response(), the bounds check is performed after reading the
block header:

occ_parse_poll_response() {
    ...
    offset = (block->header.num_sensors * block->header.sensor_length) +
             sizeof(block->header);
    size += offset;

    if ((size + sizeof(*header)) >= OCC_RESP_DATA_BYTES) {
    ...
}

If the response buffer is almost full, will the next iteration read the block
header out of bounds?

Additionally, the driver trusts the hardware-provided sensor_length without
verifying it matches the C struct size. Could a malicious or faulty OCC
provide a large num_sensors and small sensor_length, passing the bounds
check but causing sysfs functions like occ_show_power_a0() to read arbitrary
out-of-bounds kernel memory?

[ ... ]

> @@ -1154,14 +1159,17 @@ int occ_active(struct occ *occ, bool active)
>  			goto unlock;
>  		}
>  
> -		if (occ->hwmon)
> -			hwmon_device_unregister(occ->hwmon);
> +		hwmon = occ->hwmon;
>  		occ->active = false;
>  		occ->hwmon = NULL;
>  	}
>  
>  unlock:
>  	mutex_unlock(&occ->lock);
> +	if (hwmon)
> +		hwmon_device_unregister(hwmon);
> +unlock_hwmon:
> +	mutex_unlock(&occ->hwmon_lock);
>  	return rc;
>  }

[Severity: High]
Can sysfs callbacks initiate hardware polling after the device has been
deactivated?

By moving hwmon_device_unregister() outside of occ->lock and occ->hwmon_lock,
pending hwmon sysfs callbacks that were waiting for occ->lock can execute
after occ->active is set to false.

Since functions like occ_update_response() do not verify occ->active inside
their critical sections:

occ_update_response() {
    ...
    if (time_after(jiffies, occ->next_update)) {
        rc = occ_poll(occ);
    ...
}

will they proceed to call occ_poll() and send hardware commands to the OCC
after it has been deactivated?

[ ... ]

> @@ -1190,15 +1199,22 @@ EXPORT_SYMBOL_GPL(occ_setup);
>  
>  void occ_shutdown(struct occ *occ)
>  {
> +	struct device *hwmon;
> +
> +	mutex_lock(&occ->hwmon_lock);
>  	mutex_lock(&occ->lock);
>  
>  	occ_shutdown_sysfs(occ);

[Severity: High]
This is a pre-existing issue, but does holding both occ->hwmon_lock and
occ->lock before draining sysfs callbacks in occ_shutdown_sysfs() create
a deadlock?

If occ_shutdown_sysfs() blocks in kernfs_drain() waiting for active sysfs
references to drop, and concurrently a read/write to driver attributes
(like occ_active_store()) holds a kernfs active reference and attempts to
acquire occ->lock or occ->hwmon_lock, will this result in an unresolvable
circular dependency during driver unbind/shutdown?

>  
> -	if (occ->hwmon)
> -		hwmon_device_unregister(occ->hwmon);
> +	hwmon = occ->hwmon;
> +	occ->active = false;
>  	occ->hwmon = NULL;
>  
>  	mutex_unlock(&occ->lock);
> +
> +	if (hwmon)
> +		hwmon_device_unregister(hwmon);
> +	mutex_unlock(&occ->hwmon_lock);
>  }
>  EXPORT_SYMBOL_GPL(occ_shutdown);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617150859.741453-1-runyu.xiao@seu.edu.cn?part=1

[PATCH v2] hwmon: (occ) unregister sysfs devices outside occ lock

[Runyu Xiao]

occ_active(false) and occ_shutdown() unregister sysfs-backed devices while
occ->lock is held.  hwmon_device_unregister() and sysfs_remove_group() can
wait for active sysfs callbacks to drain, and those callbacks can enter the
OCC update path and try to take occ->lock again.  That gives the unregister
paths the lock ordering occ->lock -> sysfs callback drain, while a callback
has the opposite edge sysfs callback -> occ->lock.

This issue was found by our static analysis tool and then manually
reviewed against the current tree.

The grounded PoC kept the real unregister and callback carrier:

  occ_shutdown()
  hwmon_device_unregister()
  occ_show_temp_1()
  occ_update_response()

Lockdep reported the circular dependency with occ_shutdown() already
holding the OCC mutex and hwmon_device_unregister() waiting on the sysfs
side:

  WARNING: possible circular locking dependency detected
  ... (sysfs_lock) ... at: hwmon_device_unregister+0x12/0x30 [vuln_msv]
  ... (&test_occ.lock) ... at: occ_shutdown.constprop.0+0xe/0x40 [vuln_msv]
  occ_update_response.isra.0+0xb/0x20 [vuln_msv]
  occ_show_temp_1.constprop.0.isra.0+0x23/0x40 [vuln_msv]
  *** DEADLOCK ***

Serialize hwmon registration and removal with a separate hwmon_lock.
Under that lock, detach occ->hwmon and update occ->active while occ->lock
is held so concurrent OCC state changes still see a stable state, then
drop occ->lock before calling hwmon_device_unregister().  Remove the
driver sysfs group before taking occ->lock in occ_shutdown(), so draining
the driver attributes cannot wait while the OCC mutex is held.  Also make
OCC update callbacks return -ENODEV after deactivation, so callbacks that
already passed sysfs active protection do not poll the hardware after
teardown has detached the hwmon device.

Fixes: 849b0156d996 ("hwmon: (occ) Delay hwmon registration until user request")
Fixes: ac6888ac5a11 ("hwmon: (occ) Lock mutex in shutdown to prevent race with occ_active")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
Changes in v2:
- Return -ENODEV from occ_update_response() after OCC deactivation so
  already-active sysfs callbacks do not poll hardware after teardown.
- Move occ_shutdown_sysfs() outside occ->lock so driver sysfs callback
  draining does not run while the OCC mutex is held.

 drivers/hwmon/occ/common.c | 34 ++++++++++++++++++++++++++++------
 drivers/hwmon/occ/common.h |  1 +
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 89928d38831b..567b7bc2a6e9 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -214,6 +214,11 @@ int occ_update_response(struct occ *occ)
 	if (rc)
 		return rc;
 
+	if (!occ->active) {
+		rc = -ENODEV;
+		goto unlock;
+	}
+
 	/* limit the maximum rate of polling the OCC */
 	if (time_after(jiffies, occ->next_update)) {
 		rc = occ_poll(occ);
@@ -222,6 +227,7 @@ int occ_update_response(struct occ *occ)
 		rc = occ->last_error;
 	}
 
+unlock:
 	mutex_unlock(&occ->lock);
 	return rc;
 }
@@ -1106,11 +1112,16 @@ static void occ_parse_poll_response(struct occ *occ)
 
 int occ_active(struct occ *occ, bool active)
 {
-	int rc = mutex_lock_interruptible(&occ->lock);
+	struct device *hwmon = NULL;
+	int rc = mutex_lock_interruptible(&occ->hwmon_lock);
 
 	if (rc)
 		return rc;
 
+	rc = mutex_lock_interruptible(&occ->lock);
+	if (rc)
+		goto unlock_hwmon;
+
 	if (active) {
 		if (occ->active) {
 			rc = -EALREADY;
@@ -1155,14 +1166,17 @@ int occ_active(struct occ *occ, bool active)
 			goto unlock;
 		}
 
-		if (occ->hwmon)
-			hwmon_device_unregister(occ->hwmon);
+		hwmon = occ->hwmon;
 		occ->active = false;
 		occ->hwmon = NULL;
 	}
 
 unlock:
 	mutex_unlock(&occ->lock);
+	if (hwmon)
+		hwmon_device_unregister(hwmon);
+unlock_hwmon:
+	mutex_unlock(&occ->hwmon_lock);
 	return rc;
 }
 
@@ -1171,6 +1185,7 @@ int occ_setup(struct occ *occ)
 	int rc;
 
 	mutex_init(&occ->lock);
+	mutex_init(&occ->hwmon_lock);
 	occ->groups[0] = &occ->group;
 
 	rc = occ_setup_sysfs(occ);
@@ -1191,15 +1206,22 @@ EXPORT_SYMBOL_GPL(occ_setup);
 
 void occ_shutdown(struct occ *occ)
 {
-	mutex_lock(&occ->lock);
+	struct device *hwmon;
 
 	occ_shutdown_sysfs(occ);
 
-	if (occ->hwmon)
-		hwmon_device_unregister(occ->hwmon);
+	mutex_lock(&occ->hwmon_lock);
+	mutex_lock(&occ->lock);
+
+	hwmon = occ->hwmon;
+	occ->active = false;
 	occ->hwmon = NULL;
 
 	mutex_unlock(&occ->lock);
+
+	if (hwmon)
+		hwmon_device_unregister(hwmon);
+	mutex_unlock(&occ->hwmon_lock);
 }
 EXPORT_SYMBOL_GPL(occ_shutdown);
 
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index 7ac4b2febce6..82f600093c7f 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -101,6 +101,7 @@ struct occ {
 
 	unsigned long next_update;
 	struct mutex lock;		/* lock OCC access */
+	struct mutex hwmon_lock;		/* serialize hwmon registration/removal */
 
 	struct device *hwmon;
 	struct occ_attribute *attrs;

base-commit: 44c944a679974c2d18ee9b87070456d34193f3d4

Re: [PATCH v2] hwmon: (occ) unregister sysfs devices outside occ lock

[sashiko-bot]

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

Pre-existing issues:
- [High] Repeated toggling of the `occ_active` sysfs attribute causes an unbounded memory leak due to unbalanced `devm_kcalloc` allocations.
- [High] Concurrent sysfs activation during device probe causes `occ_setup()` to fail, leading to a Use-After-Free of the `occ` struct.
- [High] Missing validation of `sensor_length` from untrusted OCC hardware response allows an out-of-bounds read, potentially leaking kernel memory to userspace.
--

commit 56e5ee484ac45f4849cdd68d58d6bdab8f0530ab
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

hwmon: (occ) unregister sysfs devices outside occ lock

occ_active(false) and occ_shutdown() unregister sysfs-backed devices while
occ->lock is held.  hwmon_device_unregister() and sysfs_remove_group() can
wait for active sysfs callbacks to drain, and those callbacks can enter the
OCC update path and try to take occ->lock again.  That gives the unregister
paths the lock ordering occ->lock -> sysfs callback drain, while a callback
has the opposite edge sysfs callback -> occ->lock.

Fixes: 849b0156d996 ("hwmon: (occ) Delay hwmon registration until user request")
Fixes: ac6888ac5a11 ("hwmon: (occ) Lock mutex in shutdown to prevent race with occ_active")
Cc: stable@vger.kernel.org

> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 89928d38831b6..567b7bc2a6e91 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c

[ ... ]

> @@ -1106,11 +1112,16 @@ static void occ_parse_poll_response(struct occ *occ)

[Severity: High]
This isn't a bug introduced by this patch, but does occ_parse_poll_response()
validate that sensor_length from the hardware response matches the size of the
mapped structures?

In occ_parse_poll_response(), the driver computes the block size as
num_sensors * sensor_length and verifies it fits within the 4096-byte
response buffer.  However, it fails to verify that sensor_length matches
the expected size of the C structures mapping the data.

A malfunctioning OCC firmware could return a large num_sensors and a small
sensor_length (e.g., 1), easily passing the bounds check.

Later, sysfs show functions like occ_show_power_a0() perform pointer
arithmetic using the sensor index and the fixed struct size:

    power = ((struct power_sensor_a0 *)sensors->power.data) + sattr->index;

For a large index, this calculates an address far beyond the end of the
occ->resp allocation, causing an out-of-bounds read of adjacent kernel
heap memory that can be emitted to userspace.

>  
>  int occ_active(struct occ *occ, bool active)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does repeatedly toggling the
occ_active sysfs attribute cause a memory leak of occ->attrs?

When occ_active(occ, true) is executed (e.g., via writing '1' to the
occ_active sysfs file), occ_setup_sensor_attrs() dynamically allocates
memory using devm_kcalloc():

    occ->attrs = devm_kcalloc(dev, num_attrs, sizeof(*occ->attrs),
                              GFP_KERNEL);

When the device is deactivated via occ_active(occ, false), the hwmon device
is unregistered, but the memory allocated by devm_kcalloc() is not freed
because it remains tied to the parent device's lifecycle (occ->bus_dev).

Can a privileged user repeatedly toggle the sysfs attribute to continuously
leak memory from previous allocations?

> -	int rc = mutex_lock_interruptible(&occ->lock);
> +	struct device *hwmon = NULL;
> +	int rc = mutex_lock_interruptible(&occ->hwmon_lock);
>  
>  	if (rc)
>  		return rc;

[ ... ]

> @@ -1171,6 +1185,7 @@ int occ_setup(struct occ *occ)
>  	int rc;
>  
>  	mutex_init(&occ->lock);

[Severity: High]
This is a pre-existing issue, but can concurrent sysfs activation during
device probe cause a use-after-free of the struct occ?

occ_setup() exposes the occ_active sysfs attribute before attempting
to activate the hardware.

If a concurrent process writes '1' to occ_active, occ_active_store()
successfully registers the hwmon device and sets occ->active = true.

The probe thread then resumes and calls occ_active(occ, true), which
returns -EALREADY because the device is now active.

occ_setup() treats this as a failure, removes the sysfs group, and
returns an error:

    rc = occ_active(occ, true);
    if (rc)
        occ_shutdown_sysfs(occ);
    return rc;

This causes the platform/i2c probe to fail and the driver core frees
the struct occ memory via devres cleanup.

However, the hwmon device registered by the concurrent thread is never
unregistered on this error path, meaning any subsequent read of hwmon
attributes will dereference the freed occ pointer.

> +	mutex_init(&occ->hwmon_lock);
>  	occ->groups[0] = &occ->group;
>  
>  	rc = occ_setup_sysfs(occ);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619015938.494464-1-runyu.xiao@seu.edu.cn?part=1