Re: [PATCH v4 4/4] 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 6 potential issue(s) to consider:
- [High] gpio: use-after-free of dangling proxy_fwnode on proxy creation failure
- [High] gpio: missing NULL check in gpio_device_teardown_shared causes panic
- [High] gpio: lockless proxy_fwnode teardown causes data race and incorrect chip lookup
- [Medium] gpio: missing proxy device cleanup on error causes permanent -EEXIST probe failure
- [Medium] gpio: missing error code in gpio_shared_make_proxy_dev causes silent failure
- [Medium] gpio: lockless proxy_fwnode initialization causes data race in proxy lookup
--

commit 5c7699dca403f7ae958b2532cf7a05f31e80a27e
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

gpio: shared: convert to using software nodes for GPIO shared proxy lookup
    
Automated management of shared GPIOs only allows to request shared pins
when they're assigned to a real struct device. Firmware node, no-device
lookup currently isn't supported and can't be because we're using
platform lookup tables to point the callers to the shared proxy GPIO
controllers.

This commit converts the GPIO shared proxy lookup mechanism to use
software nodes instead of platform lookup tables. It assigns dummy
software nodes to proxy devices and stores them in reference structures.

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index a57a2dd5ca00e..16c0a9d491167 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c
> @@ -313,37 +314,55 @@ static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
>  				      struct gpio_shared_entry *entry,
>  				      struct gpio_shared_ref *ref)
>  {
> +	struct software_node *swnode = &ref->proxy_swnode;
>  	struct auxiliary_device *adev;
> +	struct device *dev;
>  	int ret, id;
>  
>  	adev = kzalloc_obj(*adev);
>  	if (!adev)
>  		return -ENOMEM;
>  
> +
>  	id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
>  	if (id < 0) {
>  		kfree(adev);
>  		return id;
>  	}
>  
> +	ret = software_node_register(swnode);
> +	if (ret) {
> +		ida_free(&gpio_shared_ida, id);
> +		kfree(adev);
> +		return ret;
> +	}
> +
> +	ref->proxy_fwnode = software_node_fwnode(swnode);

[Severity: Medium]
Is ref->proxy_fwnode protected by a lock here?

If a concurrent consumer calls gpiod_get() and invokes
gpio_shared_get_proxy(), it will acquire ref->lock to safely read
ref->proxy_fwnode. Because the writer ignores the lock, the reader may
observe a partially initialized or stale pointer, leading to a race condition
during fwnode lookup.

> +	if (!ref->proxy_fwnode) {
> +		ida_free(&gpio_shared_ida, id);
> +		kfree(adev);
> +		goto err_swnode_unreg;

[Severity: Medium]
Will this silently return 0 (success) because ret is not set to a negative
error code before jumping to err_swnode_unreg?

Because ret is 0 from the previous successful call to
software_node_register(), this masks the initialization failure, leaving
the proxy device uninitialized but tricking the caller into proceeding as
if it succeeded.

[ ... ]

>  	ret = auxiliary_device_init(adev);
>  	if (ret) {
>  		ida_free(&gpio_shared_ida, id);
>  		kfree(adev);
> -		return ret;
> +		goto err_swnode_unreg;
>  	}
>  
>  	ret = auxiliary_device_add(adev);
> -	if (ret) {
> -		auxiliary_device_uninit(adev);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_dev_uninit;
>  
>  	pr_debug("Created an auxiliary GPIO proxy %s for GPIO device %s\n",
>  		 dev_name(&adev->dev), gpio_device_get_label(gdev));
> @@ -353,6 +372,13 @@ static int gpio_shared_make_proxy_dev(struct gpio_device *gdev,
>  	}
>  
>  	return 0;
> +
> +err_dev_uninit:
> +	auxiliary_device_uninit(adev);
> +err_swnode_unreg:
> +	software_node_unregister(swnode);
> +
> +	return ret;

[Severity: High]
Does this error path leave ref->proxy_fwnode dangling?

If auxiliary_device_init() or auxiliary_device_add() fails, the code jumps
to err_swnode_unreg, which unregisters and frees the swnode. However,
ref->proxy_fwnode is not reset to NULL.

Since ref is part of the global gpio_shared_list, a concurrent or
subsequent consumer calling gpiod_get() will trigger
gpio_shared_get_proxy(), which will dereference the freed proxy_fwnode
pointer, leading to a Use-After-Free.

[Severity: Medium]
Can returning an error here cause a permanent probe failure for the controller?

In gpiochip_setup_shared(), if gpio_shared_make_proxy_dev() fails for one
entry, it immediately returns the error without cleaning up the proxy
devices and software nodes successfully created in previous iterations:

	ret = gpio_shared_make_proxy_dev(gdev, entry, ref);
	if (ret) {
		gpiod_free_commit(desc);
		return ret;
	}

The caller (gpiochip_add_data_with_key()) will then bypass
gpio_device_teardown_shared(). The software_node remains registered, and
when the GPIO controller is reprobed (e.g. after -EPROBE_DEFER),
software_node_register() will be called again with the exact same pointer,
returning -EEXIST.

>  }
>  
>  #if IS_ENABLED(CONFIG_RESET_GPIO)

[ ... ]

> @@ -628,15 +623,11 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
>  			gpiod_free_commit(&gdev->descs[entry->offset]);
>  
>  		list_for_each_entry(ref, &entry->refs, list) {
> -			if (ref->lookup) {
> -				gpiod_remove_lookup_table(ref->lookup);
> -				kfree(ref->lookup->table[0].key);
> -				kfree(ref->lookup->dev_id);
> -				kfree(ref->lookup);
> -				ref->lookup = NULL;
> -			}
> -
> -			gpio_shared_remove_proxy_dev(ref);
> +			auxiliary_device_delete(ref->proxy_dev);
> +			auxiliary_device_uninit(ref->proxy_dev);

[Severity: High]
Might ref->proxy_dev be NULL here?

In gpiochip_setup_shared(), if an entry has only 1 consumer
(list_count_nodes <= 1), proxy creation is skipped, leaving ref->proxy_dev
as NULL. Additionally, silent initialization failures also leave it NULL.

Passing NULL triggers an immediate kernel panic due to
device_del(&NULL->dev) and mutex_destroy(&NULL->sysfs.lock).

> +			software_node_unregister(&ref->proxy_swnode);
> +			ref->proxy_dev = NULL;
> +			ref->proxy_fwnode = NULL;

[Severity: High]
Could modifying ref->proxy_fwnode outside of ref->lock cause a data race?

Concurrently, a consumer calling gpio_shared_get_proxy() acquires
ref->lock to safely read ref->proxy_fwnode. Because this teardown path
modifies it locklessly, a data race occurs.

If gpio_shared_get_proxy() reads the pointer as NULL during the race, it
calls gpio_device_find_by_fwnode(NULL):

	struct gpio_device *gdev __free(gpio_device_put) =
			gpio_device_find_by_fwnode(ref->proxy_fwnode);

The matching function gpio_chip_match_by_fwnode() incorrectly returns true
for NULL if a device has no secondary fwnode. As a result, the consumer
acquires a descriptor from a completely unrelated GPIO chip instead of
failing safely.

>  		}
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-gpio-shared-fwnode-lookup-v4-0-2253ad645fd6@oss.qualcomm.com?part=4