linux-kernel - Re: [PATCH 2/2] perf, ftrace: Fix use-after-free in __ftrace_ops_list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210324211123.64d72c02@oasis.local.home>
Date:   Wed, 24 Mar 2021 21:11:23 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Li Huafei <lihuafei1@...wei.com>
Cc:     <mingo@...hat.com>, <jolsa@...hat.com>,
        <linux-kernel@...r.kernel.org>, <yangjihong1@...wei.com>,
        <xukuohai@...wei.com>, <zhangjinhao2@...wei.com>
Subject: Re: [PATCH 2/2] perf, ftrace: Fix use-after-free in
 __ftrace_ops_list_func()

On Wed, 17 Mar 2021 18:25:29 +0800
Li Huafei <lihuafei1@...wei.com> wrote:

> We see the comment of ftrace_ops in include/linux/ftrace.h, which
> actually mentions that for dynamically allocated ftrace_ops, after
> unregistering it should be guaranteed that no user will reference that
> ftrace_ops again, but the current interface unregister_ftrace_function()
> does not seem to guarantee this.
> 
> We add the ftrace_function_unregister_sync() interface to give users
> like perf event a chance to synchronize. The reason we don't do
> synchronization directly in unregister_ftrace_function() is that for
> some users whose registered ftrace_ops is static, synchronization is not
> necessary for them and is performance intensive, so we add a separate
> interface.

You are papering over a real bug then.

Also, synchronize_rcu() is not strong enough. Callbacks can happen that
reference ftrace_ops that are not protected by standard RCU. You need
full synchronize_rcu_tasks_rude() which is much more disruptive that
normal synchronize_rcu().

The code has:

	if (ops->flags & FTRACE_OPS_FL_DYNAMIC) {
		/*
		 * We need to do a hard force of sched synchronization.
		 * This is because we use preempt_disable() to do RCU, but
		 * the function tracers can be called where RCU is not watching
		 * (like before user_exit()). We can not rely on the RCU
		 * infrastructure to do the synchronization, thus we must do it
		 * ourselves.
		 */
		synchronize_rcu_tasks_rude();

		/*
		 * When the kernel is preemptive, tasks can be preempted
		 * while on a ftrace trampoline. Just scheduling a task on
		 * a CPU is not good enough to flush them. Calling
		 * synchronize_rcu_tasks() will wait for those tasks to
		 * execute and either schedule voluntarily or enter user space.
		 */
		if (IS_ENABLED(CONFIG_PREEMPTION))
			synchronize_rcu_tasks();

 free_ops:
		ftrace_trampoline_free(ops);
	}

And what you are saying is that we are not getting there, where the
dynamically allocated perf ops is not set to be DYNAMIC?

That should be set as DYNAMIC if the ops was allocated, and can later
be freed. This code was written specifically to handle perf.

Thus, NACK on the patch. I want to know exactly what went wrong instead
of just saying "but the current interface unregister_ftrace_function()
does not seem to guarantee this", let's actually fix the bug and not just paper over it!