LinuxLists.cc - [PATCH] ftrace: Fix use-after-free issue in ftrace

2024-04-01 12:55:25

Subject: [PATCH] ftrace: Fix use-after-free issue in ftrace_location()

2024-04-10 15:26:12

Subject: Re: [PATCH] ftrace: Fix use-after-free issue in ftrace_location()

On Mon, 1 Apr 2024 20:55:43 +0800
Zheng Yejian <[email protected]> wrote:

> 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+ #213
> [...]
> 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 when lookup_rec() is lookuping ftrace record of
> an address in some module, and at the same time in ftrace_release_mod(),
> the memory that saving ftrace records has been freed as that module is
> being deleted.
>
> register_kprobes() {
> check_kprobe_address_safe() {
> arch_check_ftrace_location() {
> ftrace_location() {
> lookup_rec() // access memory that has been freed by
> // ftrace_release_mod() !!!
>
> It seems that the ftrace_lock is required when lookuping records in
> ftrace_location(), so is ftrace_location_range().
>
> Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization")
> Signed-off-by: Zheng Yejian <[email protected]>
> ---
> kernel/trace/ftrace.c | 28 ++++++++++++++++++----------
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index da1710499698..838d175709c1 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1581,7 +1581,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
> }
>
> /**
> - * ftrace_location_range - return the first address of a traced location
> + * ftrace_location_range_locked - return the first address of a traced location
> * if it touches the given ip range
> * @start: start of range to search.
> * @end: end of range to search (inclusive). @end points to the last byte
> @@ -1592,7 +1592,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
> * that is either a NOP or call to the function tracer. It checks the ftrace
> * internal tables to determine if the address belongs or not.
> */
> -unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> +static unsigned long ftrace_location_range_locked(unsigned long start, unsigned long end)
> {
> struct dyn_ftrace *rec;
>
> @@ -1603,6 +1603,17 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> return 0;
> }
>
> +unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> +{
> + unsigned long loc;
> +
> + mutex_lock(&ftrace_lock);
> + loc = ftrace_location_range_locked(start, end);
> + mutex_unlock(&ftrace_lock);

I'm not so sure we can take a mutex in all places that call this function.

What about using RCU?

rcu_read_lock();
loc = ftrace_location_range_rcu(start, end);
rcu_read_unlock();

Then in ftrace_release_mod() we can have:

out_unlock:
mutex_unlock();

/* Need to synchronize with ftrace_location() */
if (tmp_pages)
synchronize_rcu();

-- Steve

> +
> + return loc;
> +}
> +
> /**
> * ftrace_location - return the ftrace location
> * @ip: the instruction pointer to check
> @@ -1614,25 +1625,22 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> */
> unsigned long ftrace_location(unsigned long ip)
> {
> - struct dyn_ftrace *rec;
> + unsigned long loc;
> unsigned long offset;
> unsigned long size;
>
> - rec = lookup_rec(ip, ip);
> - if (!rec) {
> + loc = ftrace_location_range(ip, ip);
> + if (!loc) {
> if (!kallsyms_lookup_size_offset(ip, &size, &offset))
> goto out;
>
> /* map sym+0 to __fentry__ */
> if (!offset)
> - rec = lookup_rec(ip, ip + size - 1);
> + loc = ftrace_location_range(ip, ip + size - 1);
> }
>
> - if (rec)
> - return rec->ip;
> -
> out:
> - return 0;
> + return loc;
> }
>
> /**

2024-04-11 01:48:55

by Zheng Yejian

[permalink] [raw]

Subject: Re: [PATCH] ftrace: Fix use-after-free issue in ftrace_location()

On 2024/4/10 23:28, Steven Rostedt wrote:
> On Mon, 1 Apr 2024 20:55:43 +0800
> Zheng Yejian <[email protected]> wrote:
>
>> 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+ #213
>> [...]
>> 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 when lookup_rec() is lookuping ftrace record of
>> an address in some module, and at the same time in ftrace_release_mod(),
>> the memory that saving ftrace records has been freed as that module is
>> being deleted.
>>
>> register_kprobes() {
>> check_kprobe_address_safe() {
>> arch_check_ftrace_location() {
>> ftrace_location() {
>> lookup_rec() // access memory that has been freed by
>> // ftrace_release_mod() !!!
>>
>> It seems that the ftrace_lock is required when lookuping records in
>> ftrace_location(), so is ftrace_location_range().
>>
>> Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization")
>> Signed-off-by: Zheng Yejian <[email protected]>
>> ---
>> kernel/trace/ftrace.c | 28 ++++++++++++++++++----------
>> 1 file changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index da1710499698..838d175709c1 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -1581,7 +1581,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>> }
>>
>> /**
>> - * ftrace_location_range - return the first address of a traced location
>> + * ftrace_location_range_locked - return the first address of a traced location
>> * if it touches the given ip range
>> * @start: start of range to search.
>> * @end: end of range to search (inclusive). @end points to the last byte
>> @@ -1592,7 +1592,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>> * that is either a NOP or call to the function tracer. It checks the ftrace
>> * internal tables to determine if the address belongs or not.
>> */
>> -unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>> +static unsigned long ftrace_location_range_locked(unsigned long start, unsigned long end)
>> {
>> struct dyn_ftrace *rec;
>>
>> @@ -1603,6 +1603,17 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>> return 0;
>> }
>>
>> +unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>> +{
>> + unsigned long loc;
>> +
>> + mutex_lock(&ftrace_lock);
>> + loc = ftrace_location_range_locked(start, end);
>> + mutex_unlock(&ftrace_lock);
>
> I'm not so sure we can take a mutex in all places that call this function.
>
> What about using RCU?
>
> rcu_read_lock();
> loc = ftrace_location_range_rcu(start, end);
> rcu_read_unlock();
>
> Then in ftrace_release_mod() we can have:
>
> out_unlock:
> mutex_unlock();
>
> /* Need to synchronize with ftrace_location() */
> if (tmp_pages)
> synchronize_rcu();
>
> -- Steve

Yes, it is better to use RCU, I'll do it in v2.

--
Thanks
Zheng Yejian

>
>
>> +
>> + return loc;
>> +}
>> +
>> /**
>> * ftrace_location - return the ftrace location
>> * @ip: the instruction pointer to check
>> @@ -1614,25 +1625,22 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>> */
>> unsigned long ftrace_location(unsigned long ip)
>> {
>> - struct dyn_ftrace *rec;
>> + unsigned long loc;
>> unsigned long offset;
>> unsigned long size;
>>
>> - rec = lookup_rec(ip, ip);
>> - if (!rec) {
>> + loc = ftrace_location_range(ip, ip);
>> + if (!loc) {
>> if (!kallsyms_lookup_size_offset(ip, &size, &offset))
>> goto out;
>>
>> /* map sym+0 to __fentry__ */
>> if (!offset)
>> - rec = lookup_rec(ip, ip + size - 1);
>> + loc = ftrace_location_range(ip, ip + size - 1);
>> }
>>
>> - if (rec)
>> - return rec->ip;
>> -
>> out:
>> - return 0;
>> + return loc;
>> }
>>
>> /**
>

2024-04-16 11:24:27

by Zheng Yejian

[permalink] [raw]

Subject: [PATCH v2] ftrace: Fix possible use-after-free issue in ftrace_location()

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 when lookup_rec() is lookuping ftrace record of
an address in ftrace pages of some module, and those ftrace pages may
at the same time being freed in ftrace_release_mod() as the corresponding
module is being deleted:

register_kprobes() {
check_kprobe_address_safe() {
arch_check_ftrace_location() {
ftrace_location() {
lookup_rec() // access memory that has been freed by
// ftrace_release_mod() !!!

To fix it, we hold rcu lock as lookuping ftrace record, and call
synchronize_rcu() before freeing any ftrace pages.

Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization")
Signed-off-by: Zheng Yejian <[email protected]>
---
kernel/trace/ftrace.c | 43 +++++++++++++++++++++++++++----------------
1 file changed, 27 insertions(+), 16 deletions(-)

v2:
- Use RCU lock instead of holding ftrace_lock as suggested by Steve.
Link: https://lore.kernel.org/all/[email protected]/

v1:
- Link: https://lore.kernel.org/all/[email protected]/

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index da1710499698..2b41837a2fac 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1581,7 +1581,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
}

/**
- * ftrace_location_range - return the first address of a traced location
+ * ftrace_location_range_rcu - return the first address of a traced location
* if it touches the given ip range
* @start: start of range to search.
* @end: end of range to search (inclusive). @end points to the last byte
@@ -1592,7 +1592,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
* that is either a NOP or call to the function tracer. It checks the ftrace
* internal tables to determine if the address belongs or not.
*/
-unsigned long ftrace_location_range(unsigned long start, unsigned long end)
+static unsigned long ftrace_location_range_rcu(unsigned long start, unsigned long end)
{
struct dyn_ftrace *rec;

@@ -1603,6 +1603,16 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
return 0;
}

+unsigned long ftrace_location_range(unsigned long start, unsigned long end)
+{
+ unsigned long loc;
+
+ rcu_read_lock();
+ loc = ftrace_location_range_rcu(start, end);
+ rcu_read_unlock();
+ return loc;
+}
+
/**
* ftrace_location - return the ftrace location
* @ip: the instruction pointer to check
@@ -1614,25 +1624,22 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
*/
unsigned long ftrace_location(unsigned long ip)
{
- struct dyn_ftrace *rec;
+ unsigned long loc;
unsigned long offset;
unsigned long size;

- rec = lookup_rec(ip, ip);
- if (!rec) {
+ loc = ftrace_location_range(ip, ip);
+ if (!loc) {
if (!kallsyms_lookup_size_offset(ip, &size, &offset))
goto out;

/* map sym+0 to __fentry__ */
if (!offset)
- rec = lookup_rec(ip, ip + size - 1);
+ loc = ftrace_location_range(ip, ip + size - 1);
}

- if (rec)
- return rec->ip;
-
out:
- return 0;
+ return loc;
}

/**
@@ -6596,6 +6603,7 @@ static int ftrace_process_locs(struct module *mod,
/* We should have used all pages unless we skipped some */
if (pg_unuse) {
WARN_ON(!skipped);
+ synchronize_rcu();
ftrace_free_pages(pg_unuse);
}
return ret;
@@ -6809,6 +6817,8 @@ void ftrace_release_mod(struct module *mod)
out_unlock:
mutex_unlock(&ftrace_lock);

+ if (tmp_page)
+ synchronize_rcu();
for (pg = tmp_page; pg; pg = tmp_page) {

/* Needs to be called outside of ftrace_lock */
@@ -7142,6 +7152,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;
@@ -7183,12 +7194,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;
@@ -7205,6 +7212,10 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
clear_func_from_hashes(func);
kfree(func);
}
+ if (tmp_page) {
+ synchronize_rcu();
+ ftrace_free_pages(tmp_page);
+ }
}

void __init ftrace_free_init_mem(void)
--
2.25.1

2024-04-16 16:02:02

by Markus Elfring

[permalink] [raw]

Subject: Re: [PATCH v2] ftrace: Fix possible use-after-free issue in ftrace_location()

…
> To fix it, we hold rcu lock as lookuping ftrace record, and call
> synchronize_rcu() before freeing any ftrace pages.

I suggest to convert this description into an imperative wording.

Regards,
Markus

2024-04-17 03:32:01

by Zheng Yejian

[permalink] [raw]

Subject: [PATCH v3] ftrace: Fix possible use-after-free issue in ftrace_location()

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().

Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization")
Suggested-by: Steven Rostedt <[email protected]>
Signed-off-by: Zheng Yejian <[email protected]>
---
kernel/trace/ftrace.c | 46 ++++++++++++++++++++++++++++---------------
1 file changed, 30 insertions(+), 16 deletions(-)

v3:
- Complete the commit description and add Suggested-by tag
- Add comments around where synchronize_rcu() is called

v2:
- Link: https://lore.kernel.org/all/[email protected]/
- Use RCU lock instead of holding ftrace_lock as suggested by Steve.
Link: https://lore.kernel.org/all/[email protected]/

v1:
- Link: https://lore.kernel.org/all/[email protected]/

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index da1710499698..e05d3e3dc06a 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1581,7 +1581,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
}

/**
- * ftrace_location_range - return the first address of a traced location
+ * ftrace_location_range_rcu - return the first address of a traced location
* if it touches the given ip range
* @start: start of range to search.
* @end: end of range to search (inclusive). @end points to the last byte
@@ -1592,7 +1592,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
* that is either a NOP or call to the function tracer. It checks the ftrace
* internal tables to determine if the address belongs or not.
*/
-unsigned long ftrace_location_range(unsigned long start, unsigned long end)
+static unsigned long ftrace_location_range_rcu(unsigned long start, unsigned long end)
{
struct dyn_ftrace *rec;

@@ -1603,6 +1603,16 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
return 0;
}

+unsigned long ftrace_location_range(unsigned long start, unsigned long end)
+{
+ unsigned long loc;
+
+ rcu_read_lock();
+ loc = ftrace_location_range_rcu(start, end);
+ rcu_read_unlock();
+ return loc;
+}
+
/**
* ftrace_location - return the ftrace location
* @ip: the instruction pointer to check
@@ -1614,25 +1624,22 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
*/
unsigned long ftrace_location(unsigned long ip)
{
- struct dyn_ftrace *rec;
+ unsigned long loc;
unsigned long offset;
unsigned long size;

- rec = lookup_rec(ip, ip);
- if (!rec) {
+ loc = ftrace_location_range(ip, ip);
+ if (!loc) {
if (!kallsyms_lookup_size_offset(ip, &size, &offset))
goto out;

/* map sym+0 to __fentry__ */
if (!offset)
- rec = lookup_rec(ip, ip + size - 1);
+ loc = ftrace_location_range(ip, ip + size - 1);
}

- if (rec)
- return rec->ip;
-
out:
- return 0;
+ return loc;
}

/**
@@ -6596,6 +6603,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;
@@ -6809,6 +6818,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 */
@@ -7142,6 +7154,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;
@@ -7183,12 +7196,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;
@@ -7205,6 +7214,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)
--
2.25.1

2024-05-02 21:07:13

by Steven Rostedt

[permalink] [raw]

Subject: Re: [PATCH v3] ftrace: Fix possible use-after-free issue in ftrace_location()

On Wed, 17 Apr 2024 11:28:30 +0800
Zheng Yejian <[email protected]> wrote:

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index da1710499698..e05d3e3dc06a 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1581,7 +1581,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
> }
>
> /**
> - * ftrace_location_range - return the first address of a traced location
> + * ftrace_location_range_rcu - return the first address of a traced location

kerneldoc comments are for external functions. You need to move this down
to ftrace_location_range() as here you are commenting a local static function.

But I have to ask, why did you create this static function anyway? There's
only one user of it (the ftrace_location_range()). Why didn't you just
simply add the rcu locking there?

unsigned long ftrace_location_range(unsigned long start, unsigned long end)
{
struct dyn_ftrace *rec;
unsigned long ip = 0;

rcu_read_lock();
rec = lookup_rec(start, end);
if (rec)
ip = rec->ip;
rcu_read_unlock();

return ip;
}

-- Steve

> * if it touches the given ip range
> * @start: start of range to search.
> * @end: end of range to search (inclusive). @end points to the last byte
> @@ -1592,7 +1592,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
> * that is either a NOP or call to the function tracer. It checks the ftrace
> * internal tables to determine if the address belongs or not.
> */
> -unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> +static unsigned long ftrace_location_range_rcu(unsigned long start, unsigned long end)
> {
> struct dyn_ftrace *rec;
>
> @@ -1603,6 +1603,16 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> return 0;
> }
>
> +unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> +{
> + unsigned long loc;
> +
> + rcu_read_lock();
> + loc = ftrace_location_range_rcu(start, end);
> + rcu_read_unlock();
> + return loc;
> +}

2024-05-09 01:39:18

by Zheng Yejian

[permalink] [raw]

Subject: Re: [PATCH v3] ftrace: Fix possible use-after-free issue in ftrace_location()

On 2024/5/3 05:07, Steven Rostedt wrote:
> On Wed, 17 Apr 2024 11:28:30 +0800
> Zheng Yejian <[email protected]> wrote:
>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index da1710499698..e05d3e3dc06a 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -1581,7 +1581,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>> }
>>
>> /**
>> - * ftrace_location_range - return the first address of a traced location
>> + * ftrace_location_range_rcu - return the first address of a traced location
>
> kerneldoc comments are for external functions. You need to move this down
> to ftrace_location_range() as here you are commenting a local static function.

I'll do it in v4.

>
> But I have to ask, why did you create this static function anyway? There's
> only one user of it (the ftrace_location_range()). Why didn't you just
> simply add the rcu locking there?

Yes, the only-one-user function looks ugly.
At first thought that ftrace_location_range() needs to a lock, I just do like that,
no specital reason.

>
> unsigned long ftrace_location_range(unsigned long start, unsigned long end)
> {
> struct dyn_ftrace *rec;
> unsigned long ip = 0;
>
> rcu_read_lock();
> rec = lookup_rec(start, end);
> if (rec)
> ip = rec->ip;
> rcu_read_unlock();
>
> return ip;
> }
>
> -- Steve
>
>
>> * if it touches the given ip range
>> * @start: start of range to search.
>> * @end: end of range to search (inclusive). @end points to the last byte
>> @@ -1592,7 +1592,7 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>> * that is either a NOP or call to the function tracer. It checks the ftrace
>> * internal tables to determine if the address belongs or not.
>> */
>> -unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>> +static unsigned long ftrace_location_range_rcu(unsigned long start, unsigned long end)
>> {
>> struct dyn_ftrace *rec;
>>
>> @@ -1603,6 +1603,16 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>> return 0;
>> }
>>
>> +unsigned long ftrace_location_range(unsigned long start, unsigned long end)
>> +{
>> + unsigned long loc;
>> +
>> + rcu_read_lock();
>> + loc = ftrace_location_range_rcu(start, end);
>> + rcu_read_unlock();
>> + return loc;
>> +}
>

2024-05-09 11:29:00

by Zheng Yejian

[permalink] [raw]

Subject: [PATCH v4] ftrace: Fix possible use-after-free issue in ftrace_location()

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().

Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization")
Suggested-by: Steven Rostedt <[email protected]>
Signed-off-by: Zheng Yejian <[email protected]>
---
kernel/trace/ftrace.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)

v4:
- Simply add rcu locking into ftrace_location_range() as suggested by Steve.
Link: https://lore.kernel.org/linux-trace-kernel/[email protected]/

v3:
- Complete the commit description and add Suggested-by tag
- Add comments around where synchronize_rcu() is called
- Link: https://lore.kernel.org/linux-trace-kernel/[email protected]/

v2:
- Link: https://lore.kernel.org/all/[email protected]/
- Use RCU lock instead of holding ftrace_lock as suggested by Steve.
Link: https://lore.kernel.org/all/[email protected]/

v1:
- Link: https://lore.kernel.org/all/[email protected]/

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index da1710499698..1e12df9bb531 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1595,12 +1595,15 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
unsigned long ftrace_location_range(unsigned long start, unsigned long end)
{
struct dyn_ftrace *rec;
+ unsigned long ip = 0;

+ rcu_read_lock();
rec = lookup_rec(start, end);
if (rec)
- return rec->ip;
+ ip = rec->ip;
+ rcu_read_unlock();

- return 0;
+ return ip;
}

/**
@@ -1614,25 +1617,22 @@ unsigned long ftrace_location_range(unsigned long start, unsigned long end)
*/
unsigned long ftrace_location(unsigned long ip)
{
- struct dyn_ftrace *rec;
+ unsigned long loc;
unsigned long offset;
unsigned long size;

- rec = lookup_rec(ip, ip);
- if (!rec) {
+ loc = ftrace_location_range(ip, ip);
+ if (!loc) {
if (!kallsyms_lookup_size_offset(ip, &size, &offset))
goto out;

/* map sym+0 to __fentry__ */
if (!offset)
- rec = lookup_rec(ip, ip + size - 1);
+ loc = ftrace_location_range(ip, ip + size - 1);
}

- if (rec)
- return rec->ip;
-
out:
- return 0;
+ return loc;
}

/**
@@ -6596,6 +6596,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;
@@ -6809,6 +6811,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 */
@@ -7142,6 +7147,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;
@@ -7183,12 +7189,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;
@@ -7205,6 +7207,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)
--
2.25.1