Date: Fri, 3 May 2024 00:55:29 +0800
From: Zhongqiu Han <quic_zhonhan@...cinc.com>
To: Kent Gibson <warthog618@...il.com>
CC: <brgl@...ev.pl>, <linus.walleij@...aro.org>, <linux-gpio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_zhonhan@...cinc.com>
Subject: Re: [PATCH] gpiolib: cdev: Fix use after free in
 lineinfo_changed_notify

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@...cinc.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

-- 
Thx and BRs,
Zhongqiu Han

Powered by blists - more mailing lists