[PATCH 0/2] tracing: Fix NULL pointer bug in hist key expressions

[PATCH 0/2] tracing: Fix NULL pointer bug in hist key expressions

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

Hi Steve,

This is a fix for a user-reported bug in the hist triggers, where if a
variable reference is used in an expression in a histogram key, it
results in a NULL pointer dereference and subsequent Oops.

I separated the fix into two small patches, the first preventing the
immediate problem bu disallowing a var_ref from ever accessing a NULL
element, and the second disallowing a user from ever constructing such
a key.

Thanks,

Tom

The following changes since commit ee474b81fe5aa5dc0faae920bf66240fbf55f891:

  tracing/kprobes: Fix strpbrk() argument order (2018-11-05 09:47:14 -0500)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/key-ref-fix

Tom Zanussi (2):
  tracing: Prevent hist_field_var_ref() from accessing NULL
    tracing_map_elts
  tracing: Check keys for variable references in expressions too

 kernel/trace/trace_events_hist.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.14.1

[PATCH 1/2] tracing: Prevent hist_field_var_ref() from accessing NULL tracing_map_elts

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

hist_field_var_ref() is an implementation of hist_field_fn_t(), which
can be called with a null tracing_map_elt elt param when assembling a
key in event_hist_trigger().

In the case of hist_field_var_ref() this doesn't make sense, because a
variable can only be resolved by looking it up using an already
assembled key i.e. a variable can't be used to assemble a key since
the key is required in order to access the variable.

Upper layers should prevent the user from constructing a key using a
variable in the first place, but in case one slips through, it
shouldn't cause a NULL pointer dereference.  Also if one does slip
through, we want to know about it, so emit a one-time warning in that
case.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Reported-by: Vincent Bernat <vincent@bernat.ch>
---
 kernel/trace/trace_events_hist.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index eb908ef2ecec..3795207a48a0 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1632,6 +1632,9 @@ static u64 hist_field_var_ref(struct hist_field *hist_field,
 	struct hist_elt_data *elt_data;
 	u64 var_val = 0;
 
+	if (WARN_ON_ONCE(!elt))
+		return var_val;
+
 	elt_data = elt->private_data;
 	var_val = elt_data->var_ref_vals[hist_field->var_ref_idx];
 
-- 
2.14.1

[PATCH 2/2] tracing: Check keys for variable references in expressions too

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

There's an existing check for variable references in keys, but it
doesn't go far enough.  It checks whether a key field is a variable
reference but doesn't check whether it's an expression containing
variable references, which can cause the same problems for callers.

Use the existing field_has_hist_vars() function rather than a direct
top-level flag check to catch all possible variable references.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Reported-by: Vincent Bernat <vincent@bernat.ch>
---
 kernel/trace/trace_events_hist.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 3795207a48a0..bf993a9a65ec 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -3970,8 +3970,8 @@ static int create_key_field(struct hist_trigger_data *hist_data,
 			goto out;
 		}
 
-		if (hist_field->flags & HIST_FIELD_FL_VAR_REF) {
-			hist_err("Using variable references as keys not supported: ", field_str);
+		if (field_has_hist_vars(hist_field, 0))	{
+			hist_err("Using variable references in keys not supported: ", field_str);
 			destroy_hist_field(hist_field, 0);
 			ret = -EINVAL;
 			goto out;
-- 
2.14.1

Re: [PATCH 0/2] tracing: Fix NULL pointer bug in hist key expressions

[Steven Rostedt]

On Thu,  8 Nov 2018 08:41:47 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> From: Tom Zanussi <tom.zanussi@linux.intel.com>
> 
> Hi Steve,
> 
> This is a fix for a user-reported bug in the hist triggers, where if a
> variable reference is used in an expression in a histogram key, it
> results in a NULL pointer dereference and subsequent Oops.

How does one add a variable reference as an expression in a
histogram key?

Can you show an example?

-- Steve

> 
> I separated the fix into two small patches, the first preventing the
> immediate problem bu disallowing a var_ref from ever accessing a NULL
> element, and the second disallowing a user from ever constructing such
> a key.
> 
> Thanks,
> 
> Tom
> 
> The following changes since commit ee474b81fe5aa5dc0faae920bf66240fbf55f891:
> 
>   tracing/kprobes: Fix strpbrk() argument order (2018-11-05 09:47:14 -0500)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/key-ref-fix
> 
> Tom Zanussi (2):
>   tracing: Prevent hist_field_var_ref() from accessing NULL
>     tracing_map_elts
>   tracing: Check keys for variable references in expressions too
> 
>  kernel/trace/trace_events_hist.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Re: [PATCH 0/2] tracing: Fix NULL pointer bug in hist key expressions

[Tom Zanussi]

Hi Steve,

On Thu, 2018-11-08 at 13:45 -0500, Steven Rostedt wrote:
> On Thu,  8 Nov 2018 08:41:47 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > 
> > Hi Steve,
> > 
> > This is a fix for a user-reported bug in the hist triggers, where
> > if a
> > variable reference is used in an expression in a histogram key, it
> > results in a NULL pointer dereference and subsequent Oops.
> 
> How does one add a variable reference as an expression in a
> histogram key?
> 
> Can you show an example?
> 

Sure, here's the example that showed the problem, and which the patch
fixes:

echo 'r:fib_table_lookup_ret fib_table_lookup $retval' > /sys/kernel/debug/tracing/kprobe_events

echo 'hist:keys=cpu:ts0=common_timestamp' > /sys/kernel/debug/tracing/events/fib/fib_table_lookup/trigger
echo 'hist:keys=cpu,common_timestamp-$ts0' > /sys/kernel/debug/tracing/events/kprobes/fib_table_lookup_ret/trigger

The following is also a variable reference in an expression, but in
this case the expression is the variable reference alone, which the
existing code handles properly and returns -EINVAL.

echo 'hist:keys=cpu,$ts0' > /sys/kernel/debug/tracing/events/kprobes/fib_table_lookup_ret/trigger

Tom

> -- Steve
> 
> > 
> > I separated the fix into two small patches, the first preventing
> > the
> > immediate problem bu disallowing a var_ref from ever accessing a
> > NULL
> > element, and the second disallowing a user from ever constructing
> > such
> > a key.
> > 
> > Thanks,
> > 
> > Tom
> > 
> > The following changes since commit
> > ee474b81fe5aa5dc0faae920bf66240fbf55f891:
> > 
> >   tracing/kprobes: Fix strpbrk() argument order (2018-11-05
> > 09:47:14 -0500)
> > 
> > are available in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-
> > trace.git ftrace/key-ref-fix
> > 
> > Tom Zanussi (2):
> >   tracing: Prevent hist_field_var_ref() from accessing NULL
> >     tracing_map_elts
> >   tracing: Check keys for variable references in expressions too
> > 
> >  kernel/trace/trace_events_hist.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> 
>