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

On Mon, 20 Jul 2026 10:02:05 -0500
"Bowman, Terry" <[email protected]> wrote:

> The LOW issue is valid and the extra cancel_work_sync() needs to be 
> removed. I propose we fix if a series respin is necessary or otherwise 
> fix in future. 
> 
> The preexisting HIGH issues are GHES area and to be fixed by owners.

Mauro, this smells like one that your nice QEMU + scripts could detect.
Fancy it as a test case? (he says hoping you'll also fix it :)

Also, I just flagged on discord that we should discuss what we do about
these sort of issues and tracking them at the CXL sync call tomorrow.
(Tracking somewhat discussion here: 
https://lore.kernel.org/ksummit/[email protected]/T/#t

Jonathan

> 
> Terry 
> 
> On 7/17/2026 5:49 PM, [email protected] wrote:
> > 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);
> >     ...
> > }
> >   
>