Re: [PATCH v9 5/5] gpio: shared: convert to using software nodes for GPIO shared proxy lookup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] gpio: shared: use-after-free in software_node_notify_remove() on proxy removal
- [Medium] gpio: shared: software node leak in gpio_shared_make_proxy_dev() on add error
--

commit 8540cb961814db62ab8d7cec3ac6bee73e8c8520
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

gpio: shared: convert to using software nodes for GPIO shared proxy lookup

This commit replaces platform lookup tables for shared GPIO proxy devices
with managed software nodes attached to the proxy auxiliary devices,
allowing shared GPIO lookup to work via firmware nodes even when no
consumer struct device is present.

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index 9d73a02e99890..57d8fd0bf2e1b 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c
[ ... ]
> @@ -303,56 +303,70 @@ static int gpio_shared_of_scan(void)
>  
>  static void gpio_shared_proxy_dev_release(struct device *dev)
>  {
> -	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +	struct auxiliary_device *proxy = to_auxiliary_dev(dev);
>  
> -	ida_free(&gpio_shared_ida, adev->id);
> -	kfree(adev);
> +	ida_free(&gpio_shared_ida, proxy->id);
> +	kfree(proxy);
>  }
>  
>  static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
>  				      struct gpio_shared_entry *entry,
>  				      struct gpio_shared_ref *ref)
>  {
> -	struct auxiliary_device *adev;
> +	struct property_entry properties = { };
> +	struct auxiliary_device *proxy;
> +	struct device *dev;
>  	int ret, id;
>  
> -	adev = kzalloc_obj(*adev);
> -	if (!adev)
> +	proxy = kzalloc_obj(*proxy);
> +	if (!proxy)
>  		return -ENOMEM;
>  
>  	id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
>  	if (id < 0) {
> -		kfree(adev);
> -		return id;
> +		ret = id;
> +		goto err_free_proxy;
>  	}
>  
> -	adev->id = id;
> -	adev->name = "proxy";
> -	adev->dev.parent = gdev->dev.parent;
> -	adev->dev.platform_data = entry;
> -	adev->dev.release = gpio_shared_proxy_dev_release;
> +	dev = &proxy->dev;
> +	proxy->id = id;
> +	proxy->name = "proxy";
> +	dev->parent = gdev->dev.parent;
> +	dev->platform_data = entry;
> +	dev->release = gpio_shared_proxy_dev_release;
>  
> -	ret = auxiliary_device_init(adev);
> +	ret = auxiliary_device_init(proxy);
> +	if (ret)
> +		goto err_free_ida;
> +
> +	ret = device_create_managed_software_node(dev, &properties, NULL);
>  	if (ret) {
> -		ida_free(&gpio_shared_ida, id);
> -		kfree(adev);
> +		auxiliary_device_uninit(proxy);
>  		return ret;
>  	}

[Severity: Critical]
As reported by Sashiko on v8 [1], can using
device_create_managed_software_node() here cause a use-after-free and
refcount underflow in software_node_notify_remove() when the proxy device
is removed or unbound?

In gpio_shared_make_proxy_dev(), device_create_managed_software_node()
creates a struct swnode with refcount 1, sets swnode->managed = true, and
attaches it to proxy->dev. When auxiliary_device_add() registers
proxy->dev, software_node_notify() increments the swnode refcount to 2.

Later, when gpio_shared_proxy_probe() registers the proxy gpio_chip with
gc->parent = &proxy->dev and gc->fwnode = NULL,
gpiochip_add_data_with_key() propagates the parent's fwnode to the child
gdev->dev:

drivers/gpio/gpiolib.c:gpiochip_add_data_with_key() {
    ...
    gdev->dev.parent = gc->parent;
    device_set_node(&gdev->dev, gpiochip_choose_fwnode(gc));
    ...
}

When gpiochip_setup_dev() calls device_add(&gdev->dev),
software_node_notify(&gdev->dev) increments the same managed swnode's
refcount to 3.

However, managed software nodes cannot be shared across devices because
software_node_notify_remove() drops two references whenever
swnode->managed is true:

drivers/base/swnode.c:software_node_notify_remove() {
    ...
    swnode_put(swnode);

    if (swnode->managed) {
        set_secondary_fwnode(dev, NULL);
        swnode_put(swnode);
    }
}

When gpio_shared_remove_proxy_dev() calls auxiliary_device_delete(proxy),
unbinding gpio-shared-proxy first removes gdev->dev, which runs
software_node_notify_remove(&gdev->dev) and drops the swnode refcount from
3 to 1. Removing proxy->dev then runs
software_node_notify_remove(&proxy->dev), where the first swnode_put()
drops the refcount to 0 and frees swnode, and the subsequent
swnode->managed check and second swnode_put() access freed memory.

>  
> -	ret = auxiliary_device_add(adev);
> +	ret = auxiliary_device_add(proxy);
>  	if (ret) {
> -		auxiliary_device_uninit(adev);
> +		auxiliary_device_uninit(proxy);
>  		return ret;
>  	}

[Severity: Medium]
Does this error path leak the software node if auxiliary_device_add()
fails before device_platform_notify() is reached in device_add()?

Because device_create_managed_software_node() is called before the device
is registered, the struct swnode is allocated and added to swnode_kset
with refcount 1, relying on device_platform_notify_remove() ->
software_node_notify_remove() to release it once device_platform_notify()
has run.

If auxiliary_device_add() fails in dev_set_name() or early in device_add()
before device_platform_notify() is called, software_node_notify_remove()
is never invoked, and auxiliary_device_uninit() ->
gpio_shared_proxy_dev_release() frees proxy without calling
device_remove_software_node(dev).

[1] https://lore.kernel.org/r/20261029-gpio-shared-fwnode-lookup-v8-5-d15d75cd560f@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-gpio-shared-fwnode-lookup-v9-0-a1d6cc501098@oss.qualcomm.com?part=5