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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20190927131458.GA19008@linux.vnet.ibm.com>
Date:   Fri, 27 Sep 2019 19:08:53 +0530
From:   Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Naveen Rao <naveen.n.rao@...ux.vnet.ibm.com>,
        Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Subject: Re: [PATCH] tracing/probe: Test nr_args match in looking for same
 probe events

<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@...dmis.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