On Fri, Nov 15, 2024 at 07:37:59AM +0100, Greg Kroah-Hartman wrote:
> 5.4-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> 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 lookup_rec(), 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>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
> kernel/trace/ftrace.c | 30 +++++++++++++++++++++---------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1552,7 +1552,9 @@ unsigned long ftrace_location_range(unsi
> 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(unsi
> sizeof(struct dyn_ftrace),
> ftrace_cmp_recs);
> if (rec)
> - return rec->ip;
> + {
> + ip = rec->ip;
> + break;
> + }
> }
> -
> - return 0;
> + rcu_read_unlock();
> + return ip;
> }
>
> /**
> @@ -5736,6 +5741,8 @@ static int ftrace_process_locs(struct mo
> /* 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 *m
> 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,
> 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,
> 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,
> 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)
>
>
Hi,
I observed that since this backport, on linux-5.4.y x86-64, a simple 'echo
function > current_tracer' without any filter can easily result in double
fault (int3) and system becomes unresponsible. linux-5.4.y x86 code has not
yet been converted to use text_poke(), so IIUC the issue appears to be that
the old ftrace_int3_handler()->ftrace_location() path now includes
rcu_read_lock() with this backport patch, which has mcount location inside,
that leads to the double fault.
I verified on an x86-64 qemu env that applying the following 11 additional
backports resolves the issue. The main purpose is to backport #7. All the
commits can be cleanly applied to the latest linux-5.4.y (v5.4.288).
#11. fd3dc56253ac ftrace/x86: Add back ftrace_expected for ftrace bug reports
#10. ac6c1b2ca77e ftrace/x86: Add back ftrace_expected assignment
#9. 59566b0b622e x86/ftrace: Have ftrace trampolines turn read-only at the end of system boot up
#8. 38ebd8d11924 x86/ftrace: Mark ftrace_modify_code_direct() __ref
#7. 768ae4406a5c x86/ftrace: Use text_poke()
#6. 63f62addb88e x86/alternatives: Add and use text_gen_insn() helper
#5. 18cbc8bed0c7 x86/alternatives, jump_label: Provide better text_poke() batching interface
#4. 8f4a4160c618 x86/alternatives: Update int3_emulate_push() comment
#3. 72ebb5ff806f x86/alternative: Update text_poke_bp() kernel-doc comment
#2. 3a1255396b5a x86/alternatives: add missing insn.h include
#1. c3d6324f841b x86/alternatives: Teach text_poke_bp() to emulate instructions
Note: #8-11 are follow-up fixes for #7
#2-3 are follow-up fixes for #1
According to [1], no regressions were observed on x86_64, which included
running kselftest-ftrace. So I'm a bit confused.
Could someone take a look and shed light on this? (ftrace on linux-5.4.y x86)
Thanks.
[1] https://lore.kernel.org/stable/CA+G9fYtdzDCDP_RxjPKS5wvQH=NsjT+bDRbukFqoX6cN+EHa7Q@xxxxxxxxxxxxxx/
-Koichiro Den
