From: Zheng Yejian zhengyejian1@huawei.com
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-zhengyej...
Cc: stable@vger.kernel.org Cc: mhiramat@kernel.org Cc: mark.rutland@arm.com Cc: mathieu.desnoyers@efficios.com Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization") Suggested-by: Steven Rostedt rostedt@goodmis.org Signed-off-by: Zheng Yejian zhengyejian1@huawei.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org [Hagar: Modified to apply on v5.4.y] Signed-off-by: Hagar Hemdan hagarhem@amazon.com --- only compile tested. --- 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; + } } - - 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)
On Mon, Nov 11, 2024 at 02:44:45PM +0000, Hagar Hemdan wrote:
From: Zheng Yejian zhengyejian1@huawei.com
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 | CPU2register_kprobes() { | delete_module() { check_kprobe_address_safe() { | arch_check_ftrace_location() { | ftrace_location() { | lookup_rec() // USE! | ftrace_release_mod() // Free!
To fix this issue:
- Hold rcu lock as accessing ftrace pages in ftrace_location_range();
- Use ftrace_location_range() instead of lookup_rec() in ftrace_location();
- 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-zhengyej...
Cc: stable@vger.kernel.org Cc: mhiramat@kernel.org Cc: mark.rutland@arm.com Cc: mathieu.desnoyers@efficios.com Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization") Suggested-by: Steven Rostedt rostedt@goodmis.org Signed-off-by: Zheng Yejian zhengyejian1@huawei.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org [Hagar: Modified to apply on v5.4.y] Signed-off-by: Hagar Hemdan hagarhem@amazon.com
only compile tested.
kernel/trace/ftrace.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
Now queued up, thanks.
greg k-h
On Mon, 11 Nov 2024 14:44:45 +0000 Hagar Hemdan hagarhem@amazon.com wrote:
From: Zheng Yejian zhengyejian1@huawei.com
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 | CPU2register_kprobes() { | delete_module() { check_kprobe_address_safe() { | arch_check_ftrace_location() { | ftrace_location() { | lookup_rec() // USE! | ftrace_release_mod() // Free!
To fix this issue:
- Hold rcu lock as accessing ftrace pages in ftrace_location_range();
- Use ftrace_location_range() instead of lookup_rec() in ftrace_location();
- 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-zhengyej...
Cc: stable@vger.kernel.org Cc: mhiramat@kernel.org Cc: mark.rutland@arm.com Cc: mathieu.desnoyers@efficios.com Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization") Suggested-by: Steven Rostedt rostedt@goodmis.org Signed-off-by: Zheng Yejian zhengyejian1@huawei.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org [Hagar: Modified to apply on v5.4.y] Signed-off-by: Hagar Hemdan hagarhem@amazon.com
only compile tested.
You should do more than that. At least run the ftrace selftests.
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
}
- 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();@@ -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();/* 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)
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@amazon.com wrote:
From: Zheng Yejian zhengyejian1@huawei.com
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 | CPU2register_kprobes() { | delete_module() { check_kprobe_address_safe() { | arch_check_ftrace_location() { | ftrace_location() { | lookup_rec() // USE! | ftrace_release_mod() // Free!
To fix this issue:
- Hold rcu lock as accessing ftrace pages in ftrace_location_range();
- Use ftrace_location_range() instead of lookup_rec() in ftrace_location();
- 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-zhengyej...
Cc: stable@vger.kernel.org Cc: mhiramat@kernel.org Cc: mark.rutland@arm.com Cc: mathieu.desnoyers@efficios.com Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization") Suggested-by: Steven Rostedt rostedt@goodmis.org Signed-off-by: Zheng Yejian zhengyejian1@huawei.com Signed-off-by: Steven Rostedt (Google) rostedt@goodmis.org [Hagar: Modified to apply on v5.4.y] Signed-off-by: Hagar Hemdan hagarhem@amazon.com
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();@@ -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();/* 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)
On Wed, 13 Nov 2024 14:50:37 +0000 Hagar Hemdan hagarhem@amazon.com wrote:
only compile tested.
You should do more than that. At least run the ftrace selftests.
ok, tested v2 and will send it soon.
You may want to run it before and after your patch applied, as there may be some tests that fail on current 5.4. As long as your change doesn't add more failures it should be OK.
-- Steve
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Hagar Hemdan -
Steven Rostedt