Re: [PATCH v2 5/9] tracing/filters: Fix function event operand parsing in parse_pred()

[Masami Hiramatsu (Google)]

On Mon, 26 Oct 2026 16:32:57 -0400, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Sun, 25 Oct 2026 17:37:56 +0900
> "Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > 
> > In parse_pred(), the ftrace_event_is_function(call) branch has several
> > bugs when parsing the string operand:
> > 
> > 1. The loop 'for (i++; str[i]; i++)' unconditionally increments 'i' before
> >    checking whether str[i] is the NUL terminator. When the operand is empty
> >    (e.g. 'ip == '), this advances 'i' past the NUL terminator and reads
> >    out-of-bounds memory until another zero byte is encountered.
> 
> Can't this part simply be fixed with:
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 5e74d2dae40a..6a14c964c115 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -1733,6 +1733,11 @@ static int parse_pred(const char *str, void *data,
>  	while (isspace(str[i]))
>  		i++;
>  
> +	if (!str[i]) {
> +		parse_error(pe, FILT_ERR_INVALID_FILTER, pos + i);
> +		goto err_free;
> +	}
> +
>  	s = i;
>  
>  	pred = kzalloc_obj(*pred);

Yeah, looks good the me. 

> 
> > 
> > 2. When the operand is quoted (q != 0), the loop breaks with str[i] at the
> >    closing quote, but 'i' is never incremented past it (unlike the regular
> >    string literal parser which does 'i++'). Furthermore, it does not check
> >    for a missing closing quote (!str[i]). Returning 'i' at the closing
> >    quote causes predicate_parse() to immediately re-invoke parse_pred() on
> >    the closing quote and fail with FILT_ERR_TOO_MANY_PREDS.
> 
> This is a separate bug and needs to be a separate patch.

OK. Let's make it a separate patch.

> 
> > 
> > 3. If the operand length is 0 (e.g. 'ip == )' or 'ip == ""'), it accepts an
> >    empty pattern instead of rejecting it with FILT_ERR_INVALID_VALUE.
> 
> Well, empty value ("") is actually valid. May be meaningless, but it's not
> wrong.

OK, I will also add this case in the test.

> 
> > 
> > 4. When the operand is unquoted (q == 0), stray quotes or '(' inside the
> >    operand are consumed as part of the pattern, desynchronizing
> >    parse_pred() from calc_stack().
> 
> This is a separate bug and needs to be a separate patch, although it may be
> the same as #2 above.

OK.

Thanks for your comments!

> 
> -- Steve
> 
> > 
> > Fix this by advancing 'i' and 's' past the opening quote before the loop,
> > starting the loop at the current 'i' without a blind increment, checking
> > for missing closing quotes, advancing 'i' past the closing quote, and
> > rejecting empty operands or stray quotes/'(' in unquoted operands.
> > 
> > Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > ---
>

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