Messages in this thread | | | Subject | Re: [PATCH] ftrace: Fix NULL pointer dereference in is_ftrace_trampoline when ftrace is dead | From | Yang Jihong <> | Date | Thu, 18 Aug 2022 09:50:40 +0800 |
| |
Hello,
On 2022/8/17 22:41, Steven Rostedt wrote: > On Thu, 4 Aug 2022 10:16:10 +0800 > Yang Jihong <yangjihong1@huawei.com> wrote: > >> @@ -2922,24 +2922,36 @@ int ftrace_startup(struct ftrace_ops *ops, int command) >> ops->flags |= FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_ADDING; >> >> ret = ftrace_hash_ipmodify_enable(ops); >> - if (ret < 0) { >> - /* Rollback registration process */ >> - __unregister_ftrace_function(ops); >> - ftrace_start_up--; >> - ops->flags &= ~FTRACE_OPS_FL_ENABLED; >> - if (ops->flags & FTRACE_OPS_FL_DYNAMIC) >> - ftrace_trampoline_free(ops); >> - return ret; > > This should stay as is. > >> - } >> + if (ret < 0) >> + goto out_rollback_registration; >> >> if (ftrace_hash_rec_enable(ops, 1)) >> command |= FTRACE_UPDATE_CALLS; >> >> ftrace_startup_enable(command); >> >> + /* >> + * If ftrace_startup_enable fails, >> + * we need to rollback registration process. >> + */ >> + if (unlikely(ftrace_disabled)) { >> + ret = -ENODEV; >> + goto out_rollback_registration; > > The only thing to do here is the _unregister_ftrace_function(ops); > And that may not even be safe. > > >> + } >> + >> ops->flags &= ~FTRACE_OPS_FL_ADDING; >> >> return 0; >> + >> +out_rollback_registration: >> + /* Rollback registration process */ >> + __unregister_ftrace_function(ops); >> + ftrace_start_up--; >> + ops->flags &= ~FTRACE_OPS_FL_ENABLED; >> + if (ops->flags & FTRACE_OPS_FL_DYNAMIC) >> + ftrace_trampoline_free(ops); >> + > > When ftrace_disabled is set, ftrace is in an undefined state, and a reboot > should be done ASAP. Because we have no idea what went wrong. It means > something happened that ftrace was not designed for. > > That means, we do not know if the trampoline can still be called or not. > Maybe it enabled some of the functions, but not all. And maybe those > functions call the dynamic trampoline directly. > > Thus, on ftrace_disable being set, only do the bare minimum, as ftrace has > now "shutdown" and will not do any more work. > > Basically, this patch is trying to mitigate a kernel that broke and needs > a reboot immediately. > > -- Steve Thanks for the detailed explanation. If panic_on_warn is not set, FTRACE_WARN_ON{_ONCE} only sets ftrace_disabled, but will not reboot. I think this is to limit the problem to ftrace itself and not spread to other subsystems(I don't know if that's right. If it's not right, please correct it). Because is_ftrace_trampoline is a common and public interface (This interface is called in many places in the kernel). If is_ftrace_trampoline interface is not restricted (for example, just return true if ftrace_disabled is set), the preceding Syzkaller scenario may be triggered when this interface is called.
Therefore, my idea is to restrict the is_ftrace_trampoline or roll back _unregister_ftrace_function when ftrace_disabled is set, so that the interface can be invoked normally. Or keep the current code and do not modify.
Please give me some suggestions that you think are better.
Thanks, Yang
> > >> + return ret; >> } >> >> int ftrace_shutdown(struct ftrace_ops *ops, int command) >> -- > . >
| |