linux-kernel - Re: [PATCH 2/2] ftrace: setup correct flags before replace code of module rec

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]

Message-ID: <20200728090211.4c94a14d@oasis.local.home>
Date:   Tue, 28 Jul 2020 09:02:11 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Chengming Zhou <zhouchengming@...edance.com>
Cc:     mingo@...hat.com, linux-kernel@...r.kernel.org,
        songmuchun@...edance.com
Subject: Re: [PATCH 2/2] ftrace: setup correct flags before replace code of
 module rec

On Tue, 28 Jul 2020 18:27:20 +0800
Chengming Zhou <zhouchengming@...edance.com> wrote:

> When module loaded and enabled, we will use __ftrace_replace_code
> for module if any ftrace_ops referenced it found. But we will get
> wrong ftrace_addr for module rec in ftrace_get_addr_new, because
> rec->flags has not been setup correctly.
> So setup correct rec->flags when we call referenced_filters to find
> ftrace_ops references it.

This is somewhat correct ;-)

> 
> Signed-off-by: Chengming Zhou <zhouchengming@...edance.com>
> Signed-off-by: Muchun Song <songmuchun@...edance.com>
> ---
>  kernel/trace/ftrace.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index fca01a168ae5..00087dea0174 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6190,8 +6190,17 @@ static int referenced_filters(struct dyn_ftrace *rec)
>  	int cnt = 0;
>  
>  	for (ops = ftrace_ops_list; ops != &ftrace_list_end; ops = ops->next) {
> -		if (ops_references_rec(ops, rec))
> -		    cnt++;
> +		if (ops_references_rec(ops, rec)) {

> +			cnt++;
> +			if (ops->flags & FTRACE_OPS_FL_DIRECT)
> +				rec->flags |= FTRACE_FL_DIRECT;

The above should be:

			if (WARN_ON_ONCE(ops->flags & FTRACE_OPS_FL_DIRECT))
				continue;
			cnt++;

The direct flag is *very* special, and should not be set automatically
like this.

Probably should add the same kind of warning and skip for
FTRACE_OPS_FL_IPMODIFY.

> +			if (ops->flags & FTRACE_OPS_FL_SAVE_REGS)
> +				rec->flags |= FTRACE_FL_REGS;

The above is definitely a bug fix. I'm thinking this patch should be
broken up into two. One with just this update (and the clear below),
and the rest later. As this should be backported to stable.

> +			if (cnt == 1 && ops->trampoline)
> +				rec->flags |= FTRACE_FL_TRAMP;
> +			else
> +				rec->flags &= ~FTRACE_FL_TRAMP;

The above is correct, but not critical that it would need to be
backported.

Thanks!

-- Steve

> +		}
>  	}
>  
>  	return cnt;
> @@ -6373,7 +6382,8 @@ void ftrace_module_enable(struct module *mod)
>  			cnt += referenced_filters(rec);
>  
>  		/* This clears FTRACE_FL_DISABLED */
> -		rec->flags = cnt;
> +		rec->flags &= ~FTRACE_FL_DISABLED;
> +		rec->flags += cnt;
>  
>  		if (ftrace_start_up && cnt) {
>  			int failed = __ftrace_replace_code(rec, 1);