Re: [PATCH v5 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:

New issues:
- [Low] The `generic` parameter in `ghes_cper_handle_status()` is completely unused.
- [Low] Internal helper functions are unnecessarily exposed in the global `<acpi/ghes_cper.h>` header.

Pre-existing issues:
- [High] The `queued` boolean is unconditionally overwritten during iteration over multiple error sections, potentially masking a successful recovery queueing and causing an unintended SIGBUS.
- [High] Incorrect pointer math using `sizeof(err)` where `err` is a pointer defeats bounds checking, allowing out-of-bounds reads.
- [High] The `twcb->pfn` field is accessed after its containing memory chunk has been freed, causing a Use-After-Free.
- [High] The global `ghes_estatus_pool` pointer is left dangling if its initialization fails, leading to a Use-After-Free.
--

commit 286897690a5dfe45676a004c4969ff7b921f6c22
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;
> +
> +	len = GHES_ESTATUS_CACHE_AVG_SIZE * GHES_ESTATUS_CACHE_ALLOCED_MAX;
> +	len += (num_ghes * GHES_ESOURCE_PREALLOC_MAX_SIZE);
> +
> +	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
> +	if (!addr)
> +		goto err_pool_alloc;
> +
> +	rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
> +	if (rc)
> +		goto err_pool_add;
> +
> +	return 0;
> +
> +err_pool_add:
> +	vfree((void *)addr);
> +
> +err_pool_alloc:
> +	gen_pool_destroy(ghes_estatus_pool);
> +
> +	return -ENOMEM;
> +}

[Severity: High]
This is a pre-existing issue, but does this leave ghes_estatus_pool as a
dangling pointer?

If vmalloc or gen_pool_add fails, gen_pool_destroy is called, but
ghes_estatus_pool is not set to NULL. If other code checks for
ghes_estatus_pool to avoid double initialization, it might use the
destroyed pool.

[ ... ]

> +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 isn't a bug introduced by this patch, but is it safe to access
twcb->pfn after twcb has been freed via gen_pool_free?

If the memory chunk is reallocated and overwritten by another thread
before the pr_err call, this could log corrupted data.

[ ... ]

> +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);
> +	int flags = sync ? MF_ACTION_REQUIRED : 0;
> +	int length = gdata->error_data_length;
> +	char error_type[120];
> +	bool queued = false;
> +	int sec_sev, i;
> +	char *p;

[ ... ]

> +	p = (char *)(err + 1);
> +	length -= sizeof(err);

[Severity: High]
This is a pre-existing issue, but does length -= sizeof(err) subtract the
correct amount here?

Since err is a pointer, sizeof(err) evaluates to the pointer size (8 bytes)
rather than the size of the underlying struct cper_sec_proc_arm. This
might artificially inflate the remaining length and bypass the bounds
checks in the loop below.

> +
> +	for (i = 0; i < err->err_info_num; i++) {
> +		struct cper_arm_err_info *err_info;
> +		bool is_cache, has_pa;
> +
> +		/* Ensure we have enough data for the error info header */
> +		if (length < sizeof(*err_info))
> +			break;

[ ... ]

> +		if (is_cache && has_pa) {
> +			queued = ghes_do_memory_failure(err_info->physical_fault_addr, flags);

[Severity: High]
This is a pre-existing issue, but does assigning directly to queued
overwrite successful recovery queueings?

If an earlier iteration successfully sets queued to true, but a later one
fails and sets it to false, the function might incorrectly return false and
cause an unintended SIGBUS. Would a logical OR (queued |= ...) be more
appropriate here?

[ ... ]

> +void ghes_cper_handle_status(struct device *dev,
> +			     const struct acpi_hest_generic *generic,

[Severity: Low]
Is the generic parameter used anywhere in this function body? It looks like
it could be removed to improve clarity.

> +			     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;

[ ... ]

> +		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);

[Severity: High]
This is a pre-existing issue, but similar to the loop above, does assigning
directly to queued overwrite successful queueings from earlier sections?

[ ... ]

> diff --git a/include/acpi/ghes_cper.h b/include/acpi/ghes_cper.h
> index dd49e9179b63a..511b95b509111 100644
> --- a/include/acpi/ghes_cper.h
> +++ b/include/acpi/ghes_cper.h

[ ... ]

> @@ -107,6 +110,23 @@ void ghes_estatus_cache_add(struct acpi_hest_generic *generic,
>  			    struct acpi_hest_generic_status *estatus);
>  void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
>  				   int sev);
> +int ghes_severity(int severity);
> +bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> +				int sev, bool sync);
> +bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
> +			      int sev, bool sync);
> +void ghes_handle_aer(struct acpi_hest_generic_data *gdata);

[Severity: Low]
Do these helper functions need to be exposed in the global header?

Since they are only invoked locally by ghes_cper_handle_status, could they
be marked static in ghes_cper.c to prevent namespace pollution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-topics-ahmtib01-ras_ffh_arm_internal_review-v5-0-2e0500d42642@arm.com?part=8