Linus,

Tracing fix for 6.13:

- Fix trace event string check when dealing with array of strings

  The xe_bo_move event has a field that indexes into an array of
  strings. The TP_fast_assign() added the index into the ring buffer
  and the TP_printk() had a "%s" that referenced the array using the
  index in the ring buffer. This is a legitimate use of "%s" in
  trace events. But this triggered a false positive in the
  test_event_printk() at boot saying that the string was dangerous.

  Change the check to allow arrays using fields in the ring buffer
  as an index to be considered a safe string.


Please pull the latest trace-v6.13-rc5 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace-v6.13-rc5

Tag SHA1: ab6af83399fec949e9ac8072301ec4d81d82a6e9
Head SHA1: afc6717628f959941d7b33728570568b4af1c4b8


Steven Rostedt (1):
      tracing: Have process_string() also allow arrays

----
 kernel/trace/trace_events.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
---------------------------
commit afc6717628f959941d7b33728570568b4af1c4b8
Author: Steven Rostedt <rostedt@goodmis.org>
Date:   Tue Dec 31 00:06:46 2024 -0500

    tracing: Have process_string() also allow arrays
    
    In order to catch a common bug where a TRACE_EVENT() TP_fast_assign()
    assigns an address of an allocated string to the ring buffer and then
    references it in TP_printk(), which can be executed hours later when the
    string is free, the function test_event_printk() runs on all events as
    they are registered to make sure there's no unwanted dereferencing.
    
    It calls process_string() to handle cases in TP_printk() format that has
    "%s". It returns whether or not the string is safe. But it can have some
    false positives.
    
    For instance, xe_bo_move() has:
    
     TP_printk("move_lacks_source:%s, migrate object %p [size %zu] from %s to %s device_id:%s",
                __entry->move_lacks_source ? "yes" : "no", __entry->bo, __entry->size,
                xe_mem_type_to_name[__entry->old_placement],
                xe_mem_type_to_name[__entry->new_placement], __get_str(device_id))
    
    Where the "%s" references into xe_mem_type_to_name[]. This is an array of
    pointers that should be safe for the event to access. Instead of flagging
    this as a bad reference, if a reference points to an array, where the
    record field is the index, consider it safe.
    
    Link: https://lore.kernel.org/all/9dee19b6185d325d0e6fa5f7cbba81d007d99166.camel@sapience.com/
    
    Cc: stable@vger.kernel.org
    Cc: Masami Hiramatsu <mhiramat@kernel.org>
    Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    Link: https://lore.kernel.org/20241231000646.324fb5f7@gandalf.local.home
    Fixes: 65a25d9f7ac02 ("tracing: Add "%s" check in test_event_printk()")
    Reported-by: Genes Lists <lists@sapience.com>
    Tested-by: Gene C <arch@sapience.com>
    Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 1545cc8b49d0..770e7ed91716 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -364,6 +364,18 @@ static bool process_string(const char *fmt, int len, struct trace_event_call *ca
 		s = r + 1;
 	} while (s < e);
 
+	/*
+	 * Check for arrays. If the argument has: foo[REC->val]
+	 * then it is very likely that foo is an array of strings
+	 * that are safe to use.
+	 */
+	r = strstr(s, "[");
+	if (r && r < e) {
+		r = strstr(r, "REC->");
+		if (r && r < e)
+			return true;
+	}
+
 	/*
 	 * If there's any strings in the argument consider this arg OK as it
 	 * could be: REC->field ? "foo" : "bar" and we don't want to get into

On Tue, Dec 31, 2024 at 08:56:22AM -0500, Steven Rostedt wrote:
>
>Linus,
>
>Tracing fix for 6.13:
>
>- Fix trace event string check when dealing with array of strings
>
>  The xe_bo_move event has a field that indexes into an array of
>  strings. The TP_fast_assign() added the index into the ring buffer
>  and the TP_printk() had a "%s" that referenced the array using the
>  index in the ring buffer. This is a legitimate use of "%s" in
>  trace events. But this triggered a false positive in the
>  test_event_printk() at boot saying that the string was dangerous.
>
>  Change the check to allow arrays using fields in the ring buffer
>  as an index to be considered a safe string.
>
>
>Please pull the latest trace-v6.13-rc5 tree, which can be found at:
>
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
>trace-v6.13-rc5

You might have forgotten to push the tag :)

-- 
Thanks,
Sasha

On Tue, 31 Dec 2024 09:09:12 -0500
Sasha Levin <sashal@kernel.org> wrote:

> >  git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
> >trace-v6.13-rc5  
> 
> You might have forgotten to push the tag :)
> 

You're quick. I usually push either just before or just after sending this.

I'm currently playing with my procmail to see how I can modify my
accounting, and it's not working as I want it to. Which distracted me in my
workflow. :-p

-- Steve