linux-kernel - Re: [PATCH] ftrace: Fix NULL pointer dereference in is_ftrace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 Aug 2022 10:41:15 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Yang Jihong <yangjihong1@...wei.com>
Cc:     <mingo@...hat.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ftrace: Fix NULL pointer dereference in
 is_ftrace_trampoline when ftrace is dead

On Thu, 4 Aug 2022 10:16:10 +0800
Yang Jihong <yangjihong1@...wei.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

> +	return ret;
>  }
>  
>  int ftrace_shutdown(struct ftrace_ops *ops, int command)
> --