Re: [PATCH] driver core: hand off fwnode ownership when shared fwnode owner is rejected

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] driver core: UAF due to race transferring fwnode to tearing-down sibling
- [High] driver core: UAF/NULL dereference due to lockless fwnode->dev access
- [Medium] driver core: race between probe failure and sibling binding orphans fwnode
--

commit a697546921283509d4dd33e97eef3ff8dcaff551
Author: Peng Fan <peng.fan@nxp.com>

driver core: hand off fwnode ownership when shared fwnode owner is rejected

This commit addresses boot failures when a shared fwnode owner fails to
probe, leading to unresolved consumer links. It introduces logic to transfer
fwnode ownership to a bound sibling when the original owner is rejected.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
[ ... ]
> @@ -1376,6 +1376,20 @@ void device_links_driver_bound(struct device *dev)
>  	struct device_link *link, *ln;
>  	LIST_HEAD(sync_list);
>  
> +	/*
> +	 * Multiple devices may share the same fwnode (e.g. the SCMI bus
> +	 * creates several devices per protocol node).  The first one
> +	 * registered owns the fwnode; if that owner's driver is rejected
> +	 * it relinquishes ownership (fwnode->dev == NULL, see
> +	 * fw_devlink_release_shared_fwnode()).  If the owning device has
> +	 * no driver at all (driver never compiled / loaded as module), it
> +	 * will never bind, so it is safe to take over.  Re-acquire the
> +	 * fwnode here so the purge/pickup still happens.
> +	 */
> +	if (dev->fwnode && (!dev->fwnode->dev ||
> +	    (dev->fwnode->dev != dev && !dev->fwnode->dev->driver)))

[Severity: High]
Is it safe to dereference the dev->fwnode->dev->driver pointer locklessly
here?

If the owning device is concurrently unregistered via device_del() and cleared
from fwnode->dev, the compiler might reload the weak pointer as NULL before
dereferencing ->driver, leading to a NULL pointer dereference.

Alternatively, it could dereference ->driver on a pointer to a newly freed
device object, resulting in a use-after-free.

[Severity: Medium]
Could this condition incorrectly rely on ->driver to deduce if the owner is
still active, missing the window where the owner is tearing down but hasn't
cleared the pointer yet?

If Sibling 1 (the fwnode owner) fails probe, it calls
fw_devlink_release_shared_fwnode(). However, dev->driver is still set at this
stage.

If Sibling 2 finishes probing and evaluates this condition concurrently,
it will see Sibling 1 as the owner with a populated ->driver pointer and skip
claiming ownership. Sibling 1 then clears the pointer to NULL. Neither sibling
performs the required purge and pickup routines, leaving consumers to defer
indefinitely.

> +		dev->fwnode->dev = dev;
> +
[ ... ]
> @@ -1474,6 +1489,54 @@ void device_links_driver_bound(struct device *dev)
>  	device_links_flush_sync_list(&sync_list, dev);
>  }
>  
> +static int fwnode_shared_bound_match(struct device *dev, const void *data)
[ ... ]
> +void fw_devlink_release_shared_fwnode(struct device *dev)
> +{
> +	struct device *sibling;
> +
> +	if (!dev->fwnode || dev->fwnode->dev != dev)
> +		return;
> +
> +	sibling = bus_find_device(dev->bus, NULL, dev, fwnode_shared_bound_match);
> +	if (sibling) {
> +		dev->fwnode->dev = sibling;

[Severity: High]
Can this assignment result in a use-after-free of the sibling device pointer?

This assigns dev->fwnode->dev to the matched sibling locklessly. If the
sibling is concurrently being unregistered, its device_del() routine will run
this check early in the teardown:

if (dev->fwnode && dev->fwnode->dev == dev)
    dev->fwnode->dev = NULL;

If the ownership is transferred to the sibling right after device_del() passes
this check, the fwnode->dev pointer will never be cleared when the sibling is
subsequently freed, leaving a dangling pointer that can be accessed later.

> +		fwnode_links_purge_suppliers(dev->fwnode);
> +		fw_devlink_pickup_dangling_consumers(sibling);
> +		put_device(sibling);
> +	} else {
> +		dev->fwnode->dev = NULL;
> +	}
> +}
> +
>  /**
>   * __device_links_no_driver - Update links of a device without a driver.
>   * @dev: Device without a drvier.
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
[ ... ]
> @@ -762,6 +762,15 @@ static int really_probe(struct device *dev, const struct device_driver *drv)
>  dev_groups_failed:
>  	device_remove(dev);
>  probe_failed:
> +	/*
> +	 * -ENODEV/-ENXIO mean the driver definitively rejected this device
> +	 * (errors are returned as positive values at this label). If it owns a
> +	 * fwnode shared with sibling devices (e.g. SCMI creates several devices
> +	 * per protocol node), hand the fwnode off so its dangling child
> +	 * consumers can be resolved by a sibling instead of deferring forever.
> +	 */
> +	if (ret == ENODEV || ret == ENXIO)
> +		fw_devlink_release_shared_fwnode(dev);

[Severity: Medium]
Because this handoff is initiated while dev->driver is still set (it gets
cleared much later in device_unbind_cleanup()), does this create the race
window with device_links_driver_bound() described earlier?

A successfully probing sibling running device_links_driver_bound() will see
the populated ->driver pointer and incorrectly assume the current owner is
still active, missing the handoff.

>  	driver_sysfs_remove(dev);
>  sysfs_failed:
>  	bus_notify(dev, BUS_NOTIFY_DRIVER_NOT_BOUND);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-driver-core-v1-1-0846bb8e0f32@nxp.com?part=1