hist_register_trigger() puts the trigger on the global named_triggers
list in cmd_ops->init(), and only then sets the trace clock:
if (data->cmd_ops->init) {
ret = data->cmd_ops->init(data);
if (ret < 0)
goto out;
}
if (hist_data->enable_timestamps) {
ret = tracing_set_clock(file->tr, hist_data->attrs->clock);
if (ret) {
hist_err(tr, HIST_ERR_SET_CLOCK_FAIL, errpos(clock));
goto out;
}
The clock string is not checked anywhere before that call, so a named
trigger using common_timestamp with an unknown clock fails after it has
already become findable. event_hist_trigger_parse() then frees it
without taking it off the list, and the next lookup by name reads the
freed object:
~# cd /sys/kernel/tracing/events/sched/sched_switch
~# echo 'hist:name=foo:keys=common_pid:ts=common_timestamp:clock=bogus' >
trigger
bash: echo: write error: Invalid argument
~# echo 'hist:name=foo:keys=common_pid' > trigger
BUG: KASAN: slab-use-after-free in find_named_trigger+0xac/0xc0
Read of size 8 at addr ffff88800915d760 by task init/1
find_named_trigger+0xac/0xc0
hist_register_trigger+0xc1/0x900
event_hist_trigger_parse+0x3146/0x6af0
event_trigger_write+0xce/0x160
Freed by task 63:
kfree+0x154/0x420
trigger_kthread_fn+0xfd/0x160
Set the clock before the trigger is registered, so that nothing which
can fail runs after it is published, the way commit 6f86bdeab633
("tracing: Fix bad hist from corrupting named_triggers list") moved the
registration below the rest of the setup.
tracing_set_filter_buffering() is reference counted, so the init failure
path has to drop the reference that the clock block now takes first.
Fixes: a4072fe85ba3 ("tracing: Add a clock attribute for hist triggers")
Cc: [email protected]
Signed-off-by: Donggeun Yoo <[email protected]>
---
Reproduced on x86_64 under KASAN_INLINE on v7.3-rc2, with an initramfs
that does the two writes above from init and then waits for the deferred
free. Without the patch the second write reports the slab-use-after-free
quoted above; with it there is no report, and each rejected clock leaves
its own entry in tracing/error_log instead of only the first attempt
getting that far.
Same kernel and initramfs, selftests/ftrace test.d/trigger before and
after: 45 results, identical item by item (32 passed, 3 failed, 2
unresolved, 8 unsupported). The failures and the unresolved results are
there without the patch as well.
kernel/trace/trace_events_hist.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 963e0d6b61fd..6c628415468a 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6643,12 +6643,6 @@ static int hist_register_trigger(char *glob,
data->cmd_ops = cmd_ops;
}
- if (data->cmd_ops->init) {
- ret = data->cmd_ops->init(data);
- if (ret < 0)
- goto out;
- }
-
if (hist_data->enable_timestamps) {
char *clock = hist_data->attrs->clock;
@@ -6661,6 +6655,15 @@ static int hist_register_trigger(char *glob,
tracing_set_filter_buffering(file->tr, true);
}
+ if (data->cmd_ops->init) {
+ ret = data->cmd_ops->init(data);
+ if (ret < 0) {
+ if (hist_data->enable_timestamps)
+ tracing_set_filter_buffering(file->tr, false);
+ goto out;
+ }
+ }
+
if (named_data) {
remove_hist_vars(hist_data);
destroy_hist_data(hist_data);
base-commit: df2908090cda368b01ff43709f51890076c56157
--
2.53.0
