Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] ftrace: Fix NULL pointer dereference in
 is_ftrace_trampoline when ftrace is dead
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     <mingo@redhat.com>, <linux-kernel@vger.kernel.org>
References: <20220804021610.209791-1-yangjihong1@huawei.com>
 <20220817104115.0ec6b90b@gandalf.local.home>
 <dcbc9c43-486d-e045-1e35-4226233981ea@huawei.com>
 <20220817221422.295db316@gandalf.local.home>
From:   Yang Jihong <yangjihong1@huawei.com>
Message-ID: <472bdcc7-c35c-0306-5642-3eaf9061909d@huawei.com>
Date:   Thu, 18 Aug 2022 10:32:17 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <20220817221422.295db316@gandalf.local.home>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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