[PATCH] tracing/probe: Test nr_args match in looking for same probe events

[PATCH] tracing/probe: Test nr_args match in looking for same probe events

[Steven Rostedt]

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

Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events

[Srikar Dronamraju]

<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

Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events

[Steven Rostedt]

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

Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events

[Srikar Dronamraju]

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

Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events

[Masami Hiramatsu]

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

Re: [PATCH] tracing/probe: Test nr_args match in looking for same probe events

[Masami Hiramatsu]

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>

[PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe

[Masami Hiramatsu]

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

Re: [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe

[Steven Rostedt]

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

Re: [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe

[Masami Hiramatsu]

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>

Re: [PATCH] tracing/probe: Fix to check the difference of nr_args before adding probe

[Srikar Dronamraju]

* 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