Re: [PATCH v7 10/10] RAS: add firmware-first CPER provider

[sashiko-bot@kernel.org]

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

New issues:
- [High] The ack register is mapped as cacheable memory (MEMREMAP_WB) without any cache flushing, which may prevent non-cache-coherent firmware from ever seeing the ack bit.
- [High] Fatal hardware errors (GHES_SEV_PANIC) are processed asynchronously in a threaded IRQ handler, allowing the system to continue running and potentially consume corrupted data before panicking.
- [High] Missing acquire memory barrier before reading the CPER status block allows reading a corrupted/torn error record payload.

Pre-existing issues:
- [High] A missing remaining length check in the CPER sections loop leads to a heap out-of-bounds read when parsing.
- [High] An integer overflow in cper_estatus_len() bypasses length checks, returning a dangerously small size.
--

--- Patch [10]: [PATCH v7 10/10] RAS: add firmware-first CPER provider ---
commit 32e67d0a44c9a2a1207aa498bd207d1d6f803056
Author: Ahmed Tiba <ahmed.tiba@arm.com>

RAS: add firmware-first CPER provider

Add a firmware-first CPER provider that reuses the shared
GHES helpers, wire it into the RAS Kconfig/Makefile and
document it in the admin guide.

> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index 77c10a7a7a9f4..15d11f10d0671 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -123,8 +123,7 @@ struct dentry *apei_get_debugfs_dir(void);
>  static inline u32 cper_estatus_len(struct acpi_hest_generic_status *estatus)
>  {
>  	if (estatus->raw_data_length)
> -		return estatus->raw_data_offset + \
> -			estatus->raw_data_length;
> +		return estatus->raw_data_offset + estatus->raw_data_length;

[Severity: High]
This is a pre-existing issue, but can the addition of raw_data_offset and
raw_data_length overflow?

If a malformed CPER record has values that sum to a 32-bit wrap-around
(e.g., 0xFFFFFFF0 + 0x24 = 20), this could return a dangerously small size,
potentially leading to undersized cache allocations and downstream
out-of-bounds accesses.

>  	else
>  		return sizeof(*estatus) + estatus->data_length;
>  }

[ ... ]

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 06b4fdb59917a..a78e7d17a1b9f 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -742,7 +742,8 @@ int cper_estatus_check_header(const struct acpi_hest_generic_status *estatus)
>  	    estatus->data_length < sizeof(struct acpi_hest_generic_data))
>  		return -EINVAL;
>  	if (estatus->raw_data_length &&
> -	    estatus->raw_data_offset < sizeof(*estatus) + estatus->data_length)
> +	    (estatus->raw_data_offset < sizeof(*estatus) ||
> +	     estatus->raw_data_offset - sizeof(*estatus) < estatus->data_length))
>  		return -EINVAL;
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does cper_estatus_check() (which calls
this header check) have a heap out-of-bounds read if a malformed CPER record
has a valid total data_length, but the final section leaves fewer than
sizeof(*gdata) bytes remaining?

If the loop in apei_estatus_for_each_section() executes when the remaining
length is very small, acpi_hest_get_size() will dereference gdata->revision
reading past the end of the valid data buffer.

[ ... ]

> diff --git a/drivers/ras/cper-esource.c b/drivers/ras/cper-esource.c
> new file mode 100644
> index 0000000000000..3f60c9849d9a6
> --- /dev/null
> +++ b/drivers/ras/cper-esource.c
> @@ -0,0 +1,332 @@

[ ... ]

> +static void *cper_esource_map_region(struct device *dev, unsigned int index,
> +				     size_t *size)
> +{
> +	struct resource res;
> +	void *addr;
> +
> +	if (of_reserved_mem_region_to_resource(dev->of_node, index, &res))
> +		return ERR_PTR(dev_err_probe(dev, -EINVAL,
> +					     "unable to resolve memory-region %u\n",
> +					     index));
> +
> +	*size = resource_size(&res);
> +	if (!*size)
> +		return ERR_PTR(dev_err_probe(dev, -EINVAL,
> +					     "memory-region %u has zero length\n",
> +					     index));
> +
> +	addr = devm_memremap(dev, res.start, *size, MEMREMAP_WB);

[Severity: High]
Will the firmware be able to observe the ack bit being set in the mapped
ack region?

The region is mapped as cacheable memory (MEMREMAP_WB). Later,
cper_esource_ack() uses wmb() to publish the update, but wmb() only enforces
memory ordering and does not flush the CPU data cache to main memory. 

Unless the firmware is fully cache-coherent with the OS, the updated ack bit
might remain in the CPU cache, potentially causing the firmware to wait
indefinitely.

Could we map the ack register as uncached (e.g., MEMREMAP_WT, MEMREMAP_WC,
or devm_ioremap), or perform an explicit cache clean operation?

[ ... ]

> +static void cper_esource_ack(struct cper_esource *ctx)
> +{
> +	if (!ctx->ack.present)
> +		return;
> +
> +	if (ctx->ack.width == 64) {
> +		u64 *addr = ctx->ack.addr;
> +		u64 val = READ_ONCE(*addr);
> +
> +		/* Publish status-buffer updates before raising the ack bit. */
> +		wmb();
> +		val &= ctx->ack.preserve;
> +		val |= ctx->ack.set;
> +		WRITE_ONCE(*addr, val);

[Severity: High]
As mentioned above, wmb() does not clean the cache line to main memory.
Could this leave the updated ack bit trapped in the CPU cache?

[ ... ]

> +static irqreturn_t cper_esource_process(struct cper_esource *ctx)
> +{
> +	int rc;
> +	int sev;
> +
> +	memcpy(ctx->estatus, ctx->status, ctx->status_len);

[Severity: High]
Does copying the entire status block before checking the block_status flag
allow reading a corrupted or torn error record payload?

Because the firmware buffer is mapped as cacheable memory, the CPU can reorder
the loads. If the CPU races with the firmware updating the record, it might
load stale payload bytes out of order before observing the new
block_status == 1.

Should we read block_status using smp_load_acquire(), check if it is non-zero,
and only then read the rest of the payload?

> +
> +	rc = cper_esource_validate_status(ctx);
> +	if (rc == -ENOENT)
> +		return IRQ_NONE;
> +	if (rc) {
> +		dev_warn_ratelimited(ctx->dev, FW_WARN GHES_PFX
> +				     "Invalid error status block\n");
> +		cper_esource_clear_status(ctx);
> +		cper_esource_ack(ctx);
> +		return IRQ_HANDLED;
> +	}
> +
> +	sev = ghes_severity(ctx->estatus->error_severity);
> +	if (sev >= GHES_SEV_PANIC)
> +		cper_esource_fatal(ctx);

[Severity: High]
Is it safe to process fatal hardware errors (GHES_SEV_PANIC) asynchronously in
a threaded IRQ handler?

If the system continues running, could the CPU consume poisoned memory before
the threaded handler executes and panics?

[ ... ]

> +static int cper_esource_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cper_esource *ctx;
> +	size_t size;
> +	int source_id;
> +	int rc;

[ ... ]

> +	rc = devm_request_threaded_irq(dev, ctx->irq, NULL, cper_esource_irq,
> +				       IRQF_ONESHOT,
> +				       dev_name(dev), ctx);

[Severity: High]
Because the IRQ is requested without a hardirq primary handler, the default
primary handler masks the interrupt and wakes the thread, allowing the
currently executing task to resume.

For fatal RAS errors, shouldn't the system panic immediately in a hardirq
(or NMI) handler to ensure containment guarantees?

> +	if (rc)
> +		return dev_err_probe(dev, rc, "failed to request interrupt\n");
> +
> +	return 0;
> +}

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