tracing: More fixes for v7.3

tracing fixes for v7.3:

- Fix a crash in print_graph_function_event()

When a tracer does not define static or default flags the tr->current_trace_flags is set to NULL. If the ring buffer is not cleared when switching tracers, the previous tracers events could be read. The function_graph entries will dereference the tr->current_trace_flags and cause a crash.

Set the tr->current_trace_flags to a empty set of flags to prevent it from being NULL.

- Separate tracing filter predicate count from allocation

The amount allocated for the predicate program is the number of predicates defined plus two for adding "true" and "false" at the end. The nr_preds is incremented by two to accommodate the booleans but is then used as the limit check for number of predicates. This is incorrect as it is now two higher than what is expected.

Use a separate variable for the size of the predicate array.

- Fix the filter operator stack check

The operator stack increments the pointer and then writes the content onto the stack. The check for overflow compared the current pointer to see if it was greater than the stack size before doing so. This is an off by 2 error as the write will be at the size - 1 and the compare is done before the increment. Adjust the check accordingly.

git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace/fixes

Head SHA1: c47a7b11bcb3d45e29723afcf6336866476b1d08

Hao Ge (1):
tracing: Fix crash in print_graph_function_event()

Taeyang Lee (2):
tracing: Keep the predicate limit separate from program capacity
tracing: Check operator stack capacity before pushing

----

 kernel/trace/trace.c               | 20 ++++++++++++++++++--
 kernel/trace/trace_events_filter.c |  9 ++++-----
 2 files changed, 22 insertions(+), 7 deletions(-)

From: Hao Ge <[email protected]>

tracing_set_tracer() assigns the flags of the newly selected tracer with:

	tr->current_trace_flags = t->flags ? : t->tracer->flags;

If the new tracer defines neither static flags nor default flags, like hwlat, irqsoff or wakeup, add_tracer() has no per instance flags copy for it, and both operands are NULL, so tr->current_trace_flags ends up being NULL as well.

The ring buffer is not cleared when switching tracers: it is reset inside tracer_init(), after current_trace_flags was already assigned. If the buffer still holds function_graph entries of the previous tracer, printing one of them dereferences the NULL pointer. This was hit by cat trace during an ftrace stress test:

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  #PF: supervisor read access in kernel mode
  RIP: 0010:print_graph_function_event+0xe/0x20
  Call Trace:
   s_show+0x4d/0x110
   seq_read_iter+0x2d4/0x490
   seq_read+0x130/0x170
   vfs_read+0xb5/0x370
   ksys_read+0x6d/0xe0
   do_syscall_64+0xbc/0x450

The snapshot buffer is not reset at all, so this can be reproduced without a race:

echo function_graph > /sys/kernel/tracing/current_tracer echo 1 > /sys/kernel/tracing/snapshot
echo hwlat > /sys/kernel/tracing/current_tracer cat /sys/kernel/tracing/snapshot

Rather than teaching every user of tr->current_trace_flags about this corner case, make sure it is never NULL: when the newly selected tracer has no flags at all, tracing_set_tracer() falls back to a static empty no_tracer_flags. run_tracer_selftest() can hit the same case, so the same fallback is added there too. The existing users already handle opts == NULL, so these tracers keep behaving as before.

---
 kernel/trace/trace.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

index 3b22e70c6016..97a058623f94 100644
@@ -99,6 +99,12 @@ struct tracers {
 	struct tracer_flags	*flags;
 };
 
+/*
+ * Tracers that do not define their own flags use these empty flags,
+ * as tr->current_trace_flags must never be NULL.
+ */
+static struct tracer_flags no_tracer_flags;
+
 /*
  * To prevent the comm cache from being overwritten when no
  * tracing is active, only save the comm when a trace event

@@ -1318,7 +1324,12 @@ static int run_tracer_selftest(struct tracer *type)
 	tracing_reset_online_cpus(&tr->array_buffer);
 
 	tr->current_trace = type;
-	tr->current_trace_flags = type->flags ? : type->default_flags;
+	if (type->flags)
+		tr->current_trace_flags = type->flags;
+	else if (type->default_flags)
+		tr->current_trace_flags = type->default_flags;
+	else
+		tr->current_trace_flags = &no_tracer_flags;
 
 #ifdef CONFIG_TRACER_MAX_TRACE
 	if (tracer_uses_snapshot(type)) {

@@ -5089,7 +5100,12 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
 			return ret;
 	}
 
-	tr->current_trace_flags = t->flags ? : t->tracer->flags;
+	if (t->flags)
+		tr->current_trace_flags = t->flags;
+	else if (t->tracer->flags)
+		tr->current_trace_flags = t->tracer->flags;
+	else
+		tr->current_trace_flags = &no_tracer_flags;
 
 	if (trace->init) {
 		ret = tracer_init(trace, tr);

2.53.0

From: Taeyang Lee <[email protected]>

predicate_parse() adds two to nr_preds to reserve the terminal TRUE and FALSE entries. It also uses that increased value as the limit for parsed predicates. If the initial count is smaller than the number actually parsed, the program can fill the allocation and subsequent accesses can go past its end.

Keep nr_preds as the predicate limit and use a separate value for the allocation size. This leaves space for both terminal entries and a zeroed entry when parsing stops at the limit.

---
 kernel/trace/trace_events_filter.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

index 2b46ca536045..781416ba6bdf 100644
@@ -482,18 +482,17 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
 	int len;
 	int N = 0;
 	int i;
-
-	nr_preds += 2; /* For TRUE and FALSE */
+	int nr_preds_alloc = nr_preds + 2; /* For TRUE and FALSE */
 
 	op_stack = kmalloc_objs(*op_stack, nr_parens);
 	if (!op_stack)
 		return ERR_PTR(-ENOMEM);
-	prog_stack = kzalloc_objs(*prog_stack, nr_preds);
+	prog_stack = kzalloc_objs(*prog_stack, nr_preds_alloc);
 	if (!prog_stack) {
 		parse_error(pe, -ENOMEM, 0);
 		goto out_free;
 	}
-	inverts = kmalloc_array(nr_preds, sizeof(*inverts), GFP_KERNEL);
+	inverts = kmalloc_array(nr_preds_alloc, sizeof(*inverts), GFP_KERNEL);
 	if (!inverts) {
 		parse_error(pe, -ENOMEM, 0);
 		goto out_free;

2.53.0

From: Taeyang Lee <[email protected]>

predicate_parse() uses the first operator stack entry before processing any opening parentheses. A push then increments top before writing. The existing check still permits that write when top already points to the last allocated entry, which can access beyond the operator stack.

Reject the push when top reaches the last allocated entry.

---
 kernel/trace/trace_events_filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 781416ba6bdf..127880ebe83f 100644
@@ -511,7 +511,7 @@ predicate_parse(const char *str, int nr_parens, int nr_preds,
 
 		switch (*next) {
 		case '(':					/* #2 */
-			if (top - op_stack > nr_parens) {
+			if (top - op_stack >= nr_parens - 1) {
 				ret = -EINVAL;
 				goto out_free;
 			}

2.53.0