Date: Wed, 13 Nov 2024 14:50:37 +0000
From: Hagar Hemdan <hagarhem@amazon.com>
To: Steven Rostedt <rostedt@goodmis.org>
CC: <stable@vger.kernel.org>, Zheng Yejian <zhengyejian1@huawei.com>,
	<mhiramat@kernel.org>, <mark.rutland@arm.com>,
	<mathieu.desnoyers@efficios.com>
Subject: Re: [PATCH 5.4] ftrace: Fix possible use-after-free issue in
 ftrace_location()
Message-ID: <20241113145037.GA7895@amazon.com>
References: <20241111144445.27428-1-hagarhem@amazon.com>
 <20241112104618.4f2720d8@gandalf.local.home>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20241112104618.4f2720d8@gandalf.local.home>
User-Agent: Mutt/1.5.21 (2010-09-15)

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                       |      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@huawei.com
> > 
> > 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();
> >  		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)
>