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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: shared: unlocked proxy_dev teardown in gpio_shared_remove_proxy_dev()
--

commit 744026242eec2478ed3cf7962883c73cf7ffb5cd
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 out of struct gpio_shared_ref in
drivers/gpio/gpiolib-shared.c and allocates it dynamically as proxy_dev
when setting up a shared GPIO proxy device.

> diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c
> index 043d5daba7a73..3ed25609cae10 100644
> --- a/drivers/gpio/gpiolib-shared.c
> +++ b/drivers/gpio/gpiolib-shared.c
> @@ -35,11 +35,10 @@ struct gpio_shared_ref {
>  	/* GPIO flags this consumer uses for the request. */
>  	enum gpiod_flags flags;
>  	char *con_id;
> -	int dev_id;
>  	/* Protects the auxiliary device struct and the lookup table. */
>  	struct mutex lock;
>  	struct lock_class_key lock_key;
> -	struct auxiliary_device adev;
> +	struct auxiliary_device *proxy_dev;
>  	struct gpiod_lookup_table *lookup;
>  	bool is_reset_gpio;
>  };

[ ... ]

> @@ -306,31 +301,43 @@ static int gpio_shared_of_scan(void)
>  }
>  #endif /* CONFIG_OF */
>  
> -static void gpio_shared_adev_release(struct device *dev)
> +static void gpio_shared_proxy_dev_release(struct device *dev)
>  {
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
>  
> +	ida_free(&gpio_shared_ida, adev->id);
> +	kfree(adev);
>  }
>  
> -static int gpio_shared_make_adev(struct gpio_device *gdev,
> -				 struct gpio_shared_entry *entry,
> -				 struct gpio_shared_ref *ref)
> +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 = &ref->adev;
> -	int ret;
> +	struct auxiliary_device *adev;
> +	int ret, id;
>  
> -	guard(mutex)(&ref->lock);
> +	adev = kzalloc_obj(*adev);
> +	if (!adev)
> +		return -ENOMEM;

[ ... ]

>  	pr_debug("Created an auxiliary GPIO proxy %s for GPIO device %s\n",
>  		 dev_name(&adev->dev), gpio_device_get_label(gdev));
>  
> +	scoped_guard(mutex, &ref->lock) {
> +		ref->proxy_dev = adev;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -480,7 +491,7 @@ int gpio_shared_add_proxy_lookup(struct device *consumer, struct fwnode_handle *
>  			char *key __free(kfree) =
>  				kasprintf(GFP_KERNEL,
>  					  KBUILD_MODNAME ".proxy.%u",
> -					  ref->adev.id);
> +					  ref->proxy_dev->id);
>  			if (!key)
>  				return -ENOMEM;
>  
> @@ -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]
Can tearing down ref->proxy_dev in gpio_shared_remove_proxy_dev() without
holding ref->lock race with concurrent readers and cause a use-after-free or
NULL pointer dereference?

In drivers/gpio/gpiolib-shared.c, struct gpio_shared_ref documents that
ref->lock protects the auxiliary device and the lookup table, and
gpio_shared_make_proxy_dev() assigns ref->proxy_dev under ref->lock.

However, gpio_shared_remove_proxy_dev() and gpio_shared_teardown_entry() do
not take ref->lock, and auxiliary_device_uninit(ref->proxy_dev) drops the
final reference and frees ref->proxy_dev via gpio_shared_proxy_dev_release()
before ref->proxy_dev is set to NULL.

In gpiochip_add_data_with_key(), gpiochip_setup_dev() calls gcdev_register()
while gdev is already visible on gpio_devices. If gpiochip_sysfs_register()
subsequently fails, the error path runs:

gpiochip_add_data_with_key()
  -> gpio_device_teardown_shared()
    -> gpio_shared_teardown_entry()
      -> gpio_shared_remove_proxy_dev()

before gdev is removed from gpio_devices and synchronize_srcu() is called
(and gpiod_free_commit() leaves GPIOD_FLAG_SHARED set on the descriptor).

A concurrent consumer in gpiod_find_and_request() that looked up the
descriptor under gpio_devices_srcu can call gpio_shared_add_proxy_lookup(),
take ref->lock, and dereference ref->proxy_dev->id while
gpio_shared_remove_proxy_dev() is freeing ref->proxy_dev or after it has set
ref->proxy_dev to NULL.

Similarly, if a consumer node references multiple shared GPIOs with the same
fwnode and con_id across two GPIO controllers, requesting the pin on one
registered controller calls gpio_shared_add_proxy_lookup(), which walks
gpio_shared_list and can match the ref belonging to the second controller
while its ref->proxy_dev is still NULL or being torn down in
gpio_shared_remove_proxy_dev().

Concurrent gpiochip_setup_shared() calls for other controllers also walk
gpio_shared_list without ref->lock:

	list_for_each_entry(entry, &gpio_shared_list, list) {
		list_for_each_entry(ref, &entry->refs, list) {
			if (ref->proxy_dev && gdev->dev.parent == &ref->proxy_dev->dev) {

which can read a non-NULL ref->proxy_dev pointer after
auxiliary_device_uninit() has already freed the structure (or after the slab
slot has been reallocated).

Note that the follow-up commit ("gpio: shared: convert to using software
nodes for GPIO shared proxy lookup") still leaves
gpio_shared_remove_proxy_dev() unlocked and frees ref->proxy_dev and
unregisters ref->proxy_swnode before clearing ref->proxy_dev and
ref->proxy_fwnode to NULL.

Should gpio_shared_remove_proxy_dev() and the readers of ref->proxy_dev
serialize access with ref->lock and check for a NULL ref->proxy_dev?

>  
>  static void gpio_shared_teardown_entry(struct gpio_device *gdev,
> @@ -534,7 +546,7 @@ static void gpio_shared_teardown_entry(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/20261021-gpio-shared-fwnode-lookup-v6-0-6af7d8eefd25@oss.qualcomm.com?part=4