[PATCH] powercap: Do not release the zone when device_register() fails

[PATCH] powercap: Do not release the zone when device_register() fails

[Yuqiang Sun]

powercap_register_zone() registers a power zone that the caller
allocated, usually embedded in a larger structure, and on most
failures returns with the zone untouched, leaving it to the caller.
Since commit 1b6599f741a4 ("powercap: fix possible name leak in
powercap_register_zone()"), a failure of device_register() is handled
with put_device() instead, and the release, powercap_release(), also
calls the zone's ->release() callback.

For DTPM that callback, dtpm_release_zone(), frees the node, either
directly or through the node's own ->release() for the CPU and devfreq
backends, and all three callers of dtpm_register() free it again when
it fails: dtpm_setup_virtual(), __dtpm_cpu_setup() and
__dtpm_devfreq_setup().

With one allocation failure injected into device_register() (failslab
restricted to dtpm_register) while rockchip_dtpm builds its hierarchy,
KASAN reports:

  BUG: KASAN: double-free in dtpm_setup_virtual+0x15c/0x19c
  Call trace:
   kfree
   dtpm_setup_virtual
   dtpm_for_each_child
   dtpm_create_hierarchy
  Freed by task 67:
   kfree
   dtpm_release_zone
   powercap_release
   device_release
   kobject_put
   put_device
   powercap_register_zone
   dtpm_register
   dtpm_setup_virtual

Keep the put_device(), which frees the device name and what
powercap_register_zone() allocated, but clear the zone's ops first so
that the release does not call ->release(): the zone is left to the
caller, as on the other failures.

The problem was found with kbound, a static analysis prototype that
checks whether callers rely on an object after a callee returns a value
for which the callee may have released it, on v7.2.12; it is present
in linux-pm's linux-next branch.

Build-tested with x86_64 and arm64 allmodconfig (W=1, sparse): no new
warnings.  Run-tested on QEMU's arm64 virt machine (TCG) with KASAN,
with its device tree's root compatible set to "rockchip,rk3399", so
that rockchip_dtpm registers its two virtual nodes without the
hardware, failing each allocation in dtpm_register() in turn with
failslab: before this patch every failure in device_register() (4 of
4) gives the report above; with it nothing is reported, and the
earlier failures still return an error without reports, as before.
Not tested on Rockchip hardware.

Fixes: 1b6599f741a4 ("powercap: fix possible name leak in powercap_register_zone()")
Cc: stable@vger.kernel.org
Signed-off-by: Yuqiang Sun <izaiahsun@gmail.com>
---

Notes:
    The bug was found with kbound, a research static analysis prototype.
    The patch, the changelog and the test setup were written with the help
    of an AI coding assistant (Claude); I have reviewed and tested them as
    described above.
    
    With CONFIG_DEBUG_KOBJECT_RELEASE the same injected failures still give
    use-after-free reports, with and without this patch: the release of
    the zone's device is delayed, while the caller frees the structure the
    device is embedded in right away.  Avoiding that would need the callers
    to free a zone only through its ->release(), which changes what
    powercap_register_zone() and dtpm_register() mean on failure for all
    their users.  I kept this patch to the minimal fix and can look into
    the larger change if you prefer that.

 drivers/powercap/powercap_sys.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
index 9197fa20d93f..eacbc3c96db1 100644
--- a/drivers/powercap/powercap_sys.c
+++ b/drivers/powercap/powercap_sys.c
@@ -398,7 +398,7 @@ static void powercap_release(struct device *dev)
 		kfree(power_zone->name);
 		kfree(power_zone->zone_dev_attrs);
 		kfree(power_zone->constraints);
-		if (power_zone->ops->release)
+		if (power_zone->ops && power_zone->ops->release)
 			power_zone->ops->release(power_zone);
 		if (allocated)
 			kfree(power_zone);
@@ -555,6 +555,12 @@ struct powercap_zone *powercap_register_zone(
 					power_zone->id);
 	result = device_register(&power_zone->dev);
 	if (result) {
+		/*
+		 * As on the other failures, the zone is left to the caller:
+		 * the release frees what was allocated here, but must not call
+		 * ->release(), which may free the zone.
+		 */
+		power_zone->ops = NULL;
 		put_device(&power_zone->dev);
 		mutex_unlock(&control_type->lock);
 

base-commit: 4785d7fc44de81f850101df9f8627775a6010c5a
-- 
2.53.0