Re: [PATCH 05/11] ACPI: CPPC: Free cpc_desc in kobject release callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] acpi: cppc: dangling per-CPU pointer after exit leads to UAF
- [Medium] acpi/cppc: missing kobject_del() causes sysfs -EEXIST on driver re-bind
--

commit 1405c81459a62da53ac116a49a65cbdc3bcdcd68
Author: Ian Rogers <irogers@google.com>

ACPI: CPPC: Free cpc_desc in kobject release callback

cpc_desc_ptr embeds a struct kobject that is registered in sysfs via
kobject_init_and_add(), but cppc_ktype lacked a .release callback and
acpi_cppc_processor_exit() freed cpc_ptr immediately after kobject_put().
This patch moves the sys_mem_vaddr unmapping and kfree(cpc_ptr) into a new
cppc_release() callback.

> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index f370be8715ae2..0fefe1cdb60ce 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> [ ... ]
> @@ -961,15 +966,7 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
>  	if (!cpc_ptr)
>  		return;
>  
> -	/* Free all the mapped sys mem areas for this CPU */
> -	for (i = 2; i < cpc_ptr->num_entries; i++) {
> -		addr = cpc_ptr->cpc_regs[i-2].sys_mem_vaddr;
> -		if (addr)
> -			iounmap(addr);
> -	}
> -
>  	kobject_put(&cpc_ptr->kobj);

[Severity: High]
Does this leave a dangling pointer in the per-CPU variable?

When acpi_cppc_processor_exit() drops the kobject refcount, the underlying
memory is freed. However, it seems per_cpu(cpc_desc_ptr, pr->id) is never
set to NULL.

Could other CPPC functions that rely on this pointer being NULL if
uninitialized, such as cppc_get_perf_caps():

	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
	// ...
	if (!cpc_desc) {
		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
		return -ENODEV;
	}

successfully pass the check on the dangling pointer and trigger a
use-after-free upon driver unbind?

[Severity: Medium]
Could omitting kobject_del() here cause a sysfs -EEXIST error when the
driver is re-bound?

The teardown path drops the reference but appears to omit explicitly
unregistering the sysfs directory. If the kobject's refcount does not
immediately reach 0 (for example, if userspace has a CPPC sysfs file open,
or if CONFIG_DEBUG_KOBJECT_RELEASE=y defers the release), the sysfs
directory is not synchronously removed.

Should kobject_del() be called before kobject_put() to ensure a subsequent
kobject_add() in the probe path does not collide with a lingering sysfs
directory?

> -	kfree(cpc_ptr);
>  }
>  EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013223946.1836757-1-irogers@google.com?part=5