* [PATCH] tracing/probe: Test nr_args match in looking for same probe events @ 2019-09-27 9:50 Steven Rostedt 2019-09-27 13:38 ` Srikar Dronamraju 0 siblings, 1 reply; 10+ messages in thread From: Steven Rostedt @ 2019-09-27 9:50 UTC (permalink / raw) To: LKML; +Cc: Masami Hiramatsu, Srikar Dronamraju, Naveen Rao, Ravi Bangoria From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Testing triggered: ================================================================== BUG: KASAN: slab-out-of-bounds in trace_kprobe_create+0xa9e/0xe40 Read of size 8 at addr ffff8880c4f25a48 by task ftracetest/4798 CPU: 2 PID: 4798 Comm: ftracetest Not tainted 5.3.0-rc6-test+ #30 Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016 Call Trace: dump_stack+0x7c/0xc0 ? trace_kprobe_create+0xa9e/0xe40 print_address_description+0x6c/0x332 ? trace_kprobe_create+0xa9e/0xe40 ? trace_kprobe_create+0xa9e/0xe40 __kasan_report.cold.6+0x1a/0x3b ? trace_kprobe_create+0xa9e/0xe40 kasan_report+0xe/0x12 trace_kprobe_create+0xa9e/0xe40 ? print_kprobe_event+0x280/0x280 ? match_held_lock+0x1b/0x240 ? find_held_lock+0xac/0xd0 ? fs_reclaim_release.part.112+0x5/0x20 ? lock_downgrade+0x350/0x350 ? kasan_unpoison_shadow+0x30/0x40 ? __kasan_kmalloc.constprop.6+0xc1/0xd0 ? trace_kprobe_create+0xe40/0xe40 ? trace_kprobe_create+0xe40/0xe40 create_or_delete_trace_kprobe+0x2e/0x60 trace_run_command+0xc3/0xe0 ? trace_panic_handler+0x20/0x20 ? kasan_unpoison_shadow+0x30/0x40 trace_parse_run_command+0xdc/0x163 vfs_write+0xe1/0x240 ksys_write+0xba/0x150 ? __ia32_sys_read+0x50/0x50 ? tracer_hardirqs_on+0x61/0x180 ? trace_hardirqs_off_caller+0x43/0x110 ? mark_held_locks+0x29/0xa0 ? do_syscall_64+0x14/0x260 do_syscall_64+0x68/0x260 The cause was that the args array to compare between two probe events only looked at one of the probe events size. If the other one had a smaller number of args, it would read out of bounds memory. Fixes: fe60b0ce8e733 ("tracing/probe: Reject exactly same probe event") Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel/trace/trace_kprobe.c | 2 ++ kernel/trace/trace_uprobe.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 402dc3ce88d3..d2543a403f25 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -537,6 +537,8 @@ static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig, list_for_each_entry(pos, &tpe->probes, list) { orig = container_of(pos, struct trace_kprobe, tp); + if (orig->tp.nr_args != comp->tp.nr_args) + continue; if (strcmp(trace_kprobe_symbol(orig), trace_kprobe_symbol(comp)) || trace_kprobe_offset(orig) != trace_kprobe_offset(comp)) diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index dd884341f5c5..11bcafdc93e2 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -420,6 +420,8 @@ static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig, list_for_each_entry(pos, &tpe->probes, list) { orig = container_of(pos, struct trace_uprobe, tp); + if (orig->tp.nr_args != comp->tp.nr_args) + continue; if (comp_inode != d_real_inode(orig->path.dentry) || comp->offset != orig->offset) continue; -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events 2019-09-27 9:50 [PATCH] tracing/probe: Test nr_args match in looking for same probe events Steven Rostedt @ 2019-09-27 13:38 ` Srikar Dronamraju 2019-09-27 15:06 ` Steven Rostedt 2019-09-28 8:17 ` Masami Hiramatsu 0 siblings, 2 replies; 10+ messages in thread From: Srikar Dronamraju @ 2019-09-27 13:38 UTC (permalink / raw) To: Steven Rostedt; +Cc: LKML, Masami Hiramatsu, Naveen Rao, Ravi Bangoria <SNIP> > The cause was that the args array to compare between two probe events only > looked at one of the probe events size. If the other one had a smaller > number of args, it would read out of bounds memory. > I thought trace_probe_compare_arg_type() should have caught this. But looks like there is one case it misses. Currently trace_probe_compare_arg_type() is okay if the newer probe has lesser or equal arguments than the older probe. For all other cases, it seems to error out. In this case, we must be hitting where the newer probe has lesser arguments than older probe. > Fixes: fe60b0ce8e733 ("tracing/probe: Reject exactly same probe event") > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/trace_kprobe.c | 2 ++ > kernel/trace/trace_uprobe.c | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 402dc3ce88d3..d2543a403f25 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -537,6 +537,8 @@ static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig, > > list_for_each_entry(pos, &tpe->probes, list) { > orig = container_of(pos, struct trace_kprobe, tp); > + if (orig->tp.nr_args != comp->tp.nr_args) > + continue; This has a side-effect where the newer probe has same argument commands, we still end up appending the probe. Lets says we already have a probe with 2 arguments, the newer probe has only the first argument but same fetch command, we should have erred out. No? > if (strcmp(trace_kprobe_symbol(orig), > trace_kprobe_symbol(comp)) || > trace_kprobe_offset(orig) != trace_kprobe_offset(comp)) > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index dd884341f5c5..11bcafdc93e2 100644 > --- a/kernel/trace/trace_uprobe.c > +++ b/kernel/trace/trace_uprobe.c > @@ -420,6 +420,8 @@ static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig, > > list_for_each_entry(pos, &tpe->probes, list) { > orig = container_of(pos, struct trace_uprobe, tp); > + if (orig->tp.nr_args != comp->tp.nr_args) > + continue; > if (comp_inode != d_real_inode(orig->path.dentry) || > comp->offset != orig->offset) > continue; How about something like this? diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 402dc3ce88d3..a056ff240957 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -546,13 +546,13 @@ static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig, * trace_probe_compare_arg_type() ensured that nr_args and * each argument name and type are same. Let's compare comm. */ - for (i = 0; i < orig->tp.nr_args; i++) { + for (i = 0; i < comp->tp.nr_args; i++) { if (strcmp(orig->tp.args[i].comm, comp->tp.args[i].comm)) break; } - if (i == orig->tp.nr_args) + if (i == comp->tp.nr_args) return true; } diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index dd884341f5c5..512ce55ced91 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -428,13 +428,13 @@ static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig, * trace_probe_compare_arg_type() ensured that nr_args and * each argument name and type are same. Let's compare comm. */ - for (i = 0; i < orig->tp.nr_args; i++) { + for (i = 0; i < comp->tp.nr_args; i++) { if (strcmp(orig->tp.args[i].comm, comp->tp.args[i].comm)) break; } - if (i == orig->tp.nr_args) + if (i == comp->tp.nr_args) return true; } With the above changes: # :> kprobe_events # echo p:test _do_fork arg1=%gpr3 arg2=%gpr4 arg3=%gpr5 >> kprobe_events # cat kprobe_events p:kprobes/test _do_fork arg1=%gpr3 arg2=%gpr4 arg3=%gpr5 #Add with extra arguments : SHOULD FAIL # echo p:test _do_fork arg1=%gpr3 arg2=%gpr4 arg3=%gpr5 arg4=%gpr6>> kprobe_events bash: echo: write error: File exists #Add with same arguments :SHOULD FAIL # echo p:test _do_fork arg1=%gpr3 arg2=%gpr4 arg3=%gpr5 >> kprobe_events bash: echo: write error: File exists #Add with less events but different name arg5 instead of arg2 :SHOULD FAIL # echo p:test _do_fork arg1=%gpr3 arg5=%gpr2 >> kprobe_events bash: echo: write error: File exists #Add with less events with same name but different comm : SHOULD PASS # echo p:test _do_fork arg1=%gpr3 arg2=%gpr2 >> kprobe_events # cat kprobe_events p:kprobes/test _do_fork arg1=%gpr3 arg2=%gpr4 arg3=%gpr5 p:kprobes/test _do_fork arg1=%gpr3 arg2=%gpr2 -- Thanks and Regards Srikar Dronamraju ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events 2019-09-27 13:38 ` Srikar Dronamraju @ 2019-09-27 15:06 ` Steven Rostedt 2019-09-27 17:54 ` Srikar Dronamraju 2019-09-28 8:17 ` Masami Hiramatsu 1 sibling, 1 reply; 10+ messages in thread From: Steven Rostedt @ 2019-09-27 15:06 UTC (permalink / raw) To: Srikar Dronamraju; +Cc: LKML, Masami Hiramatsu, Naveen Rao, Ravi Bangoria On Fri, 27 Sep 2019 19:08:53 +0530 Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote: \> > --- > > kernel/trace/trace_kprobe.c | 2 ++ > > kernel/trace/trace_uprobe.c | 2 ++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 402dc3ce88d3..d2543a403f25 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -537,6 +537,8 @@ static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig, > > > > list_for_each_entry(pos, &tpe->probes, list) { > > orig = container_of(pos, struct trace_kprobe, tp); > > + if (orig->tp.nr_args != comp->tp.nr_args) > > + continue; > > This has a side-effect where the newer probe has same argument commands, we > still end up appending the probe. ?? How so? If the two have the same number of arguments we do exactly what we did before this patch. Please explain to me how that side effect would happen? It basically is doing, "if the two probes do not have the same number of arguments, don't bother comparing, because they are different." -- Steve ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events 2019-09-27 15:06 ` Steven Rostedt @ 2019-09-27 17:54 ` Srikar Dronamraju 0 siblings, 0 replies; 10+ messages in thread From: Srikar Dronamraju @ 2019-09-27 17:54 UTC (permalink / raw) To: Steven Rostedt; +Cc: LKML, Masami Hiramatsu, Naveen Rao, Ravi Bangoria > > > > This has a side-effect where the newer probe has same argument commands, we > > still end up appending the probe. > > ?? > > How so? > > If the two have the same number of arguments we do exactly what we did > before this patch. Please explain to me how that side effect would happen? > > It basically is doing, "if the two probes do not have the same number > of arguments, don't bother comparing, because they are different." > Lets take the first probe has 3 arguments passed to it and the second probe has just 2 arguments. If the first two arguments are same type, name, and comm, should we append to the first probe? I think No, I would believe we should append only if the comm of either of the arguments was different. Example: echo p:test _do_fork arg1=%ax arg2=%bx arg3=%cx >> kprobe_events echo p:test _do_fork arg1=%ax arg2=%bx >> kprobe_events ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events 2019-09-27 13:38 ` Srikar Dronamraju 2019-09-27 15:06 ` Steven Rostedt @ 2019-09-28 8:17 ` Masami Hiramatsu 2019-09-28 9:56 ` Masami Hiramatsu 2019-09-28 9:59 ` [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe Masami Hiramatsu 1 sibling, 2 replies; 10+ messages in thread From: Masami Hiramatsu @ 2019-09-28 8:17 UTC (permalink / raw) To: Srikar Dronamraju Cc: Steven Rostedt, LKML, Masami Hiramatsu, Naveen Rao, Ravi Bangoria [-- Attachment #1: Type: text/plain, Size: 4067 bytes --] Hi Sriker and Steve, Sorry for later, I was in a conference. On Fri, 27 Sep 2019 19:08:53 +0530 Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote: > <SNIP> > > > The cause was that the args array to compare between two probe events only > > looked at one of the probe events size. If the other one had a smaller > > number of args, it would read out of bounds memory. > > > > I thought trace_probe_compare_arg_type() should have caught this. But looks > like there is one case it misses. > > Currently trace_probe_compare_arg_type() is okay if the newer probe has > lesser or equal arguments than the older probe. For all other cases, it > seems to error out. In this case, we must be hitting where the newer probe > has lesser arguments than older probe. > > > > Fixes: fe60b0ce8e733 ("tracing/probe: Reject exactly same probe event") > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > > --- > > kernel/trace/trace_kprobe.c | 2 ++ > > kernel/trace/trace_uprobe.c | 2 ++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 402dc3ce88d3..d2543a403f25 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -537,6 +537,8 @@ static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig, > > > > list_for_each_entry(pos, &tpe->probes, list) { > > orig = container_of(pos, struct trace_kprobe, tp); > > + if (orig->tp.nr_args != comp->tp.nr_args) > > + continue; > > This has a side-effect where the newer probe has same argument commands, we > still end up appending the probe. > > Lets says we already have a probe with 2 arguments, the newer probe has only > the first argument but same fetch command, we should have erred out. > No? Correct. > > > > if (strcmp(trace_kprobe_symbol(orig), > > trace_kprobe_symbol(comp)) || > > trace_kprobe_offset(orig) != trace_kprobe_offset(comp)) > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > > index dd884341f5c5..11bcafdc93e2 100644 > > --- a/kernel/trace/trace_uprobe.c > > +++ b/kernel/trace/trace_uprobe.c > > @@ -420,6 +420,8 @@ static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig, > > > > list_for_each_entry(pos, &tpe->probes, list) { > > orig = container_of(pos, struct trace_uprobe, tp); > > + if (orig->tp.nr_args != comp->tp.nr_args) > > + continue; > > if (comp_inode != d_real_inode(orig->path.dentry) || > > comp->offset != orig->offset) > > continue; > > How about something like this? Thank you for pointing it out. But from what I intended, this is caused by a bug in trace_probe_compare_arg_type() or it's caller. /* * trace_probe_compare_arg_type() ensured that nr_args and * each argument name and type are same. Let's compare comm. */ If we found that 2 probes have different number of argument should not be folded at all. Also, we have to adjust error log position. Attached patch will fix those errors correctly as bellow. /sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx >> kprobe_events /sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx >> kprobe_events sh: write error: File exists /sys/kernel/debug/tracing # echo p:myevent kernel_read %ax >> kprobe_events sh: write error: File exists /sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx %cx >> kprobe_eve nts sh: write error: File exists /sys/kernel/debug/tracing # cat error_log [ 15.917727] trace_kprobe: error: There is already the exact same probe event Command: p:myevent kernel_read %ax %bx ^ [ 24.890638] trace_kprobe: error: Argument type or name is different from existing probe Command: p:myevent kernel_read %ax ^ [ 31.480521] trace_kprobe: error: Argument type or name is different from existing probe Command: p:myevent kernel_read %ax %bx %cx ^ Thank you, -- Masami Hiramatsu [-- Attachment #2: tracing-probe-fix-to-check-the --] [-- Type: application/octet-stream, Size: 1524 bytes --] tracing/probe: Fix to check the difference of nr_args before adding probe From: Masami Hiramatsu <mhiramat@kernel.org> Fix to check the difference of nr_args before adding probe on existing probes. This also may set the error log index bigger than the number of command parameters. In that case it sets the error position is next to the last parameter. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- kernel/trace/trace_probe.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c index baf58a3612c0..905b10af5d5c 100644 --- a/kernel/trace/trace_probe.c +++ b/kernel/trace/trace_probe.c @@ -178,6 +178,16 @@ void __trace_probe_log_err(int offset, int err_type) if (!command) return; + if (trace_probe_log.index >= trace_probe_log.argc) { + /** + * Set the error position is next to the last arg + space. + * Note that len includes the terminal null and the cursor + * appaers at pos + 1. + */ + pos = len; + offset = 0; + } + /* And make a command string from argv array */ p = command; for (i = 0; i < trace_probe_log.argc; i++) { @@ -1084,6 +1094,12 @@ int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b) { int i; + /* In case of more arguments */ + if (a->nr_args < b->nr_args) + return a->nr_args + 1; + if (a->nr_args > b->nr_args) + return b->nr_args + 1; + for (i = 0; i < a->nr_args; i++) { if ((b->nr_args <= i) || ((a->args[i].type != b->args[i].type) || ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events 2019-09-28 8:17 ` Masami Hiramatsu @ 2019-09-28 9:56 ` Masami Hiramatsu 2019-09-28 9:59 ` [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe Masami Hiramatsu 1 sibling, 0 replies; 10+ messages in thread From: Masami Hiramatsu @ 2019-09-28 9:56 UTC (permalink / raw) To: Masami Hiramatsu Cc: Srikar Dronamraju, Steven Rostedt, LKML, Naveen Rao, Ravi Bangoria On Sat, 28 Sep 2019 01:17:48 -0700 Masami Hiramatsu <mhiramat@kernel.org> wrote: > If we found that 2 probes have different number of argument should not be > folded at all. > Also, we have to adjust error log position. Attached patch will fix those > errors correctly as bellow. Oops, missed the fixed tag. Anyway I'll send it as a patch mail. Thank you, > > /sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx >> kprobe_events > /sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx >> kprobe_events > sh: write error: File exists > /sys/kernel/debug/tracing # echo p:myevent kernel_read %ax >> kprobe_events > sh: write error: File exists > /sys/kernel/debug/tracing # echo p:myevent kernel_read %ax %bx %cx >> kprobe_eve > nts > sh: write error: File exists > /sys/kernel/debug/tracing # cat error_log > [ 15.917727] trace_kprobe: error: There is already the exact same probe event > Command: p:myevent kernel_read %ax %bx > ^ > [ 24.890638] trace_kprobe: error: Argument type or name is different from existing probe > Command: p:myevent kernel_read %ax > ^ > [ 31.480521] trace_kprobe: error: Argument type or name is different from existing probe > Command: p:myevent kernel_read %ax %bx %cx > ^ > > Thank you, > > -- > Masami Hiramatsu -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe 2019-09-28 8:17 ` Masami Hiramatsu 2019-09-28 9:56 ` Masami Hiramatsu @ 2019-09-28 9:59 ` Masami Hiramatsu 2019-09-28 21:11 ` Steven Rostedt 1 sibling, 1 reply; 10+ messages in thread From: Masami Hiramatsu @ 2019-09-28 9:59 UTC (permalink / raw) To: Steven Rostedt Cc: Srikar Dronamraju, Naveen Rao, Ravi Bangoria, linux-kernel, mhiramat, mingo Fix to check the difference of nr_args before adding probe on existing probes. This also may set the error log index bigger than the number of command parameters. In that case it sets the error position is next to the last parameter. Fixes: ca89bc071d5e ("tracing/kprobe: Add multi-probe per event support") Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- kernel/trace/trace_probe.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c index baf58a3612c0..905b10af5d5c 100644 --- a/kernel/trace/trace_probe.c +++ b/kernel/trace/trace_probe.c @@ -178,6 +178,16 @@ void __trace_probe_log_err(int offset, int err_type) if (!command) return; + if (trace_probe_log.index >= trace_probe_log.argc) { + /** + * Set the error position is next to the last arg + space. + * Note that len includes the terminal null and the cursor + * appaers at pos + 1. + */ + pos = len; + offset = 0; + } + /* And make a command string from argv array */ p = command; for (i = 0; i < trace_probe_log.argc; i++) { @@ -1084,6 +1094,12 @@ int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b) { int i; + /* In case of more arguments */ + if (a->nr_args < b->nr_args) + return a->nr_args + 1; + if (a->nr_args > b->nr_args) + return b->nr_args + 1; + for (i = 0; i < a->nr_args; i++) { if ((b->nr_args <= i) || ((a->args[i].type != b->args[i].type) || ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe 2019-09-28 9:59 ` [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe Masami Hiramatsu @ 2019-09-28 21:11 ` Steven Rostedt 2019-09-29 8:14 ` Masami Hiramatsu 2019-09-30 10:28 ` Srikar Dronamraju 0 siblings, 2 replies; 10+ messages in thread From: Steven Rostedt @ 2019-09-28 21:11 UTC (permalink / raw) To: Masami Hiramatsu Cc: Srikar Dronamraju, Naveen Rao, Ravi Bangoria, linux-kernel, mingo On Sat, 28 Sep 2019 02:59:08 -0700 Masami Hiramatsu <mhiramat@kernel.org> wrote: > Fix to check the difference of nr_args before adding probe > on existing probes. This also may set the error log index > bigger than the number of command parameters. In that case > it sets the error position is next to the last parameter. > > Fixes: ca89bc071d5e ("tracing/kprobe: Add multi-probe per event support") > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> I modified the change log a bit, below is the patch I plan on submitting. You OK with this? -- Steve From: Masami Hiramatsu <mhiramat@kernel.org> Date: Sat, 28 Sep 2019 05:53:29 -0400 Subject: [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe Steven reported that a test triggered: ================================================================== BUG: KASAN: slab-out-of-bounds in trace_kprobe_create+0xa9e/0xe40 Read of size 8 at addr ffff8880c4f25a48 by task ftracetest/4798 CPU: 2 PID: 4798 Comm: ftracetest Not tainted 5.3.0-rc6-test+ #30 Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016 Call Trace: dump_stack+0x7c/0xc0 ? trace_kprobe_create+0xa9e/0xe40 print_address_description+0x6c/0x332 ? trace_kprobe_create+0xa9e/0xe40 ? trace_kprobe_create+0xa9e/0xe40 __kasan_report.cold.6+0x1a/0x3b ? trace_kprobe_create+0xa9e/0xe40 kasan_report+0xe/0x12 trace_kprobe_create+0xa9e/0xe40 ? print_kprobe_event+0x280/0x280 ? match_held_lock+0x1b/0x240 ? find_held_lock+0xac/0xd0 ? fs_reclaim_release.part.112+0x5/0x20 ? lock_downgrade+0x350/0x350 ? kasan_unpoison_shadow+0x30/0x40 ? __kasan_kmalloc.constprop.6+0xc1/0xd0 ? trace_kprobe_create+0xe40/0xe40 ? trace_kprobe_create+0xe40/0xe40 create_or_delete_trace_kprobe+0x2e/0x60 trace_run_command+0xc3/0xe0 ? trace_panic_handler+0x20/0x20 ? kasan_unpoison_shadow+0x30/0x40 trace_parse_run_command+0xdc/0x163 vfs_write+0xe1/0x240 ksys_write+0xba/0x150 ? __ia32_sys_read+0x50/0x50 ? tracer_hardirqs_on+0x61/0x180 ? trace_hardirqs_off_caller+0x43/0x110 ? mark_held_locks+0x29/0xa0 ? do_syscall_64+0x14/0x260 do_syscall_64+0x68/0x260 Fix to check the difference of nr_args before adding probe on existing probes. This also may set the error log index bigger than the number of command parameters. In that case it sets the error position is next to the last parameter. Link: http://lkml.kernel.org/r/156966474783.3478.13217501608215769150.stgit@devnote2 Fixes: ca89bc071d5e ("tracing/kprobe: Add multi-probe per event support") Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel/trace/trace_probe.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c index baf58a3612c0..905b10af5d5c 100644 --- a/kernel/trace/trace_probe.c +++ b/kernel/trace/trace_probe.c @@ -178,6 +178,16 @@ void __trace_probe_log_err(int offset, int err_type) if (!command) return; + if (trace_probe_log.index >= trace_probe_log.argc) { + /** + * Set the error position is next to the last arg + space. + * Note that len includes the terminal null and the cursor + * appaers at pos + 1. + */ + pos = len; + offset = 0; + } + /* And make a command string from argv array */ p = command; for (i = 0; i < trace_probe_log.argc; i++) { @@ -1084,6 +1094,12 @@ int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b) { int i; + /* In case of more arguments */ + if (a->nr_args < b->nr_args) + return a->nr_args + 1; + if (a->nr_args > b->nr_args) + return b->nr_args + 1; + for (i = 0; i < a->nr_args; i++) { if ((b->nr_args <= i) || ((a->args[i].type != b->args[i].type) || -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe 2019-09-28 21:11 ` Steven Rostedt @ 2019-09-29 8:14 ` Masami Hiramatsu 2019-09-30 10:28 ` Srikar Dronamraju 1 sibling, 0 replies; 10+ messages in thread From: Masami Hiramatsu @ 2019-09-29 8:14 UTC (permalink / raw) To: Steven Rostedt Cc: Srikar Dronamraju, Naveen Rao, Ravi Bangoria, linux-kernel, mingo Hi Steve, On Sat, 28 Sep 2019 17:11:58 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Sat, 28 Sep 2019 02:59:08 -0700 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > Fix to check the difference of nr_args before adding probe > > on existing probes. This also may set the error log index > > bigger than the number of command parameters. In that case > > it sets the error position is next to the last parameter. > > > > Fixes: ca89bc071d5e ("tracing/kprobe: Add multi-probe per event support") > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > I modified the change log a bit, below is the patch I plan on submitting. > > You OK with this? Yes, of course. Thank you for updating! > > -- Steve > > > From: Masami Hiramatsu <mhiramat@kernel.org> > Date: Sat, 28 Sep 2019 05:53:29 -0400 > Subject: [PATCH] tracing/probe: Fix to check the difference of nr_args before > adding probe > > Steven reported that a test triggered: > > ================================================================== > BUG: KASAN: slab-out-of-bounds in trace_kprobe_create+0xa9e/0xe40 > Read of size 8 at addr ffff8880c4f25a48 by task ftracetest/4798 > > CPU: 2 PID: 4798 Comm: ftracetest Not tainted 5.3.0-rc6-test+ #30 > Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016 > Call Trace: > dump_stack+0x7c/0xc0 > ? trace_kprobe_create+0xa9e/0xe40 > print_address_description+0x6c/0x332 > ? trace_kprobe_create+0xa9e/0xe40 > ? trace_kprobe_create+0xa9e/0xe40 > __kasan_report.cold.6+0x1a/0x3b > ? trace_kprobe_create+0xa9e/0xe40 > kasan_report+0xe/0x12 > trace_kprobe_create+0xa9e/0xe40 > ? print_kprobe_event+0x280/0x280 > ? match_held_lock+0x1b/0x240 > ? find_held_lock+0xac/0xd0 > ? fs_reclaim_release.part.112+0x5/0x20 > ? lock_downgrade+0x350/0x350 > ? kasan_unpoison_shadow+0x30/0x40 > ? __kasan_kmalloc.constprop.6+0xc1/0xd0 > ? trace_kprobe_create+0xe40/0xe40 > ? trace_kprobe_create+0xe40/0xe40 > create_or_delete_trace_kprobe+0x2e/0x60 > trace_run_command+0xc3/0xe0 > ? trace_panic_handler+0x20/0x20 > ? kasan_unpoison_shadow+0x30/0x40 > trace_parse_run_command+0xdc/0x163 > vfs_write+0xe1/0x240 > ksys_write+0xba/0x150 > ? __ia32_sys_read+0x50/0x50 > ? tracer_hardirqs_on+0x61/0x180 > ? trace_hardirqs_off_caller+0x43/0x110 > ? mark_held_locks+0x29/0xa0 > ? do_syscall_64+0x14/0x260 > do_syscall_64+0x68/0x260 > > Fix to check the difference of nr_args before adding probe > on existing probes. This also may set the error log index > bigger than the number of command parameters. In that case > it sets the error position is next to the last parameter. > > Link: http://lkml.kernel.org/r/156966474783.3478.13217501608215769150.stgit@devnote2 > > Fixes: ca89bc071d5e ("tracing/kprobe: Add multi-probe per event support") > Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/trace_probe.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > index baf58a3612c0..905b10af5d5c 100644 > --- a/kernel/trace/trace_probe.c > +++ b/kernel/trace/trace_probe.c > @@ -178,6 +178,16 @@ void __trace_probe_log_err(int offset, int err_type) > if (!command) > return; > > + if (trace_probe_log.index >= trace_probe_log.argc) { > + /** > + * Set the error position is next to the last arg + space. > + * Note that len includes the terminal null and the cursor > + * appaers at pos + 1. > + */ > + pos = len; > + offset = 0; > + } > + > /* And make a command string from argv array */ > p = command; > for (i = 0; i < trace_probe_log.argc; i++) { > @@ -1084,6 +1094,12 @@ int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b) > { > int i; > > + /* In case of more arguments */ > + if (a->nr_args < b->nr_args) > + return a->nr_args + 1; > + if (a->nr_args > b->nr_args) > + return b->nr_args + 1; > + > for (i = 0; i < a->nr_args; i++) { > if ((b->nr_args <= i) || > ((a->args[i].type != b->args[i].type) || > -- > 2.20.1 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe 2019-09-28 21:11 ` Steven Rostedt 2019-09-29 8:14 ` Masami Hiramatsu @ 2019-09-30 10:28 ` Srikar Dronamraju 1 sibling, 0 replies; 10+ messages in thread From: Srikar Dronamraju @ 2019-09-30 10:28 UTC (permalink / raw) To: Steven Rostedt Cc: Masami Hiramatsu, Naveen Rao, Ravi Bangoria, linux-kernel, mingo * Steven Rostedt <rostedt@goodmis.org> [2019-09-28 17:11:58]: > On Sat, 28 Sep 2019 02:59:08 -0700 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > Fix to check the difference of nr_args before adding probe > > on existing probes. This also may set the error log index > > bigger than the number of command parameters. In that case > > it sets the error position is next to the last parameter. > > > > Fixes: ca89bc071d5e ("tracing/kprobe: Add multi-probe per event support") > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > I modified the change log a bit, below is the patch I plan on submitting. > > You OK with this? > > -- Steve > > > From: Masami Hiramatsu <mhiramat@kernel.org> > Date: Sat, 28 Sep 2019 05:53:29 -0400 > Subject: [PATCH] tracing/probe: Fix to check the difference of nr_args before > adding probe > > Steven reported that a test triggered: > > ================================================================== > BUG: KASAN: slab-out-of-bounds in trace_kprobe_create+0xa9e/0xe40 > Read of size 8 at addr ffff8880c4f25a48 by task ftracetest/4798 > > CPU: 2 PID: 4798 Comm: ftracetest Not tainted 5.3.0-rc6-test+ #30 > Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v03.03 07/14/2016 > Call Trace: > dump_stack+0x7c/0xc0 > ? trace_kprobe_create+0xa9e/0xe40 > print_address_description+0x6c/0x332 > ? trace_kprobe_create+0xa9e/0xe40 > ? trace_kprobe_create+0xa9e/0xe40 > __kasan_report.cold.6+0x1a/0x3b > ? trace_kprobe_create+0xa9e/0xe40 > kasan_report+0xe/0x12 > trace_kprobe_create+0xa9e/0xe40 > ? print_kprobe_event+0x280/0x280 > ? match_held_lock+0x1b/0x240 > ? find_held_lock+0xac/0xd0 > ? fs_reclaim_release.part.112+0x5/0x20 > ? lock_downgrade+0x350/0x350 > ? kasan_unpoison_shadow+0x30/0x40 > ? __kasan_kmalloc.constprop.6+0xc1/0xd0 > ? trace_kprobe_create+0xe40/0xe40 > ? trace_kprobe_create+0xe40/0xe40 > create_or_delete_trace_kprobe+0x2e/0x60 > trace_run_command+0xc3/0xe0 > ? trace_panic_handler+0x20/0x20 > ? kasan_unpoison_shadow+0x30/0x40 > trace_parse_run_command+0xdc/0x163 > vfs_write+0xe1/0x240 > ksys_write+0xba/0x150 > ? __ia32_sys_read+0x50/0x50 > ? tracer_hardirqs_on+0x61/0x180 > ? trace_hardirqs_off_caller+0x43/0x110 > ? mark_held_locks+0x29/0xa0 > ? do_syscall_64+0x14/0x260 > do_syscall_64+0x68/0x260 > > Fix to check the difference of nr_args before adding probe > on existing probes. This also may set the error log index > bigger than the number of command parameters. In that case > it sets the error position is next to the last parameter. > > Link: http://lkml.kernel.org/r/156966474783.3478.13217501608215769150.stgit@devnote2 > Looks good to me. Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com> > Fixes: ca89bc071d5e ("tracing/kprobe: Add multi-probe per event support") > Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/trace_probe.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > -- Thanks and Regards Srikar Dronamraju ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-09-30 10:29 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-27 9:50 [PATCH] tracing/probe: Test nr_args match in looking for same probe events Steven Rostedt 2019-09-27 13:38 ` Srikar Dronamraju 2019-09-27 15:06 ` Steven Rostedt 2019-09-27 17:54 ` Srikar Dronamraju 2019-09-28 8:17 ` Masami Hiramatsu 2019-09-28 9:56 ` Masami Hiramatsu 2019-09-28 9:59 ` [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe Masami Hiramatsu 2019-09-28 21:11 ` Steven Rostedt 2019-09-29 8:14 ` Masami Hiramatsu 2019-09-30 10:28 ` Srikar Dronamraju
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).