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

[sashiko-bot@kernel.org]

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