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;
Date:   Thu, 23 Mar 2023 13:45:34 +0000
From:   Mark Rutland <mark.rutland@arm.com>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, mhiramat@kernel.org,
        "Jose E. Marchesi" <jose.marchesi@oracle.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Sami Tolvanen <samitolvanen@google.com>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] tracepoint: Fix CFI failures with tp_stub_func
Message-ID: <ZBxX/uu/s5IKBQOw@FVFF77S0Q05N.cambridge.arm.com>
References: <20230323114012.2162285-1-mark.rutland@arm.com>
 <20230323085321.0f8d1b98@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230323085321.0f8d1b98@gandalf.local.home>
Precedence: bulk

[adding Sami and Peter]

On Thu, Mar 23, 2023 at 08:53:21AM -0400, Steven Rostedt wrote:
> On Thu, 23 Mar 2023 11:40:12 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > When CLANG_CFI is in use, using tracing will occasionally result in
> > CFI failures, e.g.
> > 
> > | CFI failure at syscall_trace_enter+0x66c/0x7d0 (target: tp_stub_func+0x0/0x2c; expected type: 0x4877830c)
> > | Internal error: Oops - CFI: 00000000f200823a [#1] PREEMPT SMP
> > | CPU: 2 PID: 118 Comm: klogd Not tainted 6.3.0-rc3-00002-gc242ea6f2f98 #2
> > | Hardware name: linux,dummy-virt (DT)
> > | pstate: 30400005 (nzCV daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > | pc : syscall_trace_enter+0x66c/0x7d0
> > | lr : syscall_trace_enter+0x5e8/0x7d0
> > | sp : ffff800015ce7d80
> > | x29: ffff800015ce7d80 x28: ffff000017538000 x27: 0000000000000003
> > | x26: ffff8000084c9454 x25: ffff00001182bd10 x24: dfff800000000000
> > | x23: 1fffe00002ea7001 x22: ffff00001182bd10 x21: ffff000017538008
> > | x20: ffff000017538000 x19: ffff800015ce7eb0 x18: 0000000000000000
> > | x17: 000000004877830c x16: 00000000a540670c x15: 0000000000000000
> > | x14: 0000000000000000 x13: 0000000000000000 x12: ff80800008039d8c
> > | x11: ff80800008039dd0 x10: 0000000000000000 x9 : 0000000000000000
> > | x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000000000
> > | x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> > | x2 : 00000000000000ce x1 : ffff800015ce7eb0 x0 : ffff800013d55000
> > | Call trace:
> > |  syscall_trace_enter+0x66c/0x7d0
> > |  el0_svc_common+0x1dc/0x268
> > |  do_el0_svc+0x70/0x1a4
> > |  el0_svc+0x58/0x14c
> > |  el0t_64_sync_handler+0x84/0xf0
> > |  el0t_64_sync+0x190/0x194
> > | Code: 72906191 72a90ef1 6b11021f 54000040 (d4304740)
> > | ---[ end trace 0000000000000000 ]---
> > 
> > This happens because the function prototype of tp_sub_func doesn't match
> > the prototype of the tracepoint function. As each tracepoint may have a
> > distinct prototype, it's not possible to share a common stub function.
> 
> I hate C!
> 
> > 
> > Avoid this by creating a stub function for each tracepoint with the
> > correct prototype, and using these rather than tp_stub_func.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org
> > ---
> >  include/linux/tracepoint-defs.h |  1 +
> >  include/linux/tracepoint.h      |  5 +++++
> >  kernel/tracepoint.c             | 31 ++++++++++++++-----------------
> >  3 files changed, 20 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> > index e7c2276be33eb..a306ac7255220 100644
> > --- a/include/linux/tracepoint-defs.h
> > +++ b/include/linux/tracepoint-defs.h
> > @@ -35,6 +35,7 @@ struct tracepoint {
> >  	struct static_call_key *static_call_key;
> >  	void *static_call_tramp;
> >  	void *iterator;
> > +	void *stub;
> >  	int (*regfunc)(void);
> >  	void (*unregfunc)(void);
> >  	struct tracepoint_func __rcu *funcs;
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 6811e43c1b5c2..1640926441910 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -303,6 +303,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> >  	__section("__tracepoints_strings") = #_name;			\
> >  	extern struct static_call_key STATIC_CALL_KEY(tp_func_##_name);	\
> >  	int __traceiter_##_name(void *__data, proto);			\
> > +	void __tracestub_##_name(void *, proto);			\
> >  	struct tracepoint __tracepoint_##_name	__used			\
> >  	__section("__tracepoints") = {					\
> >  		.name = __tpstrtab_##_name,				\
> > @@ -310,6 +311,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> >  		.static_call_key = &STATIC_CALL_KEY(tp_func_##_name),	\
> >  		.static_call_tramp = STATIC_CALL_TRAMP_ADDR(tp_func_##_name), \
> >  		.iterator = &__traceiter_##_name,			\
> > +		.stub = &__tracestub_##_name,				\
> >  		.regfunc = _reg,					\
> >  		.unregfunc = _unreg,					\
> >  		.funcs = NULL };					\
> > @@ -330,6 +332,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> >  		}							\
> >  		return 0;						\
> >  	}								\
> > +	void __tracestub_##_name(void *__data, proto)			\
> > +	{								\
> > +	}								\
> 
> I purposely did not do this because this adds over a thousand stub
> functions! It adds one for *every* tracepoint (and that is a superset of
> trace events).
> 
> Is there some other way we could do this?
> 
> C really really needs a way to make a generic void do_nothing(...) function!
> 
> I added some compiler folks to the Cc to hear our grievances.

I pulled in Sami, who did much of the kCFI work, and PeterZ too...

We can't have a generic function that's compatible will all function
prototypes, since that mechanism would undermine the CFI scheme. Either callers
would always have to omit the check, or we're have to have a special "always
compatible" type hash, and both would be gigantic targets for attack.

Can we avoid the stub entirely? e.g. make hte call conditional on the func
pointer not being some bad value (e.g. like the error pointers?). That way we
could avoid the call, and we wouldn't need the stub implementation.

Another option would be to make the tracepoint function prototypes *actually*
compatible. If each tracepoint had something like:

struct tracepoint_##_name##_data {
	unsigned long arg;
	void *ptr;
	char whatver[];
};

... then we could have tracepoint functions consistently take a void pointer to
that, and extract the args. We can wrap that in something like the syscall
wrappers so that we don't have to write that manually.

... or maybe we can somehow drop the stubs into a magic section and deduplicate
them?

Thanks.
Mark.

> 
> -- Steve
> 
> 
> >  	DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);
> >  
> >  #define DEFINE_TRACE(name, proto, args)		\
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index 8d1507dd07246..d9186629a0095 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
> > @@ -98,12 +98,6 @@ struct tp_probes {
> >  	struct tracepoint_func probes[];
> >  };
> >  
> > -/* Called in removal of a func but failed to allocate a new tp_funcs */
> > -static void tp_stub_func(void)
> > -{
> > -	return;
> > -}
> > -
> >  static inline void *allocate_probes(int count)
> >  {
> >  	struct tp_probes *p  = kmalloc(struct_size(p, probes, count),
> > @@ -177,13 +171,15 @@ static void debug_print_probes(struct tracepoint_func *funcs)
> >  }
> >  
> >  static struct tracepoint_func *
> > -func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> > +func_add(struct tracepoint *tp, struct tracepoint_func **funcs,
> > +	 struct tracepoint_func *tp_func,
> >  	 int prio)
> >  {
> >  	struct tracepoint_func *old, *new;
> >  	int iter_probes;	/* Iterate over old probe array. */
> >  	int nr_probes = 0;	/* Counter for probes */
> >  	int pos = -1;		/* Insertion position into new array */
> > +	void *stub_func = tp->stub;
> >  
> >  	if (WARN_ON(!tp_func->func))
> >  		return ERR_PTR(-EINVAL);
> > @@ -193,7 +189,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> >  	if (old) {
> >  		/* (N -> N+1), (N != 0, 1) probes */
> >  		for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> > -			if (old[iter_probes].func == tp_stub_func)
> > +			if (old[iter_probes].func == stub_func)
> >  				continue;	/* Skip stub functions. */
> >  			if (old[iter_probes].func == tp_func->func &&
> >  			    old[iter_probes].data == tp_func->data)
> > @@ -208,7 +204,7 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> >  	if (old) {
> >  		nr_probes = 0;
> >  		for (iter_probes = 0; old[iter_probes].func; iter_probes++) {
> > -			if (old[iter_probes].func == tp_stub_func)
> > +			if (old[iter_probes].func == stub_func)
> >  				continue;
> >  			/* Insert before probes of lower priority */
> >  			if (pos < 0 && old[iter_probes].prio < prio)
> > @@ -229,11 +225,12 @@ func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
> >  	return old;
> >  }
> >  
> > -static void *func_remove(struct tracepoint_func **funcs,
> > -		struct tracepoint_func *tp_func)
> > +static void *func_remove(struct tracepoint *tp, struct tracepoint_func **funcs,
> > +			 struct tracepoint_func *tp_func)
> >  {
> >  	int nr_probes = 0, nr_del = 0, i;
> >  	struct tracepoint_func *old, *new;
> > +	void *stub_func = tp->stub;
> >  
> >  	old = *funcs;
> >  
> > @@ -246,7 +243,7 @@ static void *func_remove(struct tracepoint_func **funcs,
> >  		for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
> >  			if ((old[nr_probes].func == tp_func->func &&
> >  			     old[nr_probes].data == tp_func->data) ||
> > -			    old[nr_probes].func == tp_stub_func)
> > +			    old[nr_probes].func == stub_func)
> >  				nr_del++;
> >  		}
> >  	}
> > @@ -269,7 +266,7 @@ static void *func_remove(struct tracepoint_func **funcs,
> >  			for (i = 0; old[i].func; i++) {
> >  				if ((old[i].func != tp_func->func ||
> >  				     old[i].data != tp_func->data) &&
> > -				    old[i].func != tp_stub_func)
> > +				    old[i].func != stub_func)
> >  					new[j++] = old[i];
> >  			}
> >  			new[nr_probes - nr_del].func = NULL;
> > @@ -277,12 +274,12 @@ static void *func_remove(struct tracepoint_func **funcs,
> >  		} else {
> >  			/*
> >  			 * Failed to allocate, replace the old function
> > -			 * with calls to tp_stub_func.
> > +			 * with calls to stub_func.
> >  			 */
> >  			for (i = 0; old[i].func; i++) {
> >  				if (old[i].func == tp_func->func &&
> >  				    old[i].data == tp_func->data)
> > -					WRITE_ONCE(old[i].func, tp_stub_func);
> > +					WRITE_ONCE(old[i].func, stub_func);
> >  			}
> >  			*funcs = old;
> >  		}
> > @@ -335,7 +332,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
> >  
> >  	tp_funcs = rcu_dereference_protected(tp->funcs,
> >  			lockdep_is_held(&tracepoints_mutex));
> > -	old = func_add(&tp_funcs, func, prio);
> > +	old = func_add(tp, &tp_funcs, func, prio);
> >  	if (IS_ERR(old)) {
> >  		WARN_ON_ONCE(warn && PTR_ERR(old) != -ENOMEM);
> >  		return PTR_ERR(old);
> > @@ -400,7 +397,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
> >  
> >  	tp_funcs = rcu_dereference_protected(tp->funcs,
> >  			lockdep_is_held(&tracepoints_mutex));
> > -	old = func_remove(&tp_funcs, func);
> > +	old = func_remove(tp, &tp_funcs, func);
> >  	if (WARN_ON_ONCE(IS_ERR(old)))
> >  		return PTR_ERR(old);
> >  
>