| Message ID | 20240501022612.1787143-1-quic_zhonhan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: cdev: Fix use after free in lineinfo_changed_notify | expand |
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@quicinc.com> > --- > 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
On 5/2/2024 9:51 AM, Kent Gibson wrote: > 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... > Hi Kent, Thanks a lot for the review, I will take care of this on v2 and plan to verify it as follows: The use-after-free issue occurs as follows: when the GPIO chip device file is being closed by invoking gpio_chrdev_release(), watched_lines is freed by bitmap_free(), but the unregistration of lineinfo_changed_nb notifier chain failed due to waiting write rwsem. 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. >> >> Here is the typical stack when issue happened: >> > > This stack trace is excessive [1]. > Acknowledged. I will drop it. >> 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. > Yes, I will drop the stack above and rework this paragraph into opening paragraph. Like the first comments reply. > 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? > Yes, there is no NULL access issue because there doesn't set watched_lines = NULL; after bitmap_free, I also think it can only cause the unexpected event is being generated. I will take care of it on v2. >> [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. > Acknowledged. I plan to verify it as follows: To fix this issue, call the bitmap_free() function after blocking_notifier_chain_unregister. >> 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. > Acknowledged. I will remove it. >> >> Fixes: 51c1064e82e7 ("gpiolib: add new ioctl() for monitoring changes in line info") >> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com> >> --- >> 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. > Thanks for the review. > Cheers, > Kent. > > [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
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);
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. Here is the typical stack when issue happened: 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. [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 unregistering the notifier chain. This prevents lineinfo_changed_notify() from being called, thus avoiding the use-after-free race condition. Fixes: 51c1064e82e7 ("gpiolib: add new ioctl() for monitoring changes in line info") Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com> --- drivers/gpio/gpiolib-cdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)