Re: [PATCH v18 02/13] acpi/apei/ghes: Use raw_spinlock_t for CXL CPER work locks

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

New issues:
- [Low] The patch adds internal cancellation to both unregister functions but misses removing the redundant `cancel_work_sync()` for the protocol error work in `cxl_ras_exit()`.

Pre-existing issues:
- [High] Fixed-size memcpy in cxl_cper_post_event causes an out-of-bounds read if the provided GHES CPER payload is smaller than sizeof(*rec).
- [High] Missing bounds check on untrusted dvsec_len in cxl_cper_setup_prot_err_work_data leads to an out-of-bounds read.
--

--- Patch [2]: [PATCH v18 02/13] acpi/apei/ghes: Use raw_spinlock_t for CXL CPER work locks ---
commit 70fa064ce36bd35723148aab9a91c80d0fbef760
Author: Terry Bowman <[email protected]>

acpi/apei/ghes: Use raw_spinlock_t for CXL CPER work locks

This commit converts cxl_cper_work_lock and cxl_cper_prot_err_work_lock from
spinlock_t to raw_spinlock_t to prevent a deadlock between IRQ and process
contexts. It also restructures registration functions to warn on double
registration and clear work pointers before cancelling them. Redundant
cancel_work_sync() calls are removed since quiescing is handled internally.

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3236a3ce79d6b..ca7a138c1ff2e 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
[ ... ]
> @@ -761,7 +761,7 @@ static void cxl_cper_post_prot_err(struct cxl_cper_sec_prot_err *prot_err,
>  	if (cxl_cper_sec_prot_err_valid(prot_err))
>  		return;
>  
> -	guard(spinlock_irqsave)(&cxl_cper_prot_err_work_lock);
> +	guard(raw_spinlock_irqsave)(&cxl_cper_prot_err_work_lock);
>  
>  	if (!cxl_cper_prot_err_work)
>  		return;

[Severity: High]
This is a pre-existing issue, but does cxl_cper_setup_prot_err_work_data()
lack bounds checking on the untrusted dvsec_len?

If firmware provides a CXL protocol error record with a maliciously large
dvsec_len, could this lead to an out-of-bounds read when calculating
cap_start?

drivers/acpi/apei/ghes_helpers.c:cxl_cper_setup_prot_err_work_data() {
    ...
    dvsec_start = (u8 *)(prot_err + 1);
    cap_start = dvsec_start + prot_err->dvsec_len;

    memcpy(&wd->ras_cap, cap_start, sizeof(wd->ras_cap));
    ...
}

[ ... ]
> @@ -831,7 +839,7 @@ static void cxl_cper_post_event(enum cxl_event_type event_type,
>  		return;
>  	}
>  
> -	guard(spinlock_irqsave)(&cxl_cper_work_lock);
> +	guard(raw_spinlock_irqsave)(&cxl_cper_work_lock);
>  
>  	if (!cxl_cper_work)
>  		return;

[Severity: High]
This is a pre-existing issue, but does cxl_cper_post_event() have an
out-of-bounds read if the provided GHES CPER payload is smaller than
sizeof(*rec)?

If firmware provides a CXL event record where gdata->error_data_length is
smaller than sizeof(struct cxl_cper_event_rec), this fixed-size memcpy
could exceed the bounds of the error status block:

drivers/acpi/apei/ghes.c:cxl_cper_post_event() {
    ...
    wd.event_type = event_type;
    memcpy(&wd.rec, rec, sizeof(wd.rec));
    ...
}

[ ... ]
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 267c679b0b3c2..7c6faee7f85ed 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1083,7 +1083,6 @@ static int __init cxl_pci_driver_init(void)
>  static void __exit cxl_pci_driver_exit(void)
>  {
>  	cxl_cper_unregister_work(&cxl_cper_work);
> -	cancel_work_sync(&cxl_cper_work);
>  	pci_unregister_driver(&cxl_pci_driver);
>  }

[Severity: Low]
Is there a missed redundant cancel_work_sync() call in cxl_ras_exit()
that should also be removed?

Since internal cancellation was added to the unregister functions,
cxl_ras_exit() now has redundant synchronization:

drivers/cxl/core/ras.c:cxl_ras_exit() {
    ...
    cxl_cper_unregister_prot_err_work(&cxl_cper_prot_err_work);
    cancel_work_sync(&cxl_cper_prot_err_work);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2