Re: [PATCH] gpio: virtuser: fix interrupt leak and UAF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] gpiolib: race condition and double free in desc_set_label()
  [1]
--

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

gpio: virtuser: fix interrupt leak and UAF

This commit serializes access to the virtual user interrupt state using the
per-line mutex, prevents duplicate IRQ requests, and registers a devm action
to free the interrupt on device unbind.

> diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c
> index 185c1be39b277..21b4bc64d4d34 100644
> --- a/drivers/gpio/gpio-virtuser.c
> +++ b/drivers/gpio/gpio-virtuser.c

[ ... ]

> @@ -635,7 +635,7 @@ static ssize_t gpio_virtuser_consumer_write(struct file *file,
>  	if (ret)
>  		return ret;
>  
> -	scoped_guard(mutex, &data->consumer_lock)
> +	scoped_guard(mutex, &data->lock)
>  		strscpy(data->consumer, buf, sizeof(data->consumer));
>  
>  	return count;

[Severity: Critical]
This problem wasn't introduced by this patch, but can concurrent writes in
gpio_virtuser_consumer_write() race in desc_set_label() [1] and double
enqueue old->rh via call_srcu()?

Just above this hunk, gpio_virtuser_consumer_write() calls
gpiod_set_consumer_name() -> desc_set_label() without holding data->lock, and
desc_set_label() replaces desc->label without any update-side lock:

drivers/gpio/gpiolib.c:desc_set_label() {
	...
	old = rcu_replace_pointer(desc->label, new, 1);
	if (old)
		call_srcu(&desc->gdev->desc_srcu, &old->rh, desc_free_label);

	return 0;
}

Because rcu_replace_pointer() is just a load followed by a store, two
concurrent callers can read the same old pointer:

CPU0                                CPU1
gpio_virtuser_consumer_write()      gpio_virtuser_consumer_write()
  gpiod_set_consumer_name()           gpiod_set_consumer_name()
    desc_set_label()                    desc_set_label()
      old = rcu_dereference(...)
                                          old = rcu_dereference(...)
      rcu_assign_pointer(..., new0)
                                          rcu_assign_pointer(..., new1)
      call_srcu(..., &old->rh, ...)
                                          call_srcu(..., &old->rh, ...)
                                            <-- double call_srcu() / double free

Queuing the same rcu_head twice corrupts the SRCU callback list and double
frees old after the grace period, while leaking new0.

[1] https://sashiko.dev/#/bug/linux-8fa6e742-7945-4de7-8786-8ad6e6c6ea4e

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gpio-virtuser-irq-uaf-leak-v1-1-ce9772d8ad0d@oss.qualcomm.com?part=1