Re: [PATCH] tracing: Have trace event string test handle zero length strings

From: Brian Foster <bfoster@redhat.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-xfs@vger.kernel.org
Subject: Re: [PATCH] tracing: Have trace event string test handle zero length strings
Date: Wed, 23 Mar 2022 10:46:15 -0400	[thread overview]
Message-ID: <Yjsyt/6OeFSRsmcq@bfoster> (raw)
In-Reply-To: <20220323103912.097ad3a8@gandalf.local.home>

On Wed, Mar 23, 2022 at 10:39:12AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> If a trace event has in its TP_printk():
> 
>  "%*.s", len, len ? __get_str(string) : NULL
> 
> It is perfectly valid if len is zero and passing in the NULL.
> Unfortunately, the runtime string check at time of reading the trace sees
> the NULL and flags it as a bad string and produces a WARN_ON().
> 
> Handle this case by passing into the test function if the format has an
> asterisk (star) and if so, if the length is zero, then mark it as safe.
> 
> Link: https://lore.kernel.org/all/YjsWzuw5FbWPrdqq@bfoster/
> 
> Cc: stable@vger.kernel.org
> Reported-by: Brian Foster <bfoster@redhat.com>
> Fixes: 9a6944fee68e2 ("tracing: Add a verifier to check string pointers for trace events")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---

FWIW:

Tested-by: Brian Foster <bfoster@redhat.com>

Thanks for the patch!

>  kernel/trace/trace.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index eb44418574f9..96265a717ca4 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3663,12 +3663,17 @@ static char *trace_iter_expand_format(struct trace_iterator *iter)
>  }
>  
>  /* Returns true if the string is safe to dereference from an event */
> -static bool trace_safe_str(struct trace_iterator *iter, const char *str)
> +static bool trace_safe_str(struct trace_iterator *iter, const char *str,
> +			   bool star, int len)
>  {
>  	unsigned long addr = (unsigned long)str;
>  	struct trace_event *trace_event;
>  	struct trace_event_call *event;
>  
> +	/* Ignore strings with no length */
> +	if (star && !len)
> +		return true;
> +
>  	/* OK if part of the event data */
>  	if ((addr >= (unsigned long)iter->ent) &&
>  	    (addr < (unsigned long)iter->ent + iter->ent_size))
> @@ -3854,7 +3859,7 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
>  		 * instead. See samples/trace_events/trace-events-sample.h
>  		 * for reference.
>  		 */
> -		if (WARN_ONCE(!trace_safe_str(iter, str),
> +		if (WARN_ONCE(!trace_safe_str(iter, str, star, len),
>  			      "fmt: '%s' current_buffer: '%s'",
>  			      fmt, show_buffer(&iter->seq))) {
>  			int ret;
> -- 
> 2.35.1
> 