Re: [PATCH] ftrace: Fix NULL pointer dereference in is_ftrace_trampoline when ftrace is dead

[Yang Jihong <yangjihong1@huawei.com>]

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)
>> -- 
> .
>