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

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

[Masami Hiramatsu]

Hi,

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

In this version I updated patch [1/3] according to Steve's comment.


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.h      |    1 +
 kernel/trace/trace_probe_tmpl.h |    2 +-
 kernel/trace/trace_uprobe.c     |   13 +++++++++++--
 4 files changed, 20 insertions(+), 9 deletions(-)

--
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH v3 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.

Note that this uses -ECOMM as a token value to fetch the comm
string. If the user-space pointer points -ECOMM, it will be
translated to task->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>
---
  Changes in v3
   - Always add +1 to strlen() result.
   - Use -ECOMM as a token to specify current->comm.
---
 kernel/trace/trace_probe.h  |    1 +
 kernel/trace/trace_uprobe.c |   13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index b7737666c1a8..f9a8c632188b 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -124,6 +124,7 @@ struct fetch_insn {
 
 /* fetch + deref*N + store + mod + end <= 16, this allows N=12, enough */
 #define FETCH_INSN_MAX	16
+#define FETCH_TOKEN_COMM	(-ECOMM)
 
 /* Fetch type information table */
 struct fetch_type {
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index cd8750a72768..eb7e06b54741 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 == FETCH_TOKEN_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,10 @@ fetch_store_strlen(unsigned long addr)
 	int len;
 	void __user *vaddr = (void __force __user *) addr;
 
-	len = strnlen_user(vaddr, MAX_STRING_SIZE);
+	if (addr == FETCH_TOKEN_COMM)
+		len = strlen(current->comm) + 1;
+	else
+		len = strnlen_user(vaddr, MAX_STRING_SIZE);
 
 	return (len > MAX_STRING_SIZE) ? 0 : len;
 }
@@ -220,6 +226,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 = FETCH_TOKEN_COMM;
+		break;
 	case FETCH_OP_FOFFS:
 		val = translate_user_vaddr(code->immediate);
 		break;

[PATCH v3 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 4737bb8c07a3..c30c61f12ddd 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 v3 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 4cc2d467d34c..e0d1d5353464 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -533,13 +533,14 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
 			}
 		}
 	}
-	/*
-	 * 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) {
 		trace_probe_log_err(offset + (t ? (t - arg) : 0), BAD_TYPE);

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

[Steven Rostedt]

On Tue,  7 May 2019 22:55:22 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> Here is the 3rd version of series to fix several bugs in probe event
> argument parser and handler routines.
> 
> In this version I updated patch [1/3] according to Steve's comment.
> 
> 
> 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.
>

Thanks Masami,

I applied these to my queue.

-- Steve

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

[Masami Hiramatsu]

On Tue, 07 May 2019 17:47:57 +0000
Sasha Levin <sashal@kernel.org> wrote:

> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 35abb67de744 tracing: expose current->comm to [ku]probe events.
> 
> The bot has tested the following trees: v5.0.13, v4.19.40, v4.14.116, v4.9.173.
> 
> v5.0.13: Build OK!
> v4.19.40: Failed to apply! Possible dependencies:
>     40b53b771806 ("tracing: probeevent: Add array type support")
>     533059281ee5 ("tracing: probeevent: Introduce new argument fetching code")
>     56de76305279 ("tracing: probeevent: Cleanup print argument functions")
>     60c2e0cebfd0 ("tracing: probeevent: Add symbol type")
>     eeb07b061500 ("tracing: probeevent: Cleanup argument field definition")
>     f451bc89d835 ("tracing: probeevent: Unify fetch type tables")
> 
> v4.14.116: Failed to apply! Possible dependencies:
>     40b53b771806 ("tracing: probeevent: Add array type support")
>     45408c4f9250 ("tracing: kprobes: Prohibit probing on notrace function")
>     4bebdc7a85aa ("bpf: add helper bpf_perf_prog_read_value")
>     533059281ee5 ("tracing: probeevent: Introduce new argument fetching code")
>     60c2e0cebfd0 ("tracing: probeevent: Add symbol type")
>     908432ca84fc ("bpf: add helper bpf_perf_event_read_value for perf event array map")
>     97562633bcba ("bpf: perf event change needed for subsequent bpf helpers")
>     9802d86585db ("bpf: add a bpf_override_function helper")
>     b4da3340eae2 ("tracing/kprobe: bpf: Check error injectable event is on function entry")
>     cd86d1fd2102 ("bpf: Adding helper function bpf_getsockops")
>     dd0bb688eaa2 ("bpf: add a bpf_override_function helper")
>     de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>     f3edacbd697f ("bpf: Revert bpf_overrid_function() helper changes.")
>     f451bc89d835 ("tracing: probeevent: Unify fetch type tables")
> 
> v4.9.173: Failed to apply! Possible dependencies:
>     17bedab27231 ("bpf: xdp: Allow head adjustment in XDP prog")
>     1d9995771fcb ("s390: update defconfigs")
>     23a4e389bdc7 ("nfp: create separate define for max number of vectors")
>     40b53b771806 ("tracing: probeevent: Add array type support")
>     45408c4f9250 ("tracing: kprobes: Prohibit probing on notrace function")
>     533059281ee5 ("tracing: probeevent: Introduce new argument fetching code")
>     60c2e0cebfd0 ("tracing: probeevent: Add symbol type")
>     67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings scheme")
>     68453c7a8973 ("nfp: centralize runtime reconfiguration logic")
>     6b0b7551428e ("perf/core: Rename CONFIG_[UK]PROBE_EVENT to CONFIG_[UK]PROBE_EVENTS")
>     7ff5c83a1deb ("nfp: simplify nfp_net_poll()")
>     9802d86585db ("bpf: add a bpf_override_function helper")
>     a4b562bb8ebd ("nfp: use unsigned int for vector/ring counts")
>     b4da3340eae2 ("tracing/kprobe: bpf: Check error injectable event is on function entry")
>     cbeaf7aa733a ("nfp: bring back support for different ring counts")
>     ccc109b8ed24 ("net/mlx4_en: Add TX_XDP for CQ types")
>     dd0bb688eaa2 ("bpf: add a bpf_override_function helper")
>     e390b55d5aef ("bpf: make bpf_xdp_adjust_head support mandatory")
>     ecd63a0217d5 ("nfp: add XDP support in the driver")
>     f18f97ac43d7 ("tracing/kprobes: Add a helper method to return number of probe hits")
>     f3edacbd697f ("bpf: Revert bpf_overrid_function() helper changes.")
>     f451bc89d835 ("tracing: probeevent: Unify fetch type tables")
> 
> 
> How should we proceed with this patch?
> 

Thanks for the report! I missed the Fixes tag. I realized that the comm type
check was done in other place in older kernel. This bug was introduced by
commit 533059281ee5 ("tracing: probeevent: Introduce new argument fetching code").

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Steven Rostedt]

On Wed, 8 May 2019 12:24:03 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > How should we proceed with this patch?
> >   
> 
> Thanks for the report! I missed the Fixes tag. I realized that the comm type
> check was done in other place in older kernel. This bug was introduced by
> commit 533059281ee5 ("tracing: probeevent: Introduce new argument fetching code").

I've applied this to my queue. Should I change the Fixes to the above commit?

-- Steve

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

[Masami Hiramatsu]

On Tue, 7 May 2019 23:29:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 8 May 2019 12:24:03 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > How should we proceed with this patch?
> > >   
> > 
> > Thanks for the report! I missed the Fixes tag. I realized that the comm type
> > check was done in other place in older kernel. This bug was introduced by
> > commit 533059281ee5 ("tracing: probeevent: Introduce new argument fetching code").
> 
> I've applied this to my queue. Should I change the Fixes to the above commit?

Thanks! Yes, please update the Fixes tag to 533059281ee5, that is the correct commit.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Masami Hiramatsu]

On Tue, 7 May 2019 22:10:06 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue,  7 May 2019 22:55:22 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi,
> > 
> > Here is the 3rd version of series to fix several bugs in probe event
> > argument parser and handler routines.
> > 
> > In this version I updated patch [1/3] according to Steve's comment.
> > 
> > 
> > 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.
> >
> 
> Thanks Masami,
> 
> I applied these to my queue.

Thank you, Steve! I'll rebase other series on that.



-- 
Masami Hiramatsu <mhiramat@kernel.org>