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

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

[Masami Hiramatsu]

Hi,

Here is the 2nd version of series to fix several bugs in probe event
argument parser and handler routines.

In this version I just added Fixes tags so that it makes clear which
release should be fixed.

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

Fixes: 533059281ee5 ("tracing: probeevent: Introduce new argument fetching code")
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 v2 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.

Fixes: 40b53b771806 ("tracing: probeevent: Add array type support")
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 v2 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.

Fixes: 35abb67de744 ("tracing: expose current->comm to [ku]probe events")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.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 v2 1/3] tracing: uprobes: Re-enable $comm support for uprobe events

[Steven Rostedt]

On Fri, 18 Jan 2019 13:44:25 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> 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.
> 
> Fixes: 533059281ee5 ("tracing: probeevent: Introduce new argument fetching code")
> 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);

As user space (although only root) defines the size of the event being
stored, and we could trick addr to be current->comm (although
difficult), we could possibly leak data if maxlen is > TASK_COMM_LEN. I
would feel better if we tested maxlen against TASK_COMM_LEN in this
case.

	if (maxlen > TASK_COMM_LEN)
		maxlen = TASK_COMM_LEN;

Or if we don't think it can happen, add a WARN_ON(maxlen >
TASK_COMM_LEN).

-- Steve


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

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

[Masami Hiramatsu]

On Wed, 23 Jan 2019 03:40:05 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 18 Jan 2019 13:44:25 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > 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.
> > 
> > Fixes: 533059281ee5 ("tracing: probeevent: Introduce new argument fetching code")
> > 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);
> 
> As user space (although only root) defines the size of the event being
> stored, and we could trick addr to be current->comm (although
> difficult), we could possibly leak data if maxlen is > TASK_COMM_LEN. I
> would feel better if we tested maxlen against TASK_COMM_LEN in this
> case.
> 
> 	if (maxlen > TASK_COMM_LEN)
> 		maxlen = TASK_COMM_LEN;
> 
> Or if we don't think it can happen, add a WARN_ON(maxlen >
> TASK_COMM_LEN).

Hmm, I thought current->comm is null terminated, isn't it?
Anyway, if user can specify current->comm, he must be able to specify 
current->comm + TASK_COMM_LEN too by kprobe_events.
Moreover, it can leak any data in kernel...

And also, maxlen is calculated by fetch_store_strlen, right before
this has been called.

I rather concern the case that if we have shorter size of maxlen than
current->comm. Would we better show "(fault)" or tail-cut name ?
(of course this is very difficult to happen, since the length is
already checked.)

Thank you,

> 
> -- Steve
> 
> 
> > +	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;
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Steven Rostedt]

On Thu, 24 Jan 2019 10:43:22 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > >  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);  
> > 
> > As user space (although only root) defines the size of the event being
> > stored, and we could trick addr to be current->comm (although
> > difficult), we could possibly leak data if maxlen is > TASK_COMM_LEN. I
> > would feel better if we tested maxlen against TASK_COMM_LEN in this
> > case.
> > 
> > 	if (maxlen > TASK_COMM_LEN)
> > 		maxlen = TASK_COMM_LEN;
> > 
> > Or if we don't think it can happen, add a WARN_ON(maxlen >
> > TASK_COMM_LEN).  
> 
> Hmm, I thought current->comm is null terminated, isn't it?

Yes it is. I was thinking it was a memcpy (I blame conference fatigue ;-)

> Anyway, if user can specify current->comm, he must be able to specify 
> current->comm + TASK_COMM_LEN too by kprobe_events.
> Moreover, it can leak any data in kernel...

But this is for uprobes, which I why I was concerned.

> 
> And also, maxlen is calculated by fetch_store_strlen, right before
> this has been called.
> 
> I rather concern the case that if we have shorter size of maxlen than
> current->comm. Would we better show "(fault)" or tail-cut name ?
> (of course this is very difficult to happen, since the length is
> already checked.)

Actually, it would still be OK, as strlcpy does guarantee to be nul
terminated as long as it's greater than zero.

Hmm, strlcpy doesn't pad the rest if what is written is shorter than
what is allocated. Could that leak data?

-- Steve

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

[Masami Hiramatsu]

On Wed, 23 Jan 2019 21:09:56 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 24 Jan 2019 10:43:22 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > >  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);  
> > > 
> > > As user space (although only root) defines the size of the event being
> > > stored, and we could trick addr to be current->comm (although
> > > difficult), we could possibly leak data if maxlen is > TASK_COMM_LEN. I
> > > would feel better if we tested maxlen against TASK_COMM_LEN in this
> > > case.
> > > 
> > > 	if (maxlen > TASK_COMM_LEN)
> > > 		maxlen = TASK_COMM_LEN;
> > > 
> > > Or if we don't think it can happen, add a WARN_ON(maxlen >
> > > TASK_COMM_LEN).  
> > 
> > Hmm, I thought current->comm is null terminated, isn't it?
> 
> Yes it is. I was thinking it was a memcpy (I blame conference fatigue ;-)
> 
> > Anyway, if user can specify current->comm, he must be able to specify 
> > current->comm + TASK_COMM_LEN too by kprobe_events.
> > Moreover, it can leak any data in kernel...
> 
> But this is for uprobes, which I why I was concerned.

OK, what we will leak by this, is the address of current->comm to root
user for a specific application running instance.
Since they can try to write a data on a register and trace it like
"+0(%ax):string" and if rax register has hit the current->comm,
uprobe event will record comm, but other case, it will show "(fault)".
So they can trial and error on that (but leaking just a temporary
task instance address).

If you concern this, I can change it to store a special fixed value, like
(unsigned long)-EINVAL, instead of current->comm address. :-)

> > And also, maxlen is calculated by fetch_store_strlen, right before
> > this has been called.
> > 
> > I rather concern the case that if we have shorter size of maxlen than
> > current->comm. Would we better show "(fault)" or tail-cut name ?
> > (of course this is very difficult to happen, since the length is
> > already checked.)
> 
> Actually, it would still be OK, as strlcpy does guarantee to be nul
> terminated as long as it's greater than zero.

Ah, I meant that we can record a shorter name, like "shutdown" -> "sh". 

> Hmm, strlcpy doesn't pad the rest if what is written is shorter than
> what is allocated. Could that leak data?

Even though, there should be a data that had been recorded on the ring
buffer.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Steven Rostedt]

On Fri, 18 Jan 2019 13:44:25 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> @@ -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++;

Why only add 1 if len is non zero? Why not always do it.

One thing, len should always be greater than 0, and the other is that
this makes it inconsistent with the NULL case of reading the address in
userspace.

-- Steve


> +	} else
> +		len = strnlen_user(vaddr, MAX_STRING_SIZE);
>  
>  	return (len > MAX_STRING_SIZE) ? 0 : len;

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

[Masami Hiramatsu]

On Wed, 6 Feb 2019 15:52:43 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 18 Jan 2019 13:44:25 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > @@ -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++;
> 
> Why only add 1 if len is non zero? Why not always do it.
> 
> One thing, len should always be greater than 0, and the other is that
> this makes it inconsistent with the NULL case of reading the address in
> userspace.

Agreed, it should not 0, so it should be;
len = strlen(current->comm) + 1;

Thank you!

> 
> -- Steve
> 
> 
> > +	} else
> > +		len = strnlen_user(vaddr, MAX_STRING_SIZE);
> >  
> >  	return (len > MAX_STRING_SIZE) ? 0 : len;


-- 
Masami Hiramatsu <mhiramat@kernel.org>