Re: [PATCH] gpiolib: cdev: Fix use after free in lineinfo_changed_notify

[Kent Gibson]

On Wed, May 01, 2024 at 10:26:12AM +0800, Zhongqiu Han wrote:
> The use-after-free issue occurs when userspace closes the GPIO chip device
> file (e.g., "/dev/gpiochip0") by invoking gpio_chrdev_release(), while one
> of its GPIO lines is simultaneously being released. In a stress test
> scenario, stress-ng tool starts multi stress-ng-dev threads to continuously
> open and close device file in the /dev, the use-after-free issue will occur
> with a low reproducibility.

This could be clearer.  Use-after-free of what?  We don't find out it is
the watched_lines bitmap until much later...

>
> Here is the typical stack when issue happened:
>

This stack trace is excessive [1].

> BUG: KASAN: slab-use-after-free in lineinfo_changed_notify+0x84/0x1bc
> Read of size 8 at addr ffffff803c06e840 by task stress-ng-dev/5437
> Call trace:
>  dump_backtrace
>  show_stack
>  dump_stack_lvl
>  print_report
>  kasan_report
>  __asan_load8
>  lineinfo_changed_notify
>  notifier_call_chain
>  blocking_notifier_call_chain
>  gpiod_free_commit
>  gpiod_free
>  gpio_free
>  st54spi_gpio_dev_release
>  __fput
>  __fput_sync
>  __arm64_sys_close
>  invoke_syscall
>  el0_svc_common
>  do_el0_svc
>  el0_svc
>  el0t_64_sync_handler
>  el0t_64_sync
> Allocated by task 5425:
>  kasan_set_track
>  kasan_save_alloc_info
>  __kasan_kmalloc
>  __kmalloc
>  bitmap_zalloc
>  gpio_chrdev_open
>  chrdev_open
>  do_dentry_open
>  vfs_open
>  path_openat
>  do_filp_open
>  do_sys_openat2
>  __arm64_sys_openat
>  invoke_syscall
>  el0_svc_common
>  do_el0_svc
>  el0_svc
>  el0t_64_sync_handler
>  el0t_64_sync
> Freed by task 5425:
>  kasan_set_track
>  kasan_save_free_info
>  ____kasan_slab_free
>  __kasan_slab_free
>  slab_free_freelist_hook
>  __kmem_cache_free
>  kfree
>  bitmap_free
>  gpio_chrdev_release
>  __fput
>  __fput_sync
>  __arm64_sys_close
>  invoke_syscall
>  el0_svc_common
>  do_el0_svc
>  el0_svc
>  el0t_64_sync_handler
>  el0t_64_sync
>
> The use-after-free issue occurs as follows: watched_lines is freed by
> bitmap_free(), but the lineinfo_changed_nb notifier chain cannot be
> successfully unregistered due to waiting write rwsem when closing the
> GPIO chip device file. Additionally, one of the GPIO chip's lines is
> also in the release process and holds the notifier chain's read rwsem.
> Consequently, a race condition leads to the use-after-free of
> watched_lines.
>

Drop the stack trace above and rework this paragraph into the opening
paragraph.

Might be good to note the side effects of the use-after-free.
AFAICT it may only result in an event being generated for userspace where
it shouldn't.  But, as the chrdev is being closed, userspace wont have the
chance to read that event anyway, so no appreciable difference?

> [free]
> gpio_chrdev_release()
>   --> bitmap_free(cdev->watched_lines)                  <-- freed
>   --> blocking_notifier_chain_unregister()
>     --> down_write(&nh->rwsem)                          <-- waiting rwsem
>           --> __down_write_common()
>             --> rwsem_down_write_slowpath()
>                   --> schedule_preempt_disabled()
>                     --> schedule()
>
> [use]
> st54spi_gpio_dev_release()
>   --> gpio_free()
>     --> gpiod_free()
>       --> gpiod_free_commit()
>         --> gpiod_line_state_notify()
>           --> blocking_notifier_call_chain()
>             --> down_read(&nh->rwsem);                  <-- held rwsem
>             --> notifier_call_chain()
>               --> lineinfo_changed_notify()
>                 --> test_bit(xxxx, cdev->watched_lines) <-- use after free
>
> To fix this issue, call the bitmap_free() function after successfully

"successfully" is confusing here as there is no unsuccessfully.

> unregistering the notifier chain. This prevents lineinfo_changed_notify()
> from being called, thus avoiding the use-after-free race condition.

The final sentence doesn't add anything - the reorder fixing the problem
is clear enough.

>
> Fixes: 51c1064e82e7 ("gpiolib: add new ioctl() for monitoring changes in line info")
> Signed-off-by: Zhongqiu Han <quic_zhonhan@xxxxxxxxxxx>
> ---
>  drivers/gpio/gpiolib-cdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index d09c7d728365..6b3a43e3ba47 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2799,11 +2799,11 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file)
>  	struct gpio_chardev_data *cdev = file->private_data;
>  	struct gpio_device *gdev = cdev->gdev;
>
> -	bitmap_free(cdev->watched_lines);
>  	blocking_notifier_chain_unregister(&gdev->device_notifier,
>  					   &cdev->device_unregistered_nb);
>  	blocking_notifier_chain_unregister(&gdev->line_state_notifier,
>  					   &cdev->lineinfo_changed_nb);
> +	bitmap_free(cdev->watched_lines);
>  	gpio_device_put(gdev);
>  	kfree(cdev);
>

No problem with the code change - makes total sense.

Cheers,
Kent.

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages