tracing/probe: fix memleak in fetch_op_data operations

PATCH In review Vamshi K Sthambamkadi 6 participants 6 messages Jun 15, 2020 – Jun 16, 2020 Tracing

lore

Share & tools

lore

Vamshi K Sthambamkadi

author

2020-06-15 14:30 UTC #0 lore raw

PATCH 1/1 tracing/probe: fix memleak in fetch_op_data operations

kmemleak report:

    [<57dcc2ca>] __kmalloc_track_caller+0x139/0x2b0
    [<f1c45d0f>] kstrndup+0x37/0x80
    [<f9761eb0>] parse_probe_arg.isra.7+0x3cc/0x630
    [<055bf2ba>] traceprobe_parse_probe_arg+0x2f5/0x810
    [<655a7766>] trace_kprobe_create+0x2ca/0x950
    [<4fc6a02a>] create_or_delete_trace_kprobe+0xf/0x30
    [<6d1c8a52>] trace_run_command+0x67/0x80
    [<be812cc0>] trace_parse_run_command+0xa7/0x140
    [<aecfe401>] probes_write+0x10/0x20
    [<2027641c>] __vfs_write+0x30/0x1e0
    [<6a4aeee1>] vfs_write+0x96/0x1b0
    [<3517fb7d>] ksys_write+0x53/0xc0
    [<dad91db7>] __ia32_sys_write+0x15/0x20
    [<da347f64>] do_syscall_32_irqs_on+0x3d/0x260
    [<fd0b7e7d>] do_fast_syscall_32+0x39/0xb0
    [<ea5ae810>] entry_SYSENTER_32+0xaf/0x102

Post parse_probe_arg(), the FETCH_OP_DATA operation type is overwritten to FETCH_OP_ST_STRING, as a result memory is never freed since traceprobe_free_probe_arg() iterates only over SYMBOL and DATA op types

Setup fetch string operation correctly after fetch_op_data operation.

Signed-off-by Vamshi K Sthambamkadi <[email protected]>

---
 kernel/trace/trace_probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/trace/trace_probe.c +2 -2

index b8a928e..d2867cc 100644
@@ -639,8 +639,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
 			ret = -EINVAL;
 			goto fail;
 		}
-		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM) ||
-		     parg->count) {
+		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
+		     code->op == FETCH_OP_DATA) || parg->count) {
 			/*
 			 * IMM, DATA and COMM is pointing actual address, those
 			 * must be kept, and if parg->count != 0, this is an

Signature · 1 line

2.7.4

Steven Rostedt

maintainer

2020-06-15 21:13 UTC #1 lore raw

Masami or Srikar would you like to look at this patch.

And wondering why you were not on the Cc to this patch in the first place, please take a look at the patch I want to add at the bottom ;-)

Quoted 34 lines · On Mon, 15 Jun 2020 20:00:38 +0530 Vamshi K Sthambamkadi…

kmemleak report:

    [<57dcc2ca>] __kmalloc_track_caller+0x139/0x2b0
    [<f1c45d0f>] kstrndup+0x37/0x80
    [<f9761eb0>] parse_probe_arg.isra.7+0x3cc/0x630
    [<055bf2ba>] traceprobe_parse_probe_arg+0x2f5/0x810
    [<655a7766>] trace_kprobe_create+0x2ca/0x950
    [<4fc6a02a>] create_or_delete_trace_kprobe+0xf/0x30
    [<6d1c8a52>] trace_run_command+0x67/0x80
    [<be812cc0>] trace_parse_run_command+0xa7/0x140
    [<aecfe401>] probes_write+0x10/0x20
    [<2027641c>] __vfs_write+0x30/0x1e0
    [<6a4aeee1>] vfs_write+0x96/0x1b0
    [<3517fb7d>] ksys_write+0x53/0xc0
    [<dad91db7>] __ia32_sys_write+0x15/0x20
    [<da347f64>] do_syscall_32_irqs_on+0x3d/0x260
    [<fd0b7e7d>] do_fast_syscall_32+0x39/0xb0
    [<ea5ae810>] entry_SYSENTER_32+0xaf/0x102

Setup fetch string operation correctly after fetch_op_data operation.

Signed-off-by Vamshi K Sthambamkadi <[email protected]>

---
 kernel/trace/trace_probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/trace/trace_probe.c +0 -0

index b8a928e..d2867cc 100644

kernel/trace/trace_probe.c static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size, · line 646

Quoted 10 lines

kernel/trace/trace_probe.c +2 -2

@@ -639,8 +639,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
 			ret = -EINVAL;
 			goto fail;
 		}
-		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM) ||
-		     parg->count) {
+		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
+		     code->op == FETCH_OP_DATA) || parg->count) {
 			/*
 			 * IMM, DATA and COMM is pointing actual address, those
 			 * must be kept, and if parg->count != 0, this is an

MAINTAINERS +12 -0

index 47873f2e6696..116e5cc7ef95 100644
@@ -9229,6 +9229,8 @@ F:	Documentation/kprobes.txt
 F:	include/linux/kprobes.h
 F:	include/asm-generic/kprobes.h
 F:	kernel/kprobes.c
+F:	kernel/trace/trace_kprobe.c
+F:	kernel/trace/trace_probe.c
 
 KS0108 LCD CONTROLLER DRIVER
 M:	Miguel Ojeda Sandonis <[email protected]>

@@ -16996,6 +16998,16 @@ F:	drivers/mtd/ubi/
 F:	include/linux/mtd/ubi.h
 F:	include/uapi/mtd/ubi-user.h
 
+UPROBES
+M:	Srikar Dronamraju <[email protected]>
+S:	Maintained
+F:	Documentation/trace/uprobetracer.rst
+F:	Documentation/features/debug/uprobes
+F:	include/linux/uprobes.h
+F:	kernel/events/uprobes.c
+F:	kernel/trace/trace_uprobe.c
+F:	kernel/trace/trace_probe.c
+
 USB "USBNET" DRIVER FRAMEWORK
 M:	Oliver Neukum <[email protected]>
 L:	[email protected]

-- Steve

Masami Hiramatsu

maintainer Acked-by

2020-06-16 08:46 UTC #2 lore raw

Hi Steve,

Quoted 2 lines · On Mon, 15 Jun 2020 17:13:37 -0400 Steven Rostedt <[email protected]> wrote:

Masami or Srikar would you like to look at this patch.

Thanks for letting me know!

Quoted 3 lines

And wondering why you were not on the Cc to this patch in the first place, please take a look at the patch I want to add at the bottom ;-)

Agreed to expand MAINTAINERS to clarify that. For the kprobes part,

Acked-by Masami Hiramatsu <[email protected]>

BTW, for uprobes, I think Oleg is mainly reviewed changes on that in these days. Maybe better to assign him?

Thanks,

Quoted 84 lines

Quoted 44 lines · On Mon, 15 Jun 2020 20:00:38 +0530 Vamshi K Sthambamkadi…

kmemleak report:

    [<57dcc2ca>] __kmalloc_track_caller+0x139/0x2b0
    [<f1c45d0f>] kstrndup+0x37/0x80
    [<f9761eb0>] parse_probe_arg.isra.7+0x3cc/0x630
    [<055bf2ba>] traceprobe_parse_probe_arg+0x2f5/0x810
    [<655a7766>] trace_kprobe_create+0x2ca/0x950
    [<4fc6a02a>] create_or_delete_trace_kprobe+0xf/0x30
    [<6d1c8a52>] trace_run_command+0x67/0x80
    [<be812cc0>] trace_parse_run_command+0xa7/0x140
    [<aecfe401>] probes_write+0x10/0x20
    [<2027641c>] __vfs_write+0x30/0x1e0
    [<6a4aeee1>] vfs_write+0x96/0x1b0
    [<3517fb7d>] ksys_write+0x53/0xc0
    [<dad91db7>] __ia32_sys_write+0x15/0x20
    [<da347f64>] do_syscall_32_irqs_on+0x3d/0x260
    [<fd0b7e7d>] do_fast_syscall_32+0x39/0xb0
    [<ea5ae810>] entry_SYSENTER_32+0xaf/0x102

Setup fetch string operation correctly after fetch_op_data operation.

Signed-off-by Vamshi K Sthambamkadi <[email protected]>

---
 kernel/trace/trace_probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/trace/trace_probe.c +2 -2

index b8a928e..d2867cc 100644
@@ -639,8 +639,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
 			ret = -EINVAL;
 			goto fail;
 		}
-		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM) ||
-		     parg->count) {
+		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
+		     code->op == FETCH_OP_DATA) || parg->count) {
 			/*
 			 * IMM, DATA and COMM is pointing actual address, those
 			 * must be kept, and if parg->count != 0, this is an

MAINTAINERS +12 -1

index 47873f2e6696..116e5cc7ef95 100644
@@ -9229,6 +9229,8 @@ F:	Documentation/kprobes.txt
 F:	include/linux/kprobes.h
 F:	include/asm-generic/kprobes.h
 F:	kernel/kprobes.c
+F:	kernel/trace/trace_kprobe.c
+F:	kernel/trace/trace_probe.c
 
 KS0108 LCD CONTROLLER DRIVER
 M:	Miguel Ojeda Sandonis <[email protected]>

@@ -16996,6 +16998,16 @@ F:	drivers/mtd/ubi/
 F:	include/linux/mtd/ubi.h
 F:	include/uapi/mtd/ubi-user.h
 
+UPROBES
+M:	Srikar Dronamraju <[email protected]>
+S:	Maintained
+F:	Documentation/trace/uprobetracer.rst
+F:	Documentation/features/debug/uprobes
+F:	include/linux/uprobes.h
+F:	kernel/events/uprobes.c
+F:	kernel/trace/trace_uprobe.c
+F:	kernel/trace/trace_probe.c
+
 USB "USBNET" DRIVER FRAMEWORK
 M:	Oliver Neukum <[email protected]>
 L:	[email protected]
 
 
-- Steve

Signature · 1 line

Masami Hiramatsu <[email protected]>

Oleg Nesterov

2020-06-16 14:19 UTC #3 lore raw

Quoted 3 lines · Masami Hiramatsu

BTW, for uprobes, I think Oleg is mainly reviewed changes on that in these days. Maybe better to assign him?

Well, then I'd suggest both me and Srikar.

Oleg.

Srikar Dronamraju

2020-06-16 16:22 UTC #4 lore raw

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Masami Hiramatsu
BTW, for uprobes, I think Oleg is mainly reviewed changes on that in these days. Maybe better to assign him?
Well, then I'd suggest both me and Srikar.

Agree with Oleg. Actually he is more upto date with uprobes code.

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Masami Hiramatsu

Acked-by on v1 1/1

2020-06-16 08:41 UTC #5 lore raw

In reply to Vamshi K Sthambamkadi

Quoted 18 lines · On Mon, 15 Jun 2020 20:00:38 +0530 Vamshi K Sthambamkadi…

kmemleak report:

    [<57dcc2ca>] __kmalloc_track_caller+0x139/0x2b0
    [<f1c45d0f>] kstrndup+0x37/0x80
    [<f9761eb0>] parse_probe_arg.isra.7+0x3cc/0x630
    [<055bf2ba>] traceprobe_parse_probe_arg+0x2f5/0x810
    [<655a7766>] trace_kprobe_create+0x2ca/0x950
    [<4fc6a02a>] create_or_delete_trace_kprobe+0xf/0x30
    [<6d1c8a52>] trace_run_command+0x67/0x80
    [<be812cc0>] trace_parse_run_command+0xa7/0x140
    [<aecfe401>] probes_write+0x10/0x20
    [<2027641c>] __vfs_write+0x30/0x1e0
    [<6a4aeee1>] vfs_write+0x96/0x1b0
    [<3517fb7d>] ksys_write+0x53/0xc0
    [<dad91db7>] __ia32_sys_write+0x15/0x20
    [<da347f64>] do_syscall_32_irqs_on+0x3d/0x260
    [<fd0b7e7d>] do_fast_syscall_32+0x39/0xb0
    [<ea5ae810>] entry_SYSENTER_32+0xaf/0x102

Quoted 6 lines

Post parse_probe_arg(), the FETCH_OP_DATA operation type is overwritten to FETCH_OP_ST_STRING, as a result memory is never freed since traceprobe_free_probe_arg() iterates only over SYMBOL and DATA op types
Setup fetch string operation correctly after fetch_op_data operation.

Oops, Good catch! (I've commented it but not coded...)

Acked-by Masami Hiramatsu <[email protected]>

And,

Fixes a42e3c4de964 ("tracing/probe: Add immediate string parameter support")

Cc [email protected]

Thank you!

Quoted 23 lines

Signed-off-by Vamshi K Sthambamkadi <[email protected]>

---
 kernel/trace/trace_probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/trace/trace_probe.c +2 -2

index b8a928e..d2867cc 100644
@@ -639,8 +639,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
 			ret = -EINVAL;
 			goto fail;
 		}
-		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM) ||
-		     parg->count) {
+		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
+		     code->op == FETCH_OP_DATA) || parg->count) {
 			/*
 			 * IMM, DATA and COMM is pointing actual address, those
 			 * must be kept, and if parg->count != 0, this is an

Signature · 1 line

2.7.4

Signature · 1 line

Masami Hiramatsu