Re: [PATCH v2 1/7] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length

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

New issues:
- [Medium] The new length validation ties the minimum firmware section size to the kernel's internal maximum union size, which will break backwards compatibility with existing firmwares as soon as the CXL event union grows.

Pre-existing issues:
- [High] Deadlock due to acquiring a spinlock in process context without disabling interrupts, while the same lock is acquired in hardware interrupt context.
- [High] The newly added length check `len < sizeof(*rec)` can be bypassed via an integer overflow, defeating the patch's protection and allowing an out-of-bounds kernel memory read.
--

--- Patch [1]: [PATCH v2 1/7] ACPI: APEI: GHES: Bound CXL event record copy to the firmware section length ---
commit e26fd5aa15a7b2ef67720b7ee480d05f9c35224c
Author: Dave Jiang <[email protected]>

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

This commit adds a bounds check to cxl_cper_post_event() to prevent copying
more data than what the firmware provided in the CPER section. It passes
the error data length into the function to reject undersized sections.

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 3236a3ce79d6b..9916271a81513 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -815,10 +815,21 @@ 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 isn't a bug introduced by this patch, but can this lock cause a
deadlock?

Looking at how cxl_cper_work_lock is acquired in process context:

drivers/acpi/apei/ghes.c:cxl_cper_register_work() {
    guard(spinlock)(&cxl_cper_work_lock);
    ...
}

And how it is acquired in hardirq context here in cxl_cper_post_event():

drivers/acpi/apei/ghes.c:cxl_cper_post_event() {
    ...
    guard(spinlock_irqsave)(&cxl_cper_work_lock);
    ...
}

If a process calls cxl_cper_register_work() and acquires the lock without
disabling local interrupts, and a hardware error triggers an interrupt on
the same CPU, cxl_cper_post_event() will attempt to acquire the exact same
lock. Does this violate the locking rule that if a lock is used in hardirq
context, all process-context acquisitions must disable interrupts?

>  	struct cxl_cper_work_data wd;
>  
> +	/*
> +	 * The record is copied whole below, so the firmware-provided section
> +	 * must be at least as large as the record. cper_estatus_check() only
> +	 * validates the section fits the error block, not that it satisfies a
> +	 * given section type's size, so guard the fixed-size copy here.
> +	 */
> +	if (len < sizeof(*rec)) {

[Severity: High]
This is a pre-existing issue, but does this length check remain vulnerable
to an integer overflow bypass?

If the firmware provides a length like 0xFFFFFFB9, upstream validation
implicitly casts this u32 to a signed int:

include/acpi/ghes.h:acpi_hest_get_error_length() {
    return ((struct acpi_hest_generic_data *)(gdata))->error_data_length;
}

This returns a negative value, which underflows the size calculation and
bypasses the estatus bounds check.

When execution reaches this point, len is evaluated as the unsigned value
0xFFFFFFB9. The check 0xFFFFFFB9 < sizeof(*rec) evaluates to false,
bypassing this new protection and allowing the memcpy to read out of bounds.

[Severity: Medium]
Will this strict size requirement break backwards compatibility with older
firmwares if the kernel's internal event union structure increases in size?

By enforcing that len is at least sizeof(*rec) (which currently contains
union cxl_event), the minimum firmware section size is tied to the kernel's
internal maximum union size.

If a future CXL specification introduces a larger event type and the kernel
expands the union, sizeof(*rec) will increase. When that happens, would the
updated kernel erroneously reject perfectly valid legacy events emitted by
existing older firmwares?

Could we instead verify len >= sizeof(rec->hdr), zero-initialize the work
data buffer, and safely copy min_t(size_t, len, sizeof(*rec)) bytes to
decouple firmware section sizes from internal kernel union sizes?

> +		pr_err(FW_WARN "CXL CPER section too small (%u)\n", len);
> +		return;
> +	}
> +
>  	if (rec->hdr.length <= sizeof(rec->hdr) ||
>  	    rec->hdr.length > sizeof(*rec)) {
>  		pr_err(FW_WARN "CXL CPER Invalid section length (%u)\n",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1