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 16:27:37 +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: <ZBx9+ZyiF6LoKbPr@FVFF77S0Q05N.cambridge.arm.com>
References: <20230323114012.2162285-1-mark.rutland@arm.com>
 <20230323085321.0f8d1b98@gandalf.local.home>
 <ZBxX/uu/s5IKBQOw@FVFF77S0Q05N.cambridge.arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZBxX/uu/s5IKBQOw@FVFF77S0Q05N.cambridge.arm.com>
Precedence: bulk

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?

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;
 }