[PATCH] tracing: Reject filter expressions that end on an operator

[PATCH] tracing: Reject filter expressions that end on an operator

[JongHyuk Kim (MalHyuk)]

predicate_parse() seeds every new program entry with a backwards link:

	prog[N].target = N-1;

so prog[0].target starts out as -1. It is expected to be overwritten by
update_preds() before the second pass walks the program.

If the expression ends right after "&&" or "||", the pending PROCESS_AND
or PROCESS_OR bit on the operator stack is never consumed, update_preds()
never runs, and prog[0].target is still -1 when the second pass starts:

	for (i = N-1 ; i--; ) {
		int target = prog[i].target;
		if (prog[i].when_to_branch == prog[target].when_to_branch)
			prog[i].target = prog[target].target;
	}

That reads prog[-1], 16 bytes in front of the kcalloc()ed program. The
value read there is then stored back as a branch target, and the only
check on a branch target is the lower bound in the third pass:

	if (WARN_ON(prog[i].target <= i))

so any positive value is accepted, and filter_match_preds() goes on to
use it as an unchecked index into prog[].

Reproduced on 6.12.112 with KASAN as uid 1000, with perf_event_paranoid=2
and without tracefs. One perf_event_open() and one ioctl() are enough:

	a.type = PERF_TYPE_TRACEPOINT;
	a.config = <any tracepoint id>;
	a.exclude_kernel = 1;
	fd = perf_event_open(&a, 0, -1, -1, 0);
	ioctl(fd, PERF_EVENT_IOC_SET_FILTER, "common_pid==1||common_pid==1&&");

	BUG: KASAN: slab-out-of-bounds in process_preds+0x151e/0x1b60
	Read of size 4 at addr ffff8881014ad474 by task exp_progm1/123
	CPU: 1 UID: 1000 PID: 123 Comm: exp_progm1 Not tainted 6.12.112-dirty #1
	Call Trace:
	 process_preds
	 create_filter
	 ftrace_profile_set_filter
	 _perf_ioctl
	 perf_ioctl
	 __x64_sys_ioctl

The faulting address is prog - 12, that is prog[-1].when_to_branch. On a
kernel built with CONFIG_SLAB_VIRTUAL the same read lands on the unmapped
guard page in front of the slab whenever the program happens to be the
first object in it, and that is a fatal page fault rather than a silent
read.

An expression is not supposed to end on an operator, and that is the only
way a PROCESS_AND or PROCESS_OR bit survives to the end of the parse, so
reject it there.

Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
Cc: stable@vger.kernel.org
Signed-off-by: JongHyuk Kim (MalHyuk) <malhyuk97@gmail.com>
---
 kernel/trace/trace_events_filter.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -602,6 +602,13 @@
 		goto out_free;
 	}
 
+	if (*top & (PROCESS_AND | PROCESS_OR)) {
+		/* Expression ends on an operator */
+		ret = -EINVAL;
+		parse_error(pe, FILT_ERR_INVALID_OP, ptr - str);
+		goto out_free;
+	}
+
 	if (!N) {
 		/* No program? */
 		ret = -EINVAL;

Re: [PATCH] tracing: Reject filter expressions that end on an operator

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tracing: unreachable operator check in predicate_parse()
--

commit 6ada36f117f4b8f90eaf23b50ed6bf590c01cc94
Author: JongHyuk Kim (MalHyuk) <malhyuk97@gmail.com>

tracing: Reject filter expressions that end on an operator

This commit adds a check in predicate_parse() to reject event filter
expressions where a pending PROCESS_AND or PROCESS_OR flag remains on the
operator stack after parsing completes.

> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 5e74d2dae40a3..744e70668940f 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -611,6 +611,13 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
>  		goto out_free;
>  	}
>  
> +	if (*top & (PROCESS_AND | PROCESS_OR)) {
> +		/* Expression ends on an operator */
> +		ret = -EINVAL;
> +		parse_error(pe, FILT_ERR_INVALID_OP, ptr - str);
> +		goto out_free;
> +	}

[Severity: Medium]
Can this check ever be reached with PROCESS_AND or PROCESS_OR set in *top?

In kernel/trace/trace_events_filter.c:predicate_parse(), PROCESS_AND and
PROCESS_OR are only set inside the inner loop after at least one predicate
has been parsed (N >= 1), and setting either flag immediately breaks out to
the outer while (*ptr) loop:

	if (*top & PROCESS_AND) {
		update_preds(prog, N - 1, invert);
		*top &= ~PROCESS_AND;
	}
	if (*next == '&') {
		*top |= PROCESS_AND;
		break;
	}
	if (*top & PROCESS_OR) {
		update_preds(prog, N - 1, !invert);
		*top &= ~PROCESS_OR;
	}
	if (*next == '|') {
		*top |= PROCESS_OR;
		break;
	}
	if (!*next)
		goto out;

If an expression ends on "&&" or "||" (such as
"common_pid==1||common_pid==1&&"), the outer while (*ptr) loop terminates and
hits the check added by commit 1fdc4164e488 ("tracing: Reject incomplete event
filter expressions") right above the out label:

	/* Only the inner loop can complete a non-empty expression. */
	if (N) {
		ret = -EINVAL;
		parse_error(pe, FILT_ERR_INVALID_FILTER, ptr - str);
		goto out_free;
	}
 out:

Because N >= 1 whenever PROCESS_AND or PROCESS_OR is set, falling out of the
outer loop always takes that goto out_free path before reaching out.

The only other way to reach out is via if (!*next) goto out inside the inner
loop, which only runs after both PROCESS_AND and PROCESS_OR have just been
cleared from *top in the same iteration.

Doesn't that make this new check (and FILT_ERR_INVALID_OP) unreachable dead
code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026042540.2478566-1-malhyuk97@gmail.com?part=1