Re: [PATCH] tracepoint: Fix CFI failures with tp_stub_func
From: Mathieu Desnoyers
Date: Thu Mar 23 2023 - 14:37:11 EST
On 2023-03-23 12:34, Steven Rostedt wrote:
On Thu, 23 Mar 2023 16:27:37 +0000
Mark Rutland <mark.rutland@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxxxxxxxxxx/
]
Ugh. The last thing we need there is more RCU complexity. My brain is still recovering
from fixing the last time the introduction of static calls special-cases ended up subtly
breaking tracepoints.
-- 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; \
Along the lines of my earlier proposal:
https://lore.kernel.org/all/1889971276.46615.1605559047845.JavaMail.zimbra@xxxxxxxxxxxx/
We could reserve (void *)0x1 as a stub value rather than adding an explicit stub function
if we end up skipping the call anyway. The check could be:
#define TP_STUB_FN ((void *)0x1)
if (unlikely(it_func == TP_STUB_FN))
continue;
And we would only need that check for CONFIG_HAVE_STATIC_CALL=y right ?
Thanks,
Mathieu
__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;
}
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com