Re: [PATCH] drm/xe/i2c: Fix use-after-free on driver unload

["Vivekanandan, Balasubramani" <balasubramani.vivekanandan@intel.com>]

On 12.10.2026 17:16, Kumar G, Naresh wrote:
> Hi Bala,
> 
> I was investigating this issue earlier in sometime and found the root cause
> in software _node_notify_remove().
> 
> It is a kind of ownership bug in driver/base/swnode.c
> sofware_node_notify_remove(). when a manged swnode is mirrored onto a
> 
> second device(which is exactly what i2c-core does for the adapter's own
> struct device), the old code does an extra
> set_secondary_fwnode()/swnode_put()
> 
> for every device that tears down, not just the true owner. this is a
> double-release/UAF risk. The following fix solves the issue without changing
> i2c adapter registration flow.
> 
> The following code patch is suggested/provided to @heikki: waiting for his
> inputs on the same.
> 
> Fixes: 151f6ff78cdf ("software node: Provide replacement for device_add_properties()")
> Assisted-by: GitHub_Copilot:claude-sonnet-5
> Signed-off-by: Nareshkumar Gollakoti<naresh.kumar.g@intel.com>
> ---
>  drivers/base/swnode.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 84a77ffa3dc4..2efcf12b4c21 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -1190,6 +1190,7 @@ void software_node_notify(struct device *dev)
>  void software_node_notify_remove(struct device *dev)
>  {
>  	struct swnode *swnode;
> +	bool owns_managed_fwnode;
>  	swnode = dev_to_swnode(dev);
>  	if (!swnode)
> @@ -1198,6 +1199,16 @@ void software_node_notify_remove(struct device *dev)
>  	sysfs_remove_link(&swnode->kobj, dev_name(dev));
>  	sysfs_remove_link(&dev->kobj, "software_node");
> +	/*
> +	 * A managed node can end up mirrored onto more than one device (e.g.
> +	 * a bus core copying the fwnode onto a child device, as i2c-core does
> +	 * for an adapter's own struct device). Only the device that actually
> +	 * owns the fwnode releases the creation reference and detaches it;
> +	 * every other device only releases the reference software_node_notify()
> +	 * took for it.
> +	 */
> +	owns_managed_fwnode = swnode->managed && swnode->fwnode.dev == dev;
> +
>  	/*
>  	 * Drop the device pointer mirrored onto a secondary software node in
>  	 * software_node_notify(). For a primary software node the core owns
> @@ -1206,12 +1217,13 @@ void software_node_notify_remove(struct device *dev)
>  	if (!device_match_fwnode(dev, &swnode->fwnode) && swnode->fwnode.dev == dev)
>  		swnode->fwnode.dev = NULL;
> +	if (owns_managed_fwnode)
> +		set_secondary_fwnode(dev, NULL);
> +
>  	swnode_put(swnode);
> -	if (swnode->managed) {
> -		set_secondary_fwnode(dev, NULL);
> +	if (owns_managed_fwnode)
>  		swnode_put(swnode);
> -	}
>  }
> 
> 
> Given that,
> 
> Is this rework still needed once the core fix landed?

We don't need this patch if the limitation in sharing the managed
software nodes is fixed.

Regards,
Bala

> 
> 
> Thanks,
> 
> Naresh
> 
> On 10/9/2026 10:12 PM, Balasubramani Vivekanandan wrote:
> > The kernel crashes when unloading the xe driver on CRI:
> > 
> >   Oops: general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b73
> >   RIP: 0010:platform_device_release+0x80/0x90
> >   Call Trace:
> >    device_release+0x44/0xa0
> >    kobject_put+0x9b/0x250
> >    put_device+0x13/0x30
> >    platform_device_unregister+0x29/0x40
> >    xe_i2c_remove+0x80/0xa0 [xe]
> >    devm_action_release+0x16/0x30
> >    release_nodes+0x3d/0x140
> >    devres_release_all+0x96/0xd0
> > 
> > This started after commit 0f485f88656af ("drm/xe/i2c: use
> > device_create_managed_software_node()"), which attaches a managed
> > software node to a platform device created with platform_device_alloc().
> > 
> > Comment above the device_create_managed_software_node function cautions
> > that the software node created by it should not be shared or reused as
> > the lifetime of the node is linked to the device.
> > 
> > But the DesignWare i2c driver which creates an adapter device as a child
> > of the platform device reuses the software node as its fwnode.
> > (device_set_node() in i2c_dw_probe()).
> > 
> > During module unbind, when the adapter device is deleted, it drops the
> > reference count for the software node and frees the software node.
> > Later when the platform device is removed, it tries to dereference the
> > software node pointer and results in use-after-free.
> > 
> > Instead of managed software node use a static struct software_node
> > passed through platform_device_info.swnode to
> > platform_device_register_full().
> > But we have a blocker which doesn't allow to use
> > platform_device_register_full() simply.
> > 
> > Currently the driver is using the two step alloc/add sequence while
> > creating the platform device so that i2c->pdev is set before
> > platform_device_add(), because xe_i2c_notifier() needs i2c->dev and it
> > can run before platform_device_register_full() returns.
> > 
> > We can fix it by removing the dependency on i2c->dev in
> > xe_i2c_notifier(). It is using it to match against the right adapter.
> > Instead of using the device, use its parent to match.
> > 
> > Fixes: 0f485f88656af ("drm/xe/i2c: use device_create_managed_software_node()")
> > Cc: Bartosz Golaszewski<bartosz.golaszewski@oss.qualcomm.com>
> > Cc: Danilo Krummrich<dakr@kernel.org>
> > Cc: Heikki Krogerus<heikki.krogerus@linux.intel.com>
> > Signed-off-by: Balasubramani Vivekanandan<balasubramani.vivekanandan@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_i2c.c | 45 +++++++++++++------------------------
> >   1 file changed, 15 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_i2c.c b/drivers/gpu/drm/xe/xe_i2c.c
> > index b82caaff80d7b..c54a2375f07ef 100644
> > --- a/drivers/gpu/drm/xe/xe_i2c.c
> > +++ b/drivers/gpu/drm/xe/xe_i2c.c
> > @@ -55,6 +55,10 @@ static const struct property_entry xe_i2c_adapter_properties[] = {
> >   	{ }
> >   };
> > +static const struct software_node xe_i2c_adapter_node = {
> > +	.properties = xe_i2c_adapter_properties,
> > +};
> > +
> >   static inline void xe_i2c_read_endpoint(struct xe_mmio *mmio, void *ep)
> >   {
> >   	u32 *val = ep;
> > @@ -98,7 +102,7 @@ static int xe_i2c_notifier(struct notifier_block *nb, unsigned long action, void
> >   	struct device *dev = data;
> >   	if (action == BUS_NOTIFY_ADD_DEVICE &&
> > -	    adapter && dev->parent == &i2c->pdev->dev) {
> > +	    adapter && dev->parent && dev->parent->parent == i2c->drm_dev) {
> >   		i2c->adapter = adapter;
> >   		schedule_work(&i2c->work);
> >   		return NOTIFY_OK;
> > @@ -110,41 +114,22 @@ static int xe_i2c_notifier(struct notifier_block *nb, unsigned long action, void
> >   static int xe_i2c_register_adapter(struct xe_i2c *i2c)
> >   {
> >   	struct pci_dev *pci = to_pci_dev(i2c->drm_dev);
> > +	struct platform_device_info info = {
> > +		.parent = i2c->drm_dev,
> > +		.name = adapter_name,
> > +		.swnode = &xe_i2c_adapter_node,
> > +	};
> >   	struct platform_device *pdev;
> > -	int ret;
> > -	u32 id;
> > -
> > -	id = (pci_domain_nr(pci->bus) << 16) | pci_dev_id(pci);
> > -
> > -	/*
> > -	 * Not using platform_device_register_full() here because we don't have
> > -	 * a handle to the platform_device before it returns. xe_i2c_notifier()
> > -	 * uses that handle, but it may be called before
> > -	 * platform_device_register_full() is done.
> > -	 */
> > -	pdev = platform_device_alloc(adapter_name, id);
> > -	if (!pdev)
> > -		return -ENOMEM;
> > -	ret = device_create_managed_software_node(&pdev->dev,
> > -						  xe_i2c_adapter_properties,
> > -						  NULL);
> > -	if (ret)
> > -		goto err_pdev_put;
> > +	info.id = (pci_domain_nr(pci->bus) << 16) | pci_dev_id(pci);
> > -	pdev->dev.parent = i2c->drm_dev;
> > -	i2c->pdev = pdev;
> > +	pdev = platform_device_register_full(&info);
> > +	if (IS_ERR(pdev))
> > +		return PTR_ERR(pdev);
> > -	ret = platform_device_add(pdev);
> > -	if (ret)
> > -		goto err_pdev_put;
> > +	i2c->pdev = pdev;
> >   	return 0;
> > -
> > -err_pdev_put:
> > -	platform_device_put(pdev);
> > -
> > -	return ret;
> >   }
> >   static void xe_i2c_unregister_adapter(struct xe_i2c *i2c)