Re: [PATCH v3 03/10] ACPI: APEI: GHES: Validate CXL protocol error section length before RAS cap copy

On 7/18/26 12:16 AM, Dave Jiang wrote:
> sashiko-bot flagged an out-of-bounds read driven by an unvalidated
> firmware dvsec_len.
> 
> cxl_cper_setup_prot_err_work_data() locates the RAS Capability block at
> prot_err + sizeof(*prot_err) + dvsec_len and copies it, but dvsec_len is
> firmware controlled and never validated.
> 
> Extend cxl_cper_sec_prot_err_valid() to verify the section can hold the
> header, and that the header, DVSEC and RAS Capability block all fit
> within the reported section length.
> 
> Reported-by: [email protected]
> Link: https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=6
> Link: https://lore.kernel.org/linux-cxl/[email protected]/
> Fixes: 315c2f0b53ba ("acpi/ghes, cper: Recognize and cache CXL Protocol errors")
> Assisted-by: Claude:claude-sonnet-4-6
> Reviewed-by: Ben Cheatham <[email protected]>
> Signed-off-by: Dave Jiang <[email protected]>
> ---
>   drivers/acpi/acpi_extlog.c       |  7 ++++---
>   drivers/acpi/apei/ghes.c         |  7 ++++---
>   drivers/acpi/apei/ghes_helpers.c | 21 ++++++++++++++++++++-
>   include/cxl/event.h              |  4 ++--
>   4 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 7ad3b36013cc..06a944dadbc1 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -165,12 +165,12 @@ static void extlog_print_pcie(struct cper_sec_pcie *pcie_err,
>   
>   static void
>   extlog_cxl_cper_handle_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> -				int severity)
> +				int severity, u32 len)
>   {
>   #ifdef ACPI_APEI_PCIEAER
>   	struct cxl_cper_prot_err_work_data wd;
>   
> -	if (cxl_cper_sec_prot_err_valid(prot_err))
> +	if (cxl_cper_sec_prot_err_valid(prot_err, len))
>   		return;
>   
>   	if (cxl_cper_setup_prot_err_work_data(&wd, prot_err, severity))
> @@ -236,7 +236,8 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>   				acpi_hest_get_payload(gdata);
>   
>   			extlog_cxl_cper_handle_prot_err(prot_err,
> -							gdata->error_severity);
> +							gdata->error_severity,
> +							gdata->error_data_length);
>   		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
>   			struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);
>   
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index a752e152a5a0..17e4ef555292 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -753,12 +753,12 @@ static DEFINE_SPINLOCK(cxl_cper_prot_err_work_lock);
>   struct work_struct *cxl_cper_prot_err_work;
>   
>   static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
> -				   int severity)
> +				   int severity, u32 len)
>   {
>   #ifdef CONFIG_ACPI_APEI_PCIEAER
>   	struct cxl_cper_prot_err_work_data wd;
>   
> -	if (cxl_cper_sec_prot_err_valid(prot_err))
> +	if (cxl_cper_sec_prot_err_valid(prot_err, len))
>   		return;
>   
>   	guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);
> @@ -950,7 +950,8 @@ static void ghes_do_proc(struct ghes *ghes,
>   		} else if (guid_equal(sec_type, &CPER_SEC_CXL_PROT_ERR)) {
>   			struct cxl_cper_sec_prot_err *prot_err = acpi_hest_get_payload(gdata);
>   
> -			cxl_cper_post_prot_err(prot_err, gdata->error_severity);
> +			cxl_cper_post_prot_err(prot_err, gdata->error_severity,
> +					       gdata->error_data_length);
>   		} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
>   			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
>   
> diff --git a/drivers/acpi/apei/ghes_helpers.c b/drivers/acpi/apei/ghes_helpers.c
> index bc7111b740af..d625ec98a24c 100644
> --- a/drivers/acpi/apei/ghes_helpers.c
> +++ b/drivers/acpi/apei/ghes_helpers.c
> @@ -5,8 +5,15 @@
>   #include <linux/aer.h>
>   #include <cxl/event.h>
>   
> -int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err)
> +int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err, u32 len)
>   {
> +	if (len < sizeof(*prot_err)) {
> +		pr_err_ratelimited(FW_WARN
> +				   "CXL CPER prot err section too small (%u)\n",
> +				   len);
> +		return -EINVAL;
> +	}
> +
>   	if (!(prot_err->valid_bits & PROT_ERR_VALID_AGENT_ADDRESS)) {
>   		pr_err_ratelimited("CXL CPER invalid agent type\n");
>   		return -EINVAL;
> @@ -23,6 +30,18 @@ int cxl_cper_sec_prot_err_valid(struct cxl_cper_sec_prot_err *prot_err)
>   		return -EINVAL;
>   	}
>   
> +	/*
> +	 * The RAS Capability block follows a firmware-controlled DVSEC of
> +	 * dvsec_len bytes; verify it and the header fit the section.
> +	 */
> +	if (sizeof(*prot_err) + prot_err->dvsec_len +
> +	    sizeof(struct cxl_ras_capability_regs) > len) {
> +		pr_err_ratelimited(FW_WARN
> +				   "CXL CPER prot err section too small (%u)\n",
> +				   len);

Nit, this prints the same message as the header-too-small case above, so
the log can't tell them apart. Maybe include dvsec_len here.

Reviewed-by: Shuai Xue <[email protected]>

Thanks.
Shuai