Re: [PATCH v2] cxl/port: Fix use after free of parent_port in cxl_detach_ep()

[<dan.j.williams@intel.com>]

Alison Schofield wrote:
> cxl_detach_ep() is called during bottom-up removal when all CXL memory
> devices beneath a switch port have been removed. For each port in the
> hierarchy it locks both the port and its parent, removes the endpoint,
> and if the port is now empty, marks it dead and unregisters the port
> by calling delete_switch_port(). There are two places during this work
> where the parent_port may be used after freeing:
> 
> First, a concurrent detach may have already processed a port by the
> time a second worker finds it via bus_find_device(). Without pinning
> parent_port, it may already be freed when we discover port->dead and
> attempt to unlock the parent_port. In a production kernel that's a
> silent memory corruption, with lock debug, it looks like this:
> 
> []DEBUG_LOCKS_WARN_ON(__owner_task(owner) != get_current())
> []WARNING: kernel/locking/mutex.c:949 at __mutex_unlock_slowpath+0x1ee/0x310
> []Call Trace:
> []mutex_unlock+0xd/0x20
> []cxl_detach_ep+0x180/0x400 [cxl_core]
> []devm_action_release+0x10/0x20
> []devres_release_all+0xa8/0xe0
> []device_unbind_cleanup+0xd/0xa0
> []really_probe+0x1a6/0x3e0
> 
> Fix this first case by adding a check for port->dead after acquiring
> both locks. Unlock and release the parent reference before continuing.
> 
> Second, delete_switch_port() releases three devm actions registered
> against parent_port. The last of those is unregister_port() and it
> calls device_unregister() on the child port, which can cascade. If
> parent_port is now also empty the device core may unregister and free
> it too. So by the time delete_switch_port() returns, parent_port may
> be free, and the subsequent device_unlock(&parent_port->dev) operates
> on freed memory. The kernel log looks same as above, with a different
> offset in cxl_detach_ep().
> 
> Fix this second issue by taking an extra reference on parent_port
> before locking it, preventing the memory from being freed across
> delete_switch_port(). Release it after device_unlock().
> 
> These easily reproduce with a reload of cxl_acpi in QEMU environment
> with CXL devices present.

The analysis looks correct, but I worry this fix is too tightly scoped
to this one problematic path.  Just make the rule be that parent ports
have an elevated reference until child release rather than child
unregister.

This would be a similar semantic to what happens for regions where
cxl_region_alloc() takes a reference on the root decoder, and
cxl_region_release() drops it.