Re: [PATCH] tracepoint: Fix CFI failures with tp_stub_func

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 23 Mar 2023 16:27:37 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Mar 23, 2023 at 01:45:34PM +0000, Mark Rutland wrote:
> > 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:  
> 
> > > > 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.  
> 
> Along those lines (and as Peter also suggested over IRC), would something like
> the below be preferable?

So we had this discussion a while ago:

See befe6d946551 ("tracepoint: Do not fail unregistering a probe due to memory failure")

Where I believe one answer was to use NULL instead of a stub.

I have to go back and re-read that thread. Mathieu was involved with all
this too.

And as I mentioned in my other reply. There was a more complex solution
that could handle this if the stub solution ended up being an issue.

Repeated again so Mathieu doesn't have to search for it.

    [ Note, this version does use undefined compiler behavior (assuming that
      a stub function with no parameters or return, can be called by a location
      that thinks it has parameters but still no return value. Static calls
      do the same thing, so this trick is not without precedent.

      There's another solution that uses RCU tricks and is more complex, but
      can be an alternative if this solution becomes an issue.

      Link: https://lore.kernel.org/lkml/20210127170721.58bce7cc@gandalf.local.home/
    ]

-- Steve


> 
> Mark.
> 
> ---->8----  
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 6811e43c1b5c..b8017e906049 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -33,6 +33,8 @@ struct trace_eval_map {
>  
>  #define TRACEPOINT_DEFAULT_PRIO	10
>  
> +void tp_stub_func(void);
> +
>  extern struct srcu_struct tracepoint_srcu;
>  
>  extern int
> @@ -324,6 +326,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  		if (it_func_ptr) {					\
>  			do {						\
>  				it_func = READ_ONCE((it_func_ptr)->func); \
> +				if (it_func == tp_stub_func)		\
> +					continue;			\
>  				__data = (it_func_ptr)->data;		\
>  				((void(*)(void *, proto))(it_func))(__data, args); \
>  			} while ((++it_func_ptr)->func);		\
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 8d1507dd0724..dcf5a637429f 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -99,7 +99,7 @@ struct tp_probes {
>  };
>  
>  /* Called in removal of a func but failed to allocate a new tp_funcs */
> -static void tp_stub_func(void)
> +void tp_stub_func(void)
>  {
>  	return;
>  }