Re: [PATCH v5 08/13] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Potential AB-BA deadlock due to missing IRQ disablement when acquiring spinlocks shared with hardirq context.
--

commit 5c5cd12a22557cc71932382234f47a7a0364bb43
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

cxl_cper_post_event() copies a fixed sizeof(struct cxl_cper_event_rec)
out of the firmware CPER section without checking how long the section
actually is. This patch passes the section length in and rejects sections
too small to hold the record before the copy.

Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index a382aabf9835e..e5f8dbd17017e 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -816,10 +816,15 @@ static DEFINE_SPINLOCK(cxl_cper_work_lock);
>  struct work_struct *cxl_cper_work;
>  
>  static void cxl_cper_post_event(enum cxl_event_type event_type,
> -				struct cxl_cper_event_rec *rec)
> +				struct cxl_cper_event_rec *rec, u32 len)
>  {

[Severity: High]
This is a pre-existing issue, but does the locking strategy here expose the
kernel to a potential deadlock?

cxl_cper_post_event() and cxl_cper_post_prot_err() execute in hardirq context
(e.g., via ghes_proc_in_irq() or ghes_irq_func()) and acquire
cxl_cper_work_lock or cxl_cper_prot_err_work_lock.

However, the registration functions in drivers/acpi/apei/ghes.c such as
cxl_cper_register_work(), cxl_cper_unregister_work(),
cxl_cper_register_prot_err_work(), and cxl_cper_unregister_prot_err_work()
execute in process context and acquire these same spinlocks using
guard(spinlock), which expands to spin_lock() and does not disable local
interrupts.

If a CPU executing one of these registration functions is interrupted by a
hardware error IRQ while holding the lock, the interrupt handler will attempt
to acquire the exact same lock in cxl_cper_post_event() or
cxl_cper_post_prot_err(), resulting in an immediate deadlock.

Should the registration functions use guard(spinlock_irqsave) instead of
guard(spinlock) to prevent this?

>  	struct cxl_cper_work_data wd;
>  
> +	if (len < sizeof(*rec)) {
> +		pr_err(FW_WARN "CXL CPER section too small (%u)\n", len);
> +		return;
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827203726.3027541-1-dave.jiang@intel.com?part=8