lkml.org 
[lkml]   [2022]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] ftrace: Fix NULL pointer dereference in is_ftrace_trampoline when ftrace is dead
From
Date
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)
>> --
> .
>

\
 
 \ /
  Last update: 2022-08-18 03:51    [W:0.099 / U:0.688 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site