Re: [PATCH v3 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:
- [Critical] gpio: shared: race returns wrong GPIO desc on uninitialized proxy_fwnode
- [High] gpio: shared: teardown unconditionally dereferences NULL proxy_dev
- [High] gpio: shared: NULL pointer evaluation misidentifies virtual chips as proxies
- [Medium] gpio: shared: memory leak of dynamically allocated proxy devices on error path
--

commit 5669c21b32e82fcb8daf60473942426be156bbb3
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 dynamically allocates its memory during proxy setup. It also
renames the field to proxy_dev to better reflect its purpose.

> 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);

[Severity: High]
Does this code crash if ref->proxy_dev is NULL?

If a shared GPIO entry has 1 or fewer consumers, gpiochip_setup_shared() skips
creating the proxy device, leaving ref->proxy_dev as NULL. Alternatively, if a
GPIO controller probes before gpio_shared_init populates the list, proxy_dev
remains NULL. Later, when the controller is removed,
gpio_device_teardown_shared() unconditionally calls
gpio_shared_remove_proxy_dev().

Because dev is at offset 0 of struct auxiliary_device, &auxdev->dev evaluates
to NULL, and device_del(NULL) dereferences NULL->parent, causing a panic.

> +	auxiliary_device_uninit(ref->proxy_dev);
> +	ref->proxy_dev = NULL;
>  }
>  
>  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]
Can this evaluate to NULL == NULL and incorrectly identify virtual controllers
as shared proxies?

Because ref->proxy_dev is initially NULL before setup, and dev is at offset 0,
&ref->proxy_dev->dev evaluates to NULL. If a virtual GPIO controller like
gpio-mockup or gpio-sim probes with no parent device, gdev->dev.parent is also
NULL. This early return would abort the actual shared 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: Medium]
Does this leak memory and IDA identifiers if a subsequent consumer allocation
fails?

If gpio_shared_make_proxy_dev() fails, the function immediately returns
without rolling back the dynamically allocated proxy devices from earlier
iterations in the loop. Also, in the caller gpiochip_add_data_with_key(),
a failure here jumps directly to err_remove_irqchip, bypassing
err_teardown_shared and permanently leaking the auxiliary devices.

[Severity: Critical]
Does setting the shared flag earlier in this function expose an uninitialized
proxy to concurrent consumers?

Since the provider is already on the gpio_devices list, setting
GPIOD_FLAG_SHARED before creating the proxy device can allow a concurrent
gpiod_get() to enter gpio_shared_get_proxy().

Because the proxy and its fwnode are not yet initialized, the consumer will
read NULL. It then calls gpio_device_find_by_fwnode(NULL). In
gpio_chip_match_by_fwnode(), the condition:
    return node && node->secondary == fwnode;
evaluates to node->secondary == NULL, incorrectly matching the first GPIO
controller in the system that has a primary fwnode but no secondary. This
could return an unrelated GPIO descriptor and cause cross-device hardware
corruption.

>  			}
> @@ -624,7 +636,7 @@ void gpio_device_teardown_shared(struct gpio_device *gdev)
>  				ref->lookup = NULL;
>  			}
>  
> -			gpio_shared_remove_adev(&ref->adev);
> +			gpio_shared_remove_proxy_dev(ref);
>  		}
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-gpio-shared-fwnode-lookup-v3-0-4553c1a7d80f@oss.qualcomm.com?part=3