[0/4] tracing: Replace trace_check_printf() with ignore_event()

Message

Steven Rostedt Dec. 17, 2024, 2:41 a.m. UTC

A bug was found where if the hash-ptr option was disabled, it would
pass the iter->fmt temp buffer to the trace_check_vprintf() function
that tests to make sure the "%s" strings do not point to non static
strings or strings in the ring buffer. The problem with that was
the trace_check_vprintf() used the iter->fmt as a temp buffer itself
and would overwrite the content of what it was processing.

On the git pull request[1], Linus did not take too kindly for the
way trace_check_vprintf() was using va_list. It relied on implementation
details of the compiler and would not even work if the compiler used
a copy to pass the va_list and not by reference.

I didn't like that code myself when I first wrote it, but I was tired
of fixing the "%s" bugs that were constantly reported as a bug in
the tracing sub system. Using the va_list was the only method I knew
at the time.

As Linus stated he wanted that removed regardless, I thought hard on
another approach, which leads to this solution. The test_event_printk()
works when the event is registered and it reads the print format as
well as the arguments that are passed to it (what's printed in the
format files), and can mostly figure out if the dereferenced pointers
in the print format are safe or not. Most "%s" in the trace events
use helper macros like __get_str() which can also be verified at boot.
The only ones that can not be, are a few events (which includes the
RCU events, some IPI events, and some others). In this case, we can
tag the event with a new flag TRACE_EVENT_FL_TEST_STR, and also the
fields that contain the pointers that we do not trust with a new
"needs_test" flag.

Now, instead of parsing the print format via vsnprintf() va_list hacks
at runtime, before printing the event, it calls a new function
ignore_event() that checks if the event has the TEST_STR flag set. If so, it
then iterates the event's field structures and for every field that has a
"needs_test" flag set, it will read the event buffer, getting the pointer
from the field structure's offset field, and then checks if that pointer is
"safe". If all the fields marked with "needs_test" are safe, the
ignore_event() function returns false, but if it finds anything that is
not safe, it returns true, triggers a WARN_ON() and writes into the
ring buffer that the event's field points to non-safe memory. In this case
the event's print format is never used.

The ignore_event() makes the trace_check_printf() obsolete and that function
is removed.

This passed various tests, but I'm going to run it through more tests
as well as my standard tests before pushing. I'll create an event that
uses a bad pointer to make sure it triggers (I hacked it up to make
the pointer it calculates from the field to be off by 4 and it did
trigger. But I still want to test it on a real "bad" event).

[1] https://lore.kernel.org/all/[e-mail]/

Steven Rostedt (4):
      tracing: Fix test_event_printk() to process entire print argument
      tracing: Add missing helper functions in event pointer dereference check
      tracing: Add "%s" check in test_event_printk()
      tracing: Check "%s" dereference via the field and not the TP_printk format

----
 include/linux/trace_events.h |   6 +-
 kernel/trace/trace.c         | 255 +++++++++----------------------------------
 kernel/trace/trace.h         |   6 +-
 kernel/trace/trace_events.c  | 225 +++++++++++++++++++++++++++++---------
 kernel/trace/trace_output.c  |   6 +-
 5 files changed, 242 insertions(+), 256 deletions(-)