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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yjsyt/6OeFSRsmcq@bfoster>
Date:   Wed, 23 Mar 2022 10:46:15 -0400
From:   Brian Foster <bfoster@...hat.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-xfs@...r.kernel.org
Subject: Re: [PATCH] tracing: Have trace event string test handle zero length
 strings

On Wed, Mar 23, 2022 at 10:39:12AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@...dmis.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@...r.kernel.org
> Reported-by: Brian Foster <bfoster@...hat.com>
> Fixes: 9a6944fee68e2 ("tracing: Add a verifier to check string pointers for trace events")
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---

FWIW:

Tested-by: Brian Foster <bfoster@...hat.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
>