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

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

Hi Fernando,

I found the patch from Mikel solves the same issue and his v2 patch fixes
wider situation ("$arg*" BTF argument case). So I decided to pick his patch.

https://lore.kernel.org/all/20240930202656.292869-1-mikel@mikelr.com/

Let me know if there is any case the above fix does not cover.

Thank you,


On Tue, 22 Oct 2024 14:40:45 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> 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>


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