On Tue, Nov 12, 2024 at 10:46:18AM -0500, Steven Rostedt wrote:
> On Mon, 11 Nov 2024 14:44:45 +0000
> Hagar Hemdan <hagarhem@xxxxxxxxxx> wrote:
>
> > From: Zheng Yejian <zhengyejian1@xxxxxxxxxx>
> >
> > commit e60b613df8b6253def41215402f72986fee3fc8d upstream.
> >
> > KASAN reports a bug:
> >
> > BUG: KASAN: use-after-free in ftrace_location+0x90/0x120
> > Read of size 8 at addr ffff888141d40010 by task insmod/424
> > CPU: 8 PID: 424 Comm: insmod Tainted: G W 6.9.0-rc2+
> > [...]
> > Call Trace:
> > <TASK>
> > dump_stack_lvl+0x68/0xa0
> > print_report+0xcf/0x610
> > kasan_report+0xb5/0xe0
> > ftrace_location+0x90/0x120
> > register_kprobe+0x14b/0xa40
> > kprobe_init+0x2d/0xff0 [kprobe_example]
> > do_one_initcall+0x8f/0x2d0
> > do_init_module+0x13a/0x3c0
> > load_module+0x3082/0x33d0
> > init_module_from_file+0xd2/0x130
> > __x64_sys_finit_module+0x306/0x440
> > do_syscall_64+0x68/0x140
> > entry_SYSCALL_64_after_hwframe+0x71/0x79
> >
> > The root cause is that, in ftrace_location_range(), ftrace record of some address
> > is being searched in ftrace pages of some module, but those ftrace pages
> > at the same time is being freed in ftrace_release_mod() as the
> > corresponding module is being deleted:
> >
> > CPU1 | CPU2
> > register_kprobes() { | delete_module() {
> > check_kprobe_address_safe() { |
> > arch_check_ftrace_location() { |
> > ftrace_location() { |
> > lookup_rec() // USE! | ftrace_release_mod() // Free!
> >
> > To fix this issue:
> > 1. Hold rcu lock as accessing ftrace pages in ftrace_location_range();
> > 2. Use ftrace_location_range() instead of lookup_rec() in
> > ftrace_location();
> > 3. Call synchronize_rcu() before freeing any ftrace pages both in
> > ftrace_process_locs()/ftrace_release_mod()/ftrace_free_mem().
> >
> > Link: https://lore.kernel.org/linux-trace-kernel/20240509192859.1273558-1-zhengyejian1@xxxxxxxxxx
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Cc: <mhiramat@xxxxxxxxxx>
> > Cc: <mark.rutland@xxxxxxx>
> > Cc: <mathieu.desnoyers@xxxxxxxxxxxx>
> > Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization")
> > Suggested-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Signed-off-by: Zheng Yejian <zhengyejian1@xxxxxxxxxx>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> > [Hagar: Modified to apply on v5.4.y]
> > Signed-off-by: Hagar Hemdan <hagarhem@xxxxxxxxxx>
> > ---
> > only compile tested.
>
> You should do more than that. At least run the ftrace selftests.
ok, tested v2 and will send it soon.
>
> > ---
> > kernel/trace/ftrace.c | 30 +++++++++++++++++++++---------
> > 1 file changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 412505d94865..60bf8a6d55ce 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1552,7 +1552,9 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> > struct ftrace_page *pg;
> > struct dyn_ftrace *rec;
> > struct dyn_ftrace key;
> > + unsigned long ip = 0;
> >
> > + rcu_read_lock();
> > key.ip = start;
> > key.flags = end; /* overload flags, as it is unsigned long */
> >
> > @@ -1565,10 +1567,13 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> > sizeof(struct dyn_ftrace),
> > ftrace_cmp_recs);
> > if (rec)
> > - return rec->ip;
> > + {
> > + ip = rec->ip;
> > + break;
> > + }
>
> The above breaks Linux coding style. It should be:
>
> if (rec) {
> ip = rec->ip;
> break;
> }
>
> -- Steve
ok, updated in v2, thanks!
>
>
> > }
> > -
> > - return 0;
> > + rcu_read_unlock();
> > + return ip;
> > }
> >
> > /**
> > @@ -5736,6 +5741,8 @@ static int ftrace_process_locs(struct module *mod,
> > /* We should have used all pages unless we skipped some */
> > if (pg_unuse) {
> > WARN_ON(!skipped);
> > + /* Need to synchronize with ftrace_location_range() */
> > + synchronize_rcu();
> > ftrace_free_pages(pg_unuse);
> > }
> > return ret;
> > @@ -5889,6 +5896,9 @@ void ftrace_release_mod(struct module *mod)
> > out_unlock:
> > mutex_unlock(&ftrace_lock);
> >
> > + /* Need to synchronize with ftrace_location_range() */
> > + if (tmp_page)
> > + synchronize_rcu();
> > for (pg = tmp_page; pg; pg = tmp_page) {
> >
> > /* Needs to be called outside of ftrace_lock */
> > @@ -6196,6 +6206,7 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
> > unsigned long start = (unsigned long)(start_ptr);
> > unsigned long end = (unsigned long)(end_ptr);
> > struct ftrace_page **last_pg = &ftrace_pages_start;
> > + struct ftrace_page *tmp_page = NULL;
> > struct ftrace_page *pg;
> > struct dyn_ftrace *rec;
> > struct dyn_ftrace key;
> > @@ -6239,12 +6250,8 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
> > ftrace_update_tot_cnt--;
> > if (!pg->index) {
> > *last_pg = pg->next;
> > - if (pg->records) {
> > - free_pages((unsigned long)pg->records, pg->order);
> > - ftrace_number_of_pages -= 1 << pg->order;
> > - }
> > - ftrace_number_of_groups--;
> > - kfree(pg);
> > + pg->next = tmp_page;
> > + tmp_page = pg;
> > pg = container_of(last_pg, struct ftrace_page, next);
> > if (!(*last_pg))
> > ftrace_pages = pg;
> > @@ -6261,6 +6268,11 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
> > clear_func_from_hashes(func);
> > kfree(func);
> > }
> > + /* Need to synchronize with ftrace_location_range() */
> > + if (tmp_page) {
> > + synchronize_rcu();
> > + ftrace_free_pages(tmp_page);
> > + }
> > }
> >
> > void __init ftrace_free_init_mem(void)
>
