Re: [PATCH v6 01/10] ACPI: APEI: GHES: share macros via a private header

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing irq_work_sync() during GHES device unbind leads to a Use-After-Free of `struct ghes`.
--

commit e704c2dde2d2667b464a2f5a2c5c45ed826cc539
Author: Ahmed Tiba <ahmed.tiba@arm.com>

ACPI: APEI: GHES: share macros via a private header

This commit moves CPER helper macros and function prototypes out of
ghes.c and into a private header. This allows these helpers to be
shared with other firmware-first error sources.

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3236a3ce79d6b..4f67f46410c46 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
[ ... ]
> @@ -313,7 +255,7 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic)
>  	return ERR_PTR(rc);
>  }
>  
> -static void ghes_fini(struct ghes *ghes)
> +void ghes_fini(struct ghes *ghes)
>  {

[Severity: High]
This is a pre-existing issue, but does ghes_remove() need to call
irq_work_sync() before freeing the ghes structure to prevent a
use-after-free?

When unbinding a GHES device, ghes_remove() unregisters the error
notification handlers and calls synchronize_rcu(). While this ensures no new
handlers will execute, it does not wait for any already-scheduled irq_work
to complete:

drivers/acpi/apei/ghes.c:ghes_remove() {
	...
	ghes_fini(ghes);
	...
	kfree(ghes);
}

If irq_work_queue(&ghes_proc_irq_work) was called just before the handler was
unregistered, the work remains pending. When the asynchronous work runs, it
dequeues the node and dereferences the freed ghes pointer:

drivers/acpi/apei/ghes.c:ghes_proc_in_irq() {
	...
	estatus_node = llist_entry(llnode, struct ghes_estatus_node, llnode);
	estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
	...
	ghes_do_proc(estatus_node->ghes, estatus);
}

Could this lead to a use-after-free when ghes_do_proc() accesses ghes->dev
and ghes->generic?

>  	kfree(ghes->estatus);
>  	apei_unmap_generic_address(&ghes->generic->error_status_address);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=1