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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect pointer arithmetic leads to out-of-bounds reads in ARM hardware error handling.
- [High] State overwrite of the `queued` flag can mask unrecoverable synchronous errors.
- [High] Permanent memory leak from the fixed-size `ghes_estatus_pool` when `task_work_add` fails.
- [High] Permanent memory leak from the fixed-size `ghes_estatus_pool` when the AER recovery kfifo is full.
- [High] Unsafe initialization semantics of `ghes_estatus_pool_init` risk pool corruption or NULL pointer dereferences for shared providers.
- [Medium] Incomplete decoupling of APEI Kconfig dependencies prevents functional sharing with non-APEI providers.
--

commit 593ad4fbd91409a6d18986b04b8adfb0d9f1ad5a
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.

> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> index d7a666a163c3a..0ff9d06eb78f9 100644
> --- a/drivers/acpi/apei/ghes_cper.c
> +++ b/drivers/acpi/apei/ghes_cper.c
[ ... ]
> +int ghes_estatus_pool_init(unsigned int num_ghes)
> +{
> +	unsigned long addr, len;
> +	int rc;
> +
> +	ghes_estatus_pool = gen_pool_create(GHES_ESTATUS_POOL_MIN_ALLOC_ORDER, -1);
> +	if (!ghes_estatus_pool)
> +		return -ENOMEM;
[ ... ]
> +	return 0;
> +
> +err_pool_add:
> +	vfree((void *)addr);
> +
> +err_pool_alloc:
> +	gen_pool_destroy(ghes_estatus_pool);
> +
> +	return -ENOMEM;
> +}
> +EXPORT_SYMBOL_GPL(ghes_estatus_pool_init);

Is it safe to export this initialization function without safeguards against
multiple invocations?

If multiple firmware-first providers call it, ghes_estatus_pool might be
unconditionally overwritten, leading to leaked memory and corruption when
older objects are freed into the new pool. Also, if a non-GHES provider
uses the helpers without ensuring initialization, could gen_pool_alloc crash
on a NULL pool?

[ ... ]
> +static bool ghes_do_memory_failure(u64 physical_addr, int flags)
> +{
> +	struct ghes_task_work *twcb;
> +	unsigned long pfn;
> +
> +	if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
> +		return false;

The commit message states the goal is to let these helpers be shared with
other firmware-first providers. However, functions like
ghes_do_memory_failure() and ghes_handle_aer() are guarded by
CONFIG_ACPI_APEI_MEMORY_FAILURE and CONFIG_ACPI_APEI_PCIEAER.

Since a non-APEI provider might not have these enabled, will memory and
AER errors be silently dropped?

[ ... ]
> +	if (flags == MF_ACTION_REQUIRED && current->mm) {
> +		twcb = (void *)gen_pool_alloc(ghes_estatus_pool, sizeof(*twcb));
> +		if (!twcb)
> +			return false;
> +
> +		twcb->pfn = pfn;
> +		twcb->flags = flags;
> +		init_task_work(&twcb->twork, memory_failure_cb);
> +		task_work_add(current, &twcb->twork, TWA_RESUME);
> +		return true;
> +	}

This isn't a bug introduced by this patch, but pre-exists in the moved code.
Does this code leak the twcb allocation if task_work_add() fails?

If the target task is exiting, the callback might never be executed,
permanently leaking memory from the boot-time fixed-size ghes_estatus_pool.

[ ... ]
> +bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
> +			      int sev, bool sync)
> +{
> +	struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
[ ... ]
> +	p = (char *)(err + 1);
> +	length -= sizeof(err);

This isn't a bug introduced by this patch, but pre-exists in the moved code.
Does this incorrectly subtract the size of the pointer err (8 bytes) instead
of the size of the struct it points to?

This leaves length artificially inflated, which might allow the subsequent
bounds check to falsely pass and read past the end of the firmware-provided
payload into adjacent kernel memory.

[ ... ]
> +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);
[ ... ]
> +		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));
> +
> +		aer_recover_queue(pcie_err->device_id.segment,
> +				  pcie_err->device_id.bus,
> +				  devfn, aer_severity,
> +				  (struct aer_capability_regs *)
> +				  aer_info);
> +	}
> +#endif
> +}

This isn't a bug introduced by this patch, but pre-exists in the moved code.
Is it possible for aer_info to be leaked here?

If the internal aer_recover_ring kfifo in aer_recover_queue() is full,
the item is dropped with a log message, but the aer_info memory allocated
from ghes_estatus_pool is never freed. Could this silent leak eventually
deplete the finite error status pool during an AER storm?

[ ... ]
> +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;

Also regarding sharing helpers with other modules, should
ghes_cper_handle_status be exported so that loadable modules can use it
as intended?

> +
> +	sev = ghes_severity(estatus->error_severity);
> +	apei_estatus_for_each_section(estatus, gdata) {
[ ... ]
> +		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);
> +
> +			arch_apei_report_mem_error(sev, mem_err);
> +			queued = ghes_handle_memory_failure(gdata, sev, sync);
> +		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
> +			ghes_handle_aer(gdata);
> +		} else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
> +			queued = ghes_handle_arm_hw_error(gdata, sev, sync);
[ ... ]
> +	}
> +
> +	/*
> +	 * If no memory failure work is queued for abnormal synchronous
> +	 * errors, do a force kill.
> +	 */
> +	if (sync && !queued) {
> +		dev_err(dev,
> +			HW_ERR GHES_PFX "%s:%d: synchronous unrecoverable error (SIGBUS)\n",
> +			current->comm, task_pid_nr(current));
> +		force_sig(SIGBUS);
> +	}
> +}

This isn't a bug introduced by this patch, but pre-exists in the moved code.
Can the queued flag be incorrectly overwritten during later loop iterations?

If an early section contains a synchronous unrecoverable error but fails to
queue recovery, and a later section successfully queues its recovery, the
queued flag might be overwritten to true. Could this bypass the force_sig
fallback at the end of the function, allowing the task to continue executing
despite a fatal memory error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-topics-ahmtib01-ras_ffh_arm_internal_review-v4-0-42698675ba61@arm.com?part=8