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

[sashiko-bot@kernel.org]

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