[PATCH 0/3] tracing: probeevent: Fix probe argument parser and handler

[PATCH 0/3] tracing: probeevent: Fix probe argument parser and handler

[Masami Hiramatsu]

Hi,

This series fixes several bugs in probe event argument parser
and handler routines.

Recently I got 2 issues reported by Andreas, see
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1899098.html

One issue is already fixed by Andreas (Thanks!) but $comm handling
issue still exists on uprobe event. [1/3] fixes it.
And I found other issues around that, [2/3] is just a trivial cleanup,
[3/3] fixes $comm type issue which occurs not only uprobe events but
also on kprobe events. Anyway, after this series applied, $comm must
be "string" type and not be an array.

Thank you,

---

Masami Hiramatsu (3):
      tracing: uprobes: Re-enable $comm support for uprobe events
      tracing: probeevent: Do not accumulate on ret variable
      tracing: probeevent: Fix to make the type of $comm string


 kernel/trace/trace_probe.c      |   13 +++++++------
 kernel/trace/trace_probe_tmpl.h |    2 +-
 kernel/trace/trace_uprobe.c     |   15 +++++++++++++--
 3 files changed, 21 insertions(+), 9 deletions(-)

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

[PATCH 1/3] tracing: uprobes: Re-enable $comm support for uprobe events

[Masami Hiramatsu]

Since commit 533059281ee5 ("tracing: probeevent: Introduce new
argument fetching code") dropped the $comm support from uprobe
events, this re-enables it.

For $comm support, uses strlcpy() instead of strncpy_from_user()
to copy current task's comm. Because it is in the kernel space,
strncpy_from_user() always fails to copy the comm.
This also uses strlen() instead of strnlen_user() to measure the
length of the comm.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Reported-by: Andreas Ziegler <andreas.ziegler@fau.de>
Acked-by: Andreas Ziegler <andreas.ziegler@fau.de>
---
 kernel/trace/trace_uprobe.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 3a1d5ab6b4ba..b07e498ccbc6 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -156,7 +156,10 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
 	if (unlikely(!maxlen))
 		return -ENOMEM;
 
-	ret = strncpy_from_user(dst, src, maxlen);
+	if (addr == (unsigned long)current->comm)
+		ret = strlcpy(dst, current->comm, maxlen);
+	else
+		ret = strncpy_from_user(dst, src, maxlen);
 	if (ret >= 0) {
 		if (ret == maxlen)
 			dst[ret - 1] = '\0';
@@ -180,7 +183,12 @@ fetch_store_strlen(unsigned long addr)
 	int len;
 	void __user *vaddr = (void __force __user *) addr;
 
-	len = strnlen_user(vaddr, MAX_STRING_SIZE);
+	if (addr == (unsigned long)current->comm) {
+		len = strlen(current->comm);
+		if (len)
+			len++;
+	} else
+		len = strnlen_user(vaddr, MAX_STRING_SIZE);
 
 	return (len > MAX_STRING_SIZE) ? 0 : len;
 }
@@ -220,6 +228,9 @@ process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
 	case FETCH_OP_IMM:
 		val = code->immediate;
 		break;
+	case FETCH_OP_COMM:
+		val = (unsigned long)current->comm;
+		break;
 	case FETCH_OP_FOFFS:
 		val = translate_user_vaddr(code->immediate);
 		break;

[PATCH 2/3] tracing: probeevent: Do not accumulate on ret variable

[Masami Hiramatsu]

Do not accumulate strlen result on ret local variable, because
it is accumulated on total local variable for array case.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_probe_tmpl.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index 5c56afc17cf8..e69b05a9f57c 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -88,7 +88,7 @@ process_fetch_insn_bottom(struct fetch_insn *code, unsigned long val,
 	/* 3rd stage: store value to buffer */
 	if (unlikely(!dest)) {
 		if (code->op == FETCH_OP_ST_STRING) {
-			ret += fetch_store_strlen(val + code->offset);
+			ret = fetch_store_strlen(val + code->offset);
 			code++;
 			goto array;
 		} else

[PATCH 3/3] tracing: probeevent: Fix to make the type of $comm string

[Masami Hiramatsu]

Fix to make the type of $comm "string".
If we set the other type to $comm argument, it shows
meaningless value or wrong data. Currently probe events
allow us to set string array type (e.g. ":string[2]"), or
other digit types like x8 on $comm. But since clearly
$comm is just a string data, it should not be fetched
by other types including array.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 9962cb5da8ac..44f078cda0ac 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -405,13 +405,14 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
 				return -E2BIG;
 		}
 	}
-	/*
-	 * The default type of $comm should be "string", and it can't be
-	 * dereferenced.
-	 */
-	if (!t && strcmp(arg, "$comm") == 0)
+
+	/* Since $comm can not be dereferred, we can find $comm by strcmp */
+	if (strcmp(arg, "$comm") == 0) {
+		/* The type of $comm must be "string", and not an array. */
+		if (parg->count || (t && strcmp(t, "string")))
+			return -EINVAL;
 		parg->type = find_fetch_type("string");
-	else
+	} else
 		parg->type = find_fetch_type(t);
 	if (!parg->type) {
 		pr_info("Unsupported type: %s\n", t);

Re: [PATCH 0/3] tracing: probeevent: Fix probe argument parser and handler

[Steven Rostedt]

On Thu, 17 Jan 2019 22:47:58 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> This series fixes several bugs in probe event argument parser
> and handler routines.

Hi Masami,

Can you resend with a "Fixes:" tag to each of the patches, so that it
is easy to see what patch needs to be backported to which stable kernel
(or not any stable release if the patch fixes something in 5.0-rc).

Thanks!

-- Steve

> 
> Recently I got 2 issues reported by Andreas, see
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1899098.html
> 
> One issue is already fixed by Andreas (Thanks!) but $comm handling
> issue still exists on uprobe event. [1/3] fixes it.
> And I found other issues around that, [2/3] is just a trivial cleanup,
> [3/3] fixes $comm type issue which occurs not only uprobe events but
> also on kprobe events. Anyway, after this series applied, $comm must
> be "string" type and not be an array.
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       tracing: uprobes: Re-enable $comm support for uprobe events
>       tracing: probeevent: Do not accumulate on ret variable
>       tracing: probeevent: Fix to make the type of $comm string
> 
> 
>  kernel/trace/trace_probe.c      |   13 +++++++------
>  kernel/trace/trace_probe_tmpl.h |    2 +-
>  kernel/trace/trace_uprobe.c     |   15 +++++++++++++--
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

Re: [PATCH 0/3] tracing: probeevent: Fix probe argument parser and handler

[Masami Hiramatsu]

On Thu, 17 Jan 2019 09:23:02 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 17 Jan 2019 22:47:58 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi,
> > 
> > This series fixes several bugs in probe event argument parser
> > and handler routines.
> 
> Hi Masami,
> 
> Can you resend with a "Fixes:" tag to each of the patches, so that it
> is easy to see what patch needs to be backported to which stable kernel
> (or not any stable release if the patch fixes something in 5.0-rc).

OK, I'll resend it with Fixed tags!
And maybe I also need to enhance ftracetest so that it can catch 
this bug.

Thanks,

> 
> Thanks!
> 
> -- Steve
> 
> > 
> > Recently I got 2 issues reported by Andreas, see
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1899098.html
> > 
> > One issue is already fixed by Andreas (Thanks!) but $comm handling
> > issue still exists on uprobe event. [1/3] fixes it.
> > And I found other issues around that, [2/3] is just a trivial cleanup,
> > [3/3] fixes $comm type issue which occurs not only uprobe events but
> > also on kprobe events. Anyway, after this series applied, $comm must
> > be "string" type and not be an array.
> > 
> > Thank you,
> > 
> > ---
> > 
> > Masami Hiramatsu (3):
> >       tracing: uprobes: Re-enable $comm support for uprobe events
> >       tracing: probeevent: Do not accumulate on ret variable
> >       tracing: probeevent: Fix to make the type of $comm string
> > 
> > 
> >  kernel/trace/trace_probe.c      |   13 +++++++------
> >  kernel/trace/trace_probe_tmpl.h |    2 +-
> >  kernel/trace/trace_uprobe.c     |   15 +++++++++++++--
> >  3 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > --
> > Masami Hiramatsu (Linaro) <mhiramat@kernel.org>
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>