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

On Tue, Jul 21, 2026 at 06:04:02AM +0800, [email protected] wrote:
> 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?
>

I'd push back on this.

I think check-then-act observation is fair, but it doesn't affect the patch's
target, which is a provider-locked region that is F_LOCK'd during assembly
before any userspace delete is possible, and F_LOCK is never cleared,
so the flag is always already set when delete_region_store() runs for it.

The window described by the sashiko-bot requires userspace to concurrently
attach an F_LOCK-setting target to the same region.
I don't see a reachable path for that. And it can't be closed with a lock
in delete_region_store() because unregister_region() calls device_del()
before reset.
 
> 
> [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;
> 

Will send another patch fix for this one.

Best regards,
Richard Cheng.

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