Re: [PATCH] tracing: Have process_string() also allow arrays

[Google]

On Tue, 31 Dec 2024 00:06:46 -0500
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> From: Steven Rostedt <rostedt@xxxxxxxxxxx>
> 
> 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.

OK, at least if foo[] is accessible in TP_printk, it should be a global
array. So I think the below works.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>

Thanks,

> 
> Link: https://lore.kernel.org/all/9dee19b6185d325d0e6fa5f7cbba81d007d99166.camel@xxxxxxxxxxxx/
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 65a25d9f7ac02 ("tracing: Add "%s" check in test_event_printk()")
> Reported-by: Genes Lists <lists@xxxxxxxxxxxx>
> Tested-by: Gene C <arch@xxxxxxxxxxxx>
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> ---
>  kernel/trace/trace_events.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> 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
> -- 
> 2.45.2
> 


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>