Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

Hi,

On Thu, 5 Sep 2024 00:19:16 +0200
"Fernando F. Mancera" <ffmancera@riseup.net> wrote:

> Hi,
> 
> On 26/08/2024 01:56, Masami Hiramatsu (Google) wrote:
> > On Sun, 25 Aug 2024 19:06:22 +0200
> > "Fernando F. Mancera" <ffmancera@riseup.net> wrote:
> > 
> >> Hi,
> >>
> >> On 25/08/2024 09:41, Masami Hiramatsu (Google) wrote:
> >>> Hi,
> >>>
> >>> On Tue, 13 Aug 2024 13:25:40 -0400
> >>> Fernando Fernandez Mancera <ffmancera@riseup.net> wrote:
> >>>
> >>>> When initializing trace_probes::nr_args, make sure the maximum number of
> >>>> probe arguments is honored. Oherwise, we can hit a NULL pointer
> >>>> dereferences in multiple situations like on traceprobe_set_print_fmt().
> >>>>
> >>>> Link: https://bugzilla.redhat.com/2303876
> >>>
> >>> Sorry for replying later. I'm not sure why but I did not found this in my mbox...
> >>>
> >>
> >> No worries!
> >>
> >>> Anyway, trace_probe_init() should return -E2BIG in this case because
> >>> it is actuall wrong value.
> >>>
> >>> Can you update your patch?
> >>>
> >>
> >> I agree this is the right solution but it would mean a change in
> >> behavior. Is that fine? Before merging 035ba76014c0, it was writing up
> >> to the maximum number of arguments and ignoring the rest. If you confirm
> >> it is fine to change the behavior, I will update the patch.
> > 
> > Yes, I think this is the better behavior for users too, rather than
> > silently droping some arguments.
> > We also need to handle this error code in callsite and log an error,
> > and add a testcase in the syntax error testcase. But that should be done
> > in another series. We should fix this first.
> > 
> 
> Just in case this fell through the cracks, I've sent a V2 patch. In 
> addition I also have a series ready for the logging and syntax error 
> testcase in kselftest but I didn't send it because it requires this 
> patch to be applied.

I think that v2 was lost in my mailbox, but I found that in patchwork.
Let me pick it.

Thanks, 

> 
> Thank you,
> Fernando.
> 
> > Thank you,
> > 
> > 
> >>
> >> Thank you!
> >>
> >>> Thank you,
> >>>
> >>>
> >>>>
> >>>> Fixes: 035ba76014c0 ("tracing/probes: cleanup: Set trace_probe::nr_args at trace_probe_init")
> >>>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> >>>> ---
> >>>>    kernel/trace/trace_probe.c | 8 ++++++--
> >>>>    1 file changed, 6 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> >>>> index 39877c80d6cb..f577b5e71026 100644
> >>>> --- a/kernel/trace/trace_probe.c
> >>>> +++ b/kernel/trace/trace_probe.c
> >>>> @@ -2043,10 +2043,14 @@ int trace_probe_init(struct trace_probe *tp, const char *event,
> >>>>    		goto error;
> >>>>    	}
> >>>>    
> >>>> -	tp->nr_args = nargs;
> >>>> +	if (nargs > MAX_TRACE_ARGS)
> >>>> +		tp->nr_args = MAX_TRACE_ARGS;
> >>>> +	else
> >>>> +		tp->nr_args = nargs;
> >>>> +
> >>>>    	/* Make sure pointers in args[] are NULL */
> >>>>    	if (nargs)
> >>>> -		memset(tp->args, 0, sizeof(tp->args[0]) * nargs);
> >>>> +		memset(tp->args, 0, sizeof(tp->args[0]) * tp->nr_args);
> >>>>    
> >>>>    	return 0;
> >>>>    
> >>>
> >>>
> > 
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>