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

["Bowman, Terry" <terry.bowman@amd.com>]

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.

Terry 

On 7/17/2026 5:49 PM, sashiko-bot@kernel.org 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 <terry.bowman@amd.com>
> 
> 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);
>     ...
> }
>