Re: [BUGFIX PATCH] tracing/kprobes: Fix strpbrk() argument order

[Steven Rostedt <rostedt@goodmis.org>]

On Sat, 3 Nov 2018 20:54:48 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Fri, 2 Nov 2018 09:54:24 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 2 Nov 2018 16:14:59 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >   
> > > On Thu, 1 Nov 2018 15:10:14 -0400
> > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > >   
> > > > On Thu,  1 Nov 2018 23:29:28 +0900
> > > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > >     
> > > > > Fix strpbrk()'s argument order, it must pass acceptable string
> > > > > in 2nd argument. Note that this can cause a kernel panic where
> > > > > it recovers backup character to code->data.
> > > > > 
> > > > > Fixes: a6682814f371 ("tracing/kprobes: Allow kprobe-events to record module symbol")
> > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>    
> > > > 
> > > > Thanks Masami,
> > > > 
> > > > I'm pulling this and starting to test it.    
> > > 
> > > Thank you! I still couldn't believe how this bug passed through the tests...  
> > 
> > I am too. I'm running tests with and without this patch, and the patch
> > doesn't appear to be making much difference.  
> 
> Maybe traceprobe_free_probe_arg() is silently failed.

I don't see how.

> 
> > 
> > Then I tested with this:
> > 
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index 3ef15a6683c0..4ddafddf1343 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -516,6 +516,7 @@ void traceprobe_free_probe_arg(struct probe_arg
> > *arg) kfree(code->data);
> >  		code++;
> >  	}
> > +	printk("free arg->code %s\n", arg->code);
> >  	kfree(arg->code);
> >  	kfree(arg->name);
> >  	kfree(arg->comm);
> > @@ -535,11 +536,15 @@ int traceprobe_update_arg(struct probe_arg *arg)
> >  			if (code[1].op != FETCH_OP_IMM)
> >  				return -EINVAL;
> >  
> > +			tmp = strpbrk(code->data, "+-");
> > +			printk("first tmp tmp=%s\n", tmp);
> >  			tmp = strpbrk("+-", code->data);
> > +			printk("second tmp=%s data=%s\n", tmp,
> > code->data); if (tmp)
> >  				c = *tmp;
> >  			ret =
> > traceprobe_split_symbol_offset(code->data, &offset);
> > +			printk("third data=%s\n", code->data);
> >  			if (ret)
> >  				return ret;
> >  
> > @@ -547,6 +552,7 @@ int traceprobe_update_arg(struct probe_arg *arg)
> >  				(unsigned
> > long)kallsyms_lookup_name(code->data); if (tmp)
> >  				*tmp = c;
> > +			printk("forth data=%s\n", code->data);
> >  			if (!code[1].immediate)
> >  				return -ENOENT;
> >  			code[1].immediate += offset;
> > 
> > And I don't see where that code->data is used elsewhere. That is, why
> > even bother saving the character?  
> 
> Would you mean parsing the symbol+offs every time is useless?
> It needs to solve the symbol address always because  traceprobe_update_arg
> is called when new symbols added on the kernel (by loading modules).

OK, so it is called multiple times? That is when a module is loaded?
Because I couldn't get this called multiple times.

I'll try that out and if that's the case, then yeah, this needs to be
fixed (otherwise, I was thinking we could just remove the strpbrk()
altogether).


> 
> Hmm, maybe I can introduce a struct like 
> 
> struct symbol_offset {
> 	long offset;
> 	char symbol[];
> };
> 
> and use it instead of parsing the symbol+offset always.

The parsing should be fine. The issue I had was that I couldn't trigger
it to happen more than once. That's why this passed testing. We didn't
do something that required it to be called more than once and that is
here the bug would trigger.

-- Steve