Re: [PATCH] cxl/region: Reject delete of a provider-locked region

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

New issues:
- [High] A Time-Of-Check to Time-Of-Use (TOCTOU) race condition in `delete_region_store()` allows userspace to bypass the `CXL_REGION_F_LOCK` protection, leading to orphaned hardware decoders.

Pre-existing issues:
- [High] Userspace can bypass `CXL_REGION_F_LOCK` restrictions and tear down region software state by manually detaching targets via `targetN` sysfs attributes, leading to orphaned hardware decoders.
--

--- Patch [1]: [PATCH] cxl/region: Reject delete of a provider-locked region ---
commit 447746e2d3844401b6bdd5604c498e319f1e7719
Author: Richard Cheng <[email protected]>

cxl/region: Reject delete of a provider-locked region
    
delete_region_store() destroys a region object without checking
CXL_REGION_F_LOCK, unlike commit_store() which refuses locked regions
with -EPERM. This patch gates delete_region_store() on CXL_REGION_F_LOCK.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 578622240401d..cfc9622c93095 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2894,6 +2894,9 @@ static ssize_t delete_region_store(struct device *dev,
>  	if (!cxlr || !sysfs_streq(buf, dev_name(&cxlr->dev)))
>  		return -ENODEV;
>  
> +	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
> +		return -EPERM;
> +

[Severity: High]
Is there a Time-Of-Check to Time-Of-Use (TOCTOU) race condition here?

The CXL_REGION_F_LOCK check is performed without holding synchronization
like the region rwsem, leaving a window open before unregister_region()
completes.

If userspace writes to delete_region while concurrently writing to targetN
to attach a target, delete_region_store() could check the lock flag before
it is set.

Then unregister_region() calling device_del() can block in kernfs_drain
until the concurrent target attachment finishes and sets CXL_REGION_F_LOCK.

Once resumed, the software region would be destroyed with the lock flag
set, skipping the hardware reset. Could this sequence recreate the exact
orphaned hardware decoder state this patch intended to prevent?


[Severity: High]
This is a pre-existing issue, but does a similar bypass exist in
store_targetN() when userspace manually detaches targets?

If a root user writes \n to a targetN sysfs attribute of a locked region,
store_targetN() calls detach_target() which eventually reaches
__cxl_decoder_detach(). This software teardown path lacks an F_LOCK check.

Because cxl_region_decode_reset() deliberately skips the hardware reset
for F_LOCK regions, the software state is dropped while the hardware
decoders remain committed.

The user could then write 0 to the size attribute, freeing the physical
memory while it is still actively decoded by hardware.

Should store_targetN() or __cxl_decoder_detach() also be gated by
CXL_REGION_F_LOCK to prevent userspace from tearing down the software
state of locked regions?

>  	unregister_region(cxlr);
>  
>  	return len;

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