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 10:32:17 +0800 |
| |
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
| |