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/18 10:14, Steven Rostedt wrote:
> On Thu, 18 Aug 2022 09:50:40 +0800
> Yang Jihong <yangjihong1@huawei.com> wrote:
>
>> Thanks for the detailed explanation.
>> If panic_on_warn is not set, FTRACE_WARN_ON{_ONCE} only sets
>> ftrace_disabled, but will not reboot.
>
> Correct. But whenever there's a WARN_ON() the administrator of the machine
> should think about rebooting it ASAP. That's because all WARN_ON()s are
> suppose to only happen when the system does something that was not
> expected, putting it into an inconsistent state. And could be a dangerous
> one. This is why all WARN_ON()s that are triggered are considered bugs and
> must be fixed.
>
>
>> 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).
>
> Yes, the ftrace_disable means that ftrace just found itself in a situation
> that it does not understand, and nothing can be trusted. As ftrace modifies
> kernel code, it basically stops everything and WARNs about it. Because
> anything else it does can make things worse.
>
>> 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.
>
> If ftrace_disabled is set, then any operations should fail, and any tests
> should fail with it.
>
>>
>> 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.
>
> Once ftrace_disabled is set, none of its interfaces should perform
> normally.
>
> But you reported that you could hit a NULL pointer from the
> is_ftrace_trampoline() which was caused by the failure adding the dynamic
> trampoline, and then the ops is on the list but later freed.
>
> My suggestion above is to just call _unregister_ftrace_function(ops) to
> take it off the list and prevent the NULL pointer.
>
> Doesn't that fix the bug?
>
> I don't want to totally roll it back and free the trampoline, because those
> actions could cause further damage, depending on the failed state ftrace is
> in.
OK, I understand, and will be modified in this way in next version.

Thanks,
Yang

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