Re: [PATCH v7 08/10] ACPI: APEI: share GHES CPER helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Out-of-bounds read when copying AER capability registers due to size mismatch.
- [High] Use-After-Free of `twcb` in `memory_failure_cb()`.
- [High] Missing ACPI payload length checks for CPER_SEC_PCIE and CPER_SEC_PLATFORM_MEM.
- [Medium] Time-of-check to time-of-use (TOCTOU) race conditions in CXL CPER registration allows overwriting work pointers.
- [Medium] FRU ID and FRU Text state leaks across CPER sections.
--

--- Patch [8]: [PATCH v7 08/10] ACPI: APEI: share GHES CPER helpers ---
commit b197b806e546fd072c8420ae76e7be2f42261acb
Author: Ahmed Tiba <ahmed.tiba@arm.com>

ACPI: APEI: share GHES CPER helpers

Wire GHES up to the helper routines in ghes_cper.c and remove the local
copies from ghes.c. This keeps the control flow identical while letting
the helpers be shared with other firmware-first providers.

Serialize ghes_estatus_pool_init() and allow later users to extend the
shared pool instead of racing a second initialization or silently
reusing a pool sized only for the first user.

> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> index b59e3ed3eab36..ec8881ada97ef 100644
> --- a/drivers/acpi/apei/ghes_cper.c
> +++ b/drivers/acpi/apei/ghes_cper.c

[ ... ]

> +static void memory_failure_cb(struct callback_head *twork)
> +{
> +	struct ghes_task_work *twcb = container_of(twork, struct ghes_task_work, twork);
> +	int ret;
> +
> +	ret = memory_failure(twcb->pfn, twcb->flags);
> +	gen_pool_free(ghes_estatus_pool, (unsigned long)twcb, sizeof(*twcb));
> +
> +	if (!ret || ret == -EHWPOISON || ret == -EOPNOTSUPP)
> +		return;
> +
> +	pr_err("%#llx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
> +			twcb->pfn, current->comm, task_pid_nr(current));

[Severity: High]
This is a pre-existing issue, but can we dereference a freed pointer here?

When handling a hardware memory error asynchronously via memory_failure_cb(),
the task work struct twcb is freed back to ghes_estatus_pool:

    gen_pool_free(ghes_estatus_pool, (unsigned long)twcb, sizeof(*twcb));

If memory_failure() returns an unhandled error code, the subsequent pr_err()
attempts to read twcb->pfn:

    pr_err("%#llx: Sending SIGBUS ...", twcb->pfn, ...);

Could this Use-After-Free result in printing garbage data or cause a panic?

[ ... ]

> +void ghes_handle_aer(struct acpi_hest_generic_data *gdata)
> +{
> +#ifdef CONFIG_ACPI_APEI_PCIEAER
> +	struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
> +
> +	if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&

[Severity: High]
This is a pre-existing issue, but is it possible for the firmware to provide
a payload smaller than expected?

The pcie_err payload is cast from gdata and accessed directly. If the
firmware submits an error record where gdata->error_data_length is smaller
than the size of struct cper_sec_pcie, does this result in an out-of-bounds
read?

> +	    pcie_err->validation_bits & CPER_PCIE_VALID_AER_INFO) {
> +		unsigned int devfn;
> +		int aer_severity;
> +		u8 *aer_info;

[ ... ]

> +		aer_info = (void *)gen_pool_alloc(ghes_estatus_pool,
> +						  sizeof(struct aer_capability_regs));
> +		if (!aer_info)
> +			return;
> +		memcpy(aer_info, pcie_err->aer_info, sizeof(struct aer_capability_regs));

[Severity: High]
This is a pre-existing issue, but does this memcpy() read past the end of
the array?

The size of struct aer_capability_regs is 100 bytes, but the aer_info field
in struct cper_sec_pcie is a fixed 96-byte array:

    u8 aer_info[96];

Does copying sizeof(struct aer_capability_regs) bytes from pcie_err->aer_info
cause an out-of-bounds read of up to 4 bytes past the section structure?

[ ... ]

> +void ghes_cper_handle_status(struct device *dev,
> +			     const struct acpi_hest_generic *generic,
> +			     const struct acpi_hest_generic_status *estatus,
> +			     bool sync)
> +{
> +	int sev, sec_sev;
> +	struct acpi_hest_generic_data *gdata;
> +	guid_t *sec_type;
> +	const guid_t *fru_id = &guid_null;
> +	char *fru_text = "";
> +	bool queued = false;
> +
> +	sev = ghes_severity(estatus->error_severity);
> +	apei_estatus_for_each_section(estatus, gdata) {
> +		sec_type = (guid_t *)gdata->section_type;
> +		sec_sev = ghes_severity(gdata->error_severity);
> +		if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
> +			fru_id = (guid_t *)gdata->fru_id;
> +
> +		if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
> +			fru_text = gdata->fru_text;

[Severity: Medium]
This is a pre-existing issue, but do we leak FRU ID and Text state across
sections in this loop?

If an APEI estatus contains multiple sections, and the first section contains
a valid FRU ID and Text, fru_id and fru_text are updated. If a subsequent
section does not have these validation bits set, the variables are not reset
to &guid_null and "".

Does this cause subsequent sections to incorrectly inherit the previous
section's FRU strings?

> +
> +		ghes_log_hwerr(sev, sec_type);
> +		if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
> +			struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
> +
> +			atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);

[Severity: High]
This is a pre-existing issue, but similar to the AER handling above, do we
need to validate the payload length here?

The code unconditionally casts the payload and passes it along. Unlike the
ARM sections which explicitly validate gdata->error_data_length, could a
malformed firmware record smaller than struct cper_sec_mem_err cause 
out-of-bounds reads when accessed in the notifier chain?

[ ... ]

Additionally, while reviewing ghes_cper.c, I noticed another pre-existing 
issue in cxl_cper_register_work() which is not modified by this patch:

[Severity: Medium]
Is there a race condition when checking cxl_cper_work?

In cxl_cper_register_work():

    if (cxl_cper_work)
        return -EINVAL;

    guard(spinlock_irqsave)(&cxl_cper_work_lock);
    cxl_cper_work = work;

The check against cxl_cper_work happens outside the lock. If two threads
concurrently call this API, both could see cxl_cper_work as NULL, sequentially
acquire the lock, and assign their pointers. Could this silently overwrite
a registered work struct pointer?

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