From: Kent Gibson <warthog618@gmail.com>
To: Zhongqiu Han <quic_zhonhan@quicinc.com>
Cc: brgl@bgdev.pl, linus.walleij@linaro.org,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] gpiolib: cdev: Fix use after free in lineinfo_changed_notify
Date: Thu, 2 May 2024 09:51:22 +0800 [thread overview]
Message-ID: <20240502015122.GA15967@rigel> (raw)
In-Reply-To: <20240501022612.1787143-1-quic_zhonhan@quicinc.com>
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
next prev parent reply other threads:[~2024-05-02 1:51 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-01 2:26 [PATCH] gpiolib: cdev: Fix use after free in lineinfo_changed_notify Zhongqiu Han
2024-05-02 1:51 ` Kent Gibson [this message]
2024-05-02 16:55 ` Zhongqiu Han
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240502015122.GA15967@rigel \
--to=warthog618@gmail.com \
--cc=brgl@bgdev.pl \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=quic_zhonhan@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).