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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unmapping GHES error status memory races with NMI/SEA handlers reading it locklessly under RCU.
- [High] Lockless iteration of `ghes_devs` in `ghes_edac` races with `ghes_remove`.
--

--- Patch [1]: [PATCH v7 01/10] ACPI: APEI: GHES: share macros via a private header ---
commit cfd79e83a33cc5873f3f8f572468468d4d6a93f1
Author: Ahmed Tiba <ahmed.tiba@arm.com>

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

Carve the CPER helper macros out of ghes.c and place them in a private
header so they can be shared with upcoming helper files. Move the vendor
record entry declaration and the prototypes for the CPER read and clear
helpers along with ghes_new() and ghes_fini() into the same header. This
requires dropping their local static visibility in ghes.c. Also synchronize
ghes_proc_irq_work during ghes_remove() to avoid a use-after-free.

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3236a3ce79d6b..c73a316644e8a 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)
>  {
>  	kfree(ghes->estatus);
>  	apei_unmap_generic_address(&ghes->generic->error_status_address);

[Severity: High]
This isn't a bug introduced by this patch, but does unmapping the error
status memory here race with NMI/SEA handlers reading it locklessly
under RCU?

If a root user unbinds the GHES driver (triggering ghes_nmi_remove() or
ghes_sea_remove()), those functions call ghes_unmap_error_status() and
unmap the I/O memory before synchronize_rcu() completes.

For example, an NMI/SEA handler locklessly iterating the list under
guard(rcu)() in ghes_has_active_errors() could concurrently access
ghes->error_status_vaddr and execute readl() on the unmapped pointer:

drivers/acpi/apei/ghes.c:ghes_has_active_errors() {
    ...
    if (ghes->error_status_vaddr &&
        readl(ghes->error_status_vaddr))
    ...
}

Could this trigger a kernel panic due to a fatal page fault or data abort?

[ ... ]
> @@ -1852,6 +1794,7 @@ static void ghes_remove(struct platform_device *ghes_dev)
>  		break;
>  	}
>  
> +	irq_work_sync(&ghes_proc_irq_work);
>  	ghes_fini(ghes);
>  
>  	mutex_lock(&ghes_devs_mutex);

[Severity: High]
This is a pre-existing issue, but is it safe for ghes_remove() to lock
ghes_devs_mutex, remove devices from the list, and free them with
kfree(ghes) while ghes_edac_init() iterates this list locklessly?

If an administrator loads the ghes_edac module while concurrently unbinding
a GHES device via sysfs, ghes_edac_init() iterates over the global
ghes_devs list without acquiring ghes_devs_mutex:

drivers/edac/ghes_edac.c:ghes_edac_init() {
    ...
    list_for_each_entry_safe(g, g_tmp, ghes_devs, elist) {
        ghes_edac_register(g->dev);
    }
    ...
}

Since list_for_each_entry_safe() does not protect against concurrent
deletions by other threads, could this lead to a use-after-free or list
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-topics-ahmtib01-ras_ffh_arm_internal_review-v7-0-8b3a85216cef@arm.com?part=1