Re: [PATCH v14 3/5] gpio: Leverage revocable for accessing struct gpio_chip

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] gpio: ABBA deadlock during chip removal due to SRCU lock held while sleeping in read()
- [High] gpio: TOCTOU race in gpiod_get_direction() overwrites concurrent desc->flags updates
- [Medium] gpio: memory leak of desc->label when chip is revoked before gpiod_free_commit
- [Low] core: initialization and teardown sequence mismatch (LIFO violation) in gpiodev_release
- [Low] gpio: UAF risk in callers of deprecated gpio_device_get_chip()
--

commit 1cecc3cf824694935c1167959e5066cdaf4d16c5
Author: Tzung-Bi Shih <tzungbi@kernel.org>

gpio: Leverage revocable for accessing struct gpio_chip

The underlying chip can be removed asynchronously. `gdev->srcu` is used
to ensure the synchronization before accessing `gdev->chip`. Revocable
encapsulates the details. Leverage revocable for accessing the struct
gpio_chip and remove the `gdev->srcu`.

> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index a30564d1525ad..50736dd65762d 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2590,11 +2575,9 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
>  	ssize_t bytes_read = 0;
>  	int ret;
>  	size_t event_size;
> +	struct gpio_chip *gc;
>  
> -	guard(srcu)(&cdev->gdev->srcu);
> -
> -	if (!rcu_access_pointer(cdev->gdev->chip))
> -		return -ENODEV;
> +	revocable_try_access_or_return(&cdev->gdev->chip_rev, gc);

[Severity: High]
Does this introduce a potential ABBA deadlock during chip removal?

When userspace reads from the character device (e.g., in lineinfo_watch_read(),
linereq_read(), or lineevent_read()), this macro acquires the chip_rev SRCU
read lock and holds it for the duration of the function.

If the event FIFO is empty, the reader sleeps indefinitely in
wait_event_interruptible_locked() while holding the SRCU read lock.

Concurrently, if the device is hot-unplugged, gpiochip_remove() calls
revocable_revoke(), which blocks on synchronize_srcu() waiting for all readers
to exit. The reader, however, is waiting to be woken up by the device
unregistered notifier, which is triggered via gcdev_unregister() after
revocable_revoke() has finished. This would cause the removal process to hang.

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index f9173560812d0..530daaab60dc1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -336,7 +337,10 @@ EXPORT_SYMBOL(gpio_device_get_label);
>   */
>  struct gpio_chip *gpio_device_get_chip(struct gpio_device *gdev)
>  {
> -	return rcu_dereference_check(gdev->chip, 1);
> +	struct gpio_chip *gc;
> +
> +	revocable_try_access_with(&gdev->chip_rev, gc);
> +	return gc;
>  }
>  EXPORT_SYMBOL_GPL(gpio_device_get_chip);

[Severity: Low]
This isn't a bug, but can this lead to a Use-After-Free for callers of this
deprecated API?

The revocable_try_access_with macro establishes a scoped cleanup guard that
releases the SRCU lock when exiting the function. The caller thus receives a
bare, unprotected pointer. If the GPIO chip is concurrently removed, any
subsequent dereference by callers (e.g. qe_pin_request() accessing gc->base
without synchronization primitives) could result in a UAF.

[ ... ]

> @@ -455,14 +457,13 @@ int gpiod_get_direction(struct gpio_desc *desc)
>  	unsigned long flags;
>  	unsigned int offset;
>  	int ret;
> +	struct gpio_chip *gc;
>  
>  	ret = validate_desc(desc, __func__);
>  	if (ret <= 0)
>  		return -EINVAL;
>  
> -	CLASS(gpio_chip_guard, guard)(desc);
> -	if (!guard.gc)
> -		return -ENODEV;
> +	revocable_try_access_or_return(&desc->gdev->chip_rev, gc);
>  
>  	offset = gpiod_hwgpio(desc);
>  	flags = READ_ONCE(desc->flags);
> @@ -475,7 +476,7 @@ int gpiod_get_direction(struct gpio_desc *desc)
>  	    test_bit(GPIOD_FLAG_IS_OUT, &flags))
>  		return 0;
>  
> -	ret = gpiochip_get_direction(guard.gc, offset);
> +	ret = gpiochip_get_direction(gc, offset);

[Severity: High]
Could this sequence cause a Time-of-Check to Time-of-Use (TOCTOU) race where
concurrent atomic updates to desc->flags are overwritten?

This function reads desc->flags into a local variable and then calls the
potentially sleeping gpiochip_get_direction(). If a concurrent operation
modifies desc->flags atomically (e.g., gpiochip_lock_as_irq() setting
GPIOD_FLAG_USED_AS_IRQ), the subsequent non-atomic WRITE_ONCE(desc->flags, flags)
at the end of gpiod_get_direction() will clobber that update.

If GPIOD_FLAG_USED_AS_IRQ is lost, the subsystem loses track of the fact that
the pin is used as an interrupt, allowing concurrent code to reconfigure the
pin as an output.

[ ... ]

> @@ -898,10 +897,10 @@ static void gpiodev_release(struct device *dev)
>  	synchronize_srcu(&gdev->desc_srcu);
>  	cleanup_srcu_struct(&gdev->desc_srcu);
>  
> +	revocable_destroy(&gdev->chip_rev);

[Severity: Low]
Does this teardown sequence violate LIFO (Last-In-First-Out) ordering rules?

In gpiochip_add_data_with_key(), gdev->desc_srcu is initialized before
gdev->chip_rev. Here in gpiodev_release(), gdev->desc_srcu is cleaned up
before gdev->chip_rev is destroyed.

While this may not cause an immediate crash, this fragile lifecycle
management is an anti-pattern.

[ ... ]

> @@ -2680,16 +2674,17 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
>  void gpiod_free_commit(struct gpio_desc *desc)
>  {
>  	unsigned long flags;
> +	struct gpio_chip *gc;
>  
>  	might_sleep();
>  
> -	CLASS(gpio_chip_guard, guard)(desc);
> +	revocable_try_access_or_return_void(&desc->gdev->chip_rev, gc);
>  
>  	flags = READ_ONCE(desc->flags);
>  
> -	if (guard.gc && test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
> -		if (guard.gc->free)
> -			guard.gc->free(guard.gc, gpiod_hwgpio(desc));
> +	if (test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
> +		if (gc->free)
> +			gc->free(gc, gpiod_hwgpio(desc));

[Severity: Medium]
With the early return introduced here, does this skip the rest of the cleanup
block if the chip has been revoked, leaking the label?

If a GPIO line is requested and a label is dynamically allocated, and later the
chip is unbound (revoking the chip_rev handle), calling gpiod_free_commit()
will return early. This skips the desc_set_label(desc, NULL) cleanup further
down in the function.

When gpiodev_release() later frees the descriptor array, the active labels are
not freed, which results in a memory leak.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011121246.162759-1-tzungbi@kernel.org?part=3