Re: [PATCH v4 3/4] gpio: shared: detach the proxy device from struct gpio_shared_ref

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] gpiolib: logic bug and UB falsely matching parentless chips as shared proxies
- [High] gpio: NULL pointer dereference in teardown of early-probed shared GPIOs
- [High] gpio: proxy devices and descriptors leak on setup error path
- [Medium] gpiolib: memory and IDA leak on shared proxy setup failure
--

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

gpio: shared: detach the proxy device from struct gpio_shared_ref

This commit moves the auxiliary device struct out of the shared reference
structure and allocates its memory separately on proxy setup to prevent
memory corruption if references outlive the parent's teardown.

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index 8c9ac9d933049..a57a2dd5ca00e 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c
[ ... ]
> @@ -511,10 +522,11 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *
>  	return -ENOENT;
>  }
>  
> -static void gpio_shared_remove_adev(struct auxiliary_device *adev)
> +static void gpio_shared_remove_proxy_dev(struct gpio_shared_ref *ref)
>  {
> -	auxiliary_device_delete(adev);
> -	auxiliary_device_uninit(adev);
> +	auxiliary_device_delete(ref->proxy_dev);
> +	auxiliary_device_uninit(ref->proxy_dev);
> +	ref->proxy_dev = NULL;
>  }

[Severity: High]
If a GPIO controller containing shared GPIOs is probed early, its proxy
devices might never be created, leaving ref->proxy_dev as NULL.

If the controller is later removed, gpio_device_teardown_shared() will call
gpio_shared_remove_proxy_dev(), which unconditionally passes this NULL
pointer to auxiliary_device_delete() and auxiliary_device_uninit().

Will this result in a kernel panic when the driver core attempts to
dereference the NULL proxy_dev?

>  
>  int gpiochip_setup_shared(struct gpio_chip *gc)
> @@ -527,7 +539,7 @@ int gpiochip_setup_shared(struct gpio_chip *gc)
>  
>  	list_for_each_entry(entry, &gpio_shared_list, list) {
>  		list_for_each_entry(ref, &entry->refs, list) {
> -			if (gdev->dev.parent == &ref->adev.dev) {
> +			if (gdev->dev.parent == &ref->proxy_dev->dev) {

[Severity: High]
Before a proxy is instantiated, ref->proxy_dev is NULL, meaning
&ref->proxy_dev->dev will evaluate to NULL.

If a newly registered GPIO controller has no parent device (such as virtual
controllers like gpio-mockup or gpio-sim), gdev->dev.parent is also NULL.

Does this cause the condition to falsely evaluate to true (NULL == NULL)
and erroneously identify generic controllers as shared proxies, breaking
their setup?

>  				/*
>  				 * This is a shared GPIO proxy. Mark its
>  				 * descriptor as such and return here.
[ ... ]
> @@ -592,7 +604,7 @@ int gpiochip_setup_shared(struct gpio_chip *gc)
>  				 fwnode_get_name(ref->fwnode) ?: "(no fwnode)",
>  				 ref->con_id ?: "(none)");
>  
> -			ret = gpio_shared_make_adev(gdev, entry, ref);
> +			ret = gpio_shared_make_proxy_dev(gdev, entry, ref);
>  			if (ret) {
>  				gpiod_free_commit(desc);
>  				return ret;

[Severity: High]
If gpio_shared_make_proxy_dev() fails and returns an error, the function
returns immediately without unwinding any proxy devices that were
successfully created during previous iterations of the loop.

Additionally, in the caller gpiochip_add_data_with_key(), the error path
jumps to err_remove_irqchip, bypassing err_teardown_shared.

Are the successfully registered auxiliary devices, their kzalloc'd memory,
allocated IDA slots, and committed GPIO descriptors permanently leaked on
this error path?

>  			}
[ ... ]

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