* [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation
@ 2024-08-13 17:25 Fernando Fernandez Mancera
2024-08-25 7:41 ` Masami Hiramatsu
0 siblings, 1 reply; 9+ messages in thread
From: Fernando Fernandez Mancera @ 2024-08-13 17:25 UTC (permalink / raw)
To: linux-trace-kernel; +Cc: Fernando Fernandez Mancera
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
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;
--
2.46.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation 2024-08-13 17:25 [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation Fernando Fernandez Mancera @ 2024-08-25 7:41 ` Masami Hiramatsu 2024-08-25 17:06 ` Fernando F. Mancera 0 siblings, 1 reply; 9+ messages in thread From: Masami Hiramatsu @ 2024-08-25 7:41 UTC (permalink / raw) To: Fernando Fernandez Mancera; +Cc: linux-trace-kernel 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... Anyway, trace_probe_init() should return -E2BIG in this case because it is actuall wrong value. Can you update your patch? 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation 2024-08-25 7:41 ` Masami Hiramatsu @ 2024-08-25 17:06 ` Fernando F. Mancera 2024-08-25 23:56 ` Masami Hiramatsu 0 siblings, 1 reply; 9+ messages in thread From: Fernando F. Mancera @ 2024-08-25 17:06 UTC (permalink / raw) To: Masami Hiramatsu (Google); +Cc: linux-trace-kernel 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. 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; >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation 2024-08-25 17:06 ` Fernando F. Mancera @ 2024-08-25 23:56 ` Masami Hiramatsu 2024-08-26 14:58 ` Fernando F. Mancera 2024-09-04 22:19 ` Fernando F. Mancera 0 siblings, 2 replies; 9+ messages in thread From: Masami Hiramatsu @ 2024-08-25 23:56 UTC (permalink / raw) To: Fernando F. Mancera; +Cc: linux-trace-kernel 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. 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation 2024-08-25 23:56 ` Masami Hiramatsu @ 2024-08-26 14:58 ` Fernando F. Mancera 2024-09-04 22:19 ` Fernando F. Mancera 1 sibling, 0 replies; 9+ messages in thread From: Fernando F. Mancera @ 2024-08-26 14:58 UTC (permalink / raw) To: Masami Hiramatsu (Google); +Cc: linux-trace-kernel 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. > Thanks I will update the patch. If you don't mind I would also like to do the follow up patch series. > 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; >>>> >>> >>> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation 2024-08-25 23:56 ` Masami Hiramatsu 2024-08-26 14:58 ` Fernando F. Mancera @ 2024-09-04 22:19 ` Fernando F. Mancera 2024-10-22 5:40 ` Masami Hiramatsu 1 sibling, 1 reply; 9+ messages in thread From: Fernando F. Mancera @ 2024-09-04 22:19 UTC (permalink / raw) To: Masami Hiramatsu (Google); +Cc: linux-trace-kernel 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. 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; >>>> >>> >>> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation 2024-09-04 22:19 ` Fernando F. Mancera @ 2024-10-22 5:40 ` Masami Hiramatsu 2024-10-22 13:47 ` Masami Hiramatsu 2024-10-23 8:48 ` Fernando F. Mancera 0 siblings, 2 replies; 9+ messages in thread From: Masami Hiramatsu @ 2024-10-22 5:40 UTC (permalink / raw) To: Fernando F. Mancera; +Cc: linux-trace-kernel 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation 2024-10-22 5:40 ` Masami Hiramatsu @ 2024-10-22 13:47 ` Masami Hiramatsu 2024-10-23 8:48 ` Fernando F. Mancera 1 sibling, 0 replies; 9+ messages in thread From: Masami Hiramatsu @ 2024-10-22 13:47 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: Fernando F. Mancera, linux-trace-kernel 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation 2024-10-22 5:40 ` Masami Hiramatsu 2024-10-22 13:47 ` Masami Hiramatsu @ 2024-10-23 8:48 ` Fernando F. Mancera 1 sibling, 0 replies; 9+ messages in thread From: Fernando F. Mancera @ 2024-10-23 8:48 UTC (permalink / raw) To: Masami Hiramatsu (Google); +Cc: linux-trace-kernel Hi, On 22/10/2024 07:40, Masami Hiramatsu (Google) 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. > Sure, let me know if you need me to rebase it. Thanks for taking a look to 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; >>>>>> >>>>> >>>>> >>> >>> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-23 8:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-13 17:25 [PATCH] tracing/probes: fix traceprobe out-of-bounds argument allocation Fernando Fernandez Mancera 2024-08-25 7:41 ` Masami Hiramatsu 2024-08-25 17:06 ` Fernando F. Mancera 2024-08-25 23:56 ` Masami Hiramatsu 2024-08-26 14:58 ` Fernando F. Mancera 2024-09-04 22:19 ` Fernando F. Mancera 2024-10-22 5:40 ` Masami Hiramatsu 2024-10-22 13:47 ` Masami Hiramatsu 2024-10-23 8:48 ` Fernando F. Mancera
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).