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

[Yang Jihong]

Hello,

PING.

Regards,
Yang
.

On 2022/8/4 10:16, Yang Jihong wrote:
> ftrace_startup does not remove ops from ftrace_ops_list when
> ftrace_startup_enable fails:
>
> register_ftrace_function
>    ftrace_startup
>      __register_ftrace_function
>        ...
>        add_ftrace_ops(&ftrace_ops_list, ops)
>        ...
>      ...
>      ftrace_startup_enable // if ftrace failed to modify, ftrace_disabled is set to 1
>      ...
>    return 0 // ops is in the ftrace_ops_list.
>
> When ftrace_disabled = 1, unregister_ftrace_function simply returns without doing anything:
> unregister_ftrace_function
>    ftrace_shutdown
>      if (unlikely(ftrace_disabled))
>              return -ENODEV;  // return here, __unregister_ftrace_function is not executed,
>                               // as a result, ops is still in the ftrace_ops_list
>      __unregister_ftrace_function
>      ...
>
> If ops is dynamically allocated, it will be free later, in this case,
> is_ftrace_trampoline accesses NULL pointer:
>
> is_ftrace_trampoline
>    ftrace_ops_trampoline
>      do_for_each_ftrace_op(op, ftrace_ops_list) // OOPS! op may be NULL!
>
> Syzkaller reports as follows:
> [ 1203.506103] BUG: kernel NULL pointer dereference, address: 000000000000010b
> [ 1203.508039] #PF: supervisor read access in kernel mode
> [ 1203.508798] #PF: error_code(0x0000) - not-present page
> [ 1203.509558] PGD 800000011660b067 P4D 800000011660b067 PUD 130fb8067 PMD 0
> [ 1203.510560] Oops: 0000 [#1] SMP KASAN PTI
> [ 1203.511189] CPU: 6 PID: 29532 Comm: syz-executor.2 Tainted: G    B   W         5.10.0 #8
> [ 1203.512324] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
> [ 1203.513895] RIP: 0010:is_ftrace_trampoline+0x26/0xb0
> [ 1203.514644] Code: ff eb d3 90 41 55 41 54 49 89 fc 55 53 e8 f2 00 fd ff 48 8b 1d 3b 35 5d 03 e8 e6 00 fd ff 48 8d bb 90 00 00 00 e8 2a 81 26 00 <48> 8b ab 90 00 00 00 48 85 ed 74 1d e8 c9 00 fd ff 48 8d bb 98 00
> [ 1203.518838] RSP: 0018:ffffc900012cf960 EFLAGS: 00010246
> [ 1203.520092] RAX: 0000000000000000 RBX: 000000000000007b RCX: ffffffff8a331866
> [ 1203.521469] RDX: 0000000000000000 RSI: 0000000000000008 RDI: 000000000000010b
> [ 1203.522583] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff8df18b07
> [ 1203.523550] R10: fffffbfff1be3160 R11: 0000000000000001 R12: 0000000000478399
> [ 1203.524596] R13: 0000000000000000 R14: ffff888145088000 R15: 0000000000000008
> [ 1203.525634] FS:  00007f429f5f4700(0000) GS:ffff8881daf00000(0000) knlGS:0000000000000000
> [ 1203.526801] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1203.527626] CR2: 000000000000010b CR3: 0000000170e1e001 CR4: 00000000003706e0
> [ 1203.528611] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 1203.529605] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
> Therefore, when ftrace_startup_enable fails, we need to rollback registration
> process and remove ops from ftrace_ops_list.
>
> Signed-off-by: Yang Jihong <yangjihong1@xxxxxxxxxx>
> ---
>   kernel/trace/ftrace.c | 30 +++++++++++++++++++++---------
>   1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 601ccf1b2f09..63bf7b67ab2e 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -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;
> -	}
> +	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;
> +	}
> +
>   	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);
> +
> +	return ret;
>   }
>   
>   int ftrace_shutdown(struct ftrace_ops *ops, int command)