[PATCH 0/2] [GIT PULL] tracing: Two more fixes

[PATCH 0/2] [GIT PULL] tracing: Two more fixes

[Steven Rostedt]

Linus,

Two more tracing fixes

 - Have kprobes not use copy_from_user to access kernel addresses
   as this is now considered a security issue.

 - Put back the entries counter in the tracing output that was accidentally
   removed.

Please pull the latest trace-v5.0-rc4-2 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v5.0-rc4-2

Tag SHA1: bf7473622f886c893f828195c0e7febc0b6c7e1f
Head SHA1: 177a0fd380f825734f0307d6c5d4da5a0a9445ce


Changbin Du (1):
      kprobe: Do not use uaccess functions to access kernel memory

Quentin Perret (1):
      tracing: Fix number of entries in trace header

----
 kernel/trace/trace.c        |  2 ++
 kernel/trace/trace_kprobe.c | 10 +---------
 2 files changed, 3 insertions(+), 9 deletions(-)

[PATCH 1/2] kprobe: Do not use uaccess functions to access kernel memory

[Steven Rostedt]

From: Changbin Du <changbin.du@gmail.com>

The userspace can ask kprobe to intercept strings at any memory address,
including invalid kernel address. In this case, fetch_store_strlen()
would crash since it uses general usercopy function, and user access
functions are no longer allowed to access kernel memory.

For example, we can crash the kernel by doing something as below:

$ sudo kprobe 'p:do_sys_open +0(+0(%si)):string'

[  103.620391] BUG: GPF in non-whitelisted uaccess (non-canonical address?)
[  103.622104] general protection fault: 0000 [#1] SMP PTI
[  103.623424] CPU: 10 PID: 1046 Comm: cat Not tainted 5.0.0-rc3-00130-gd73aba1-dirty #96
[  103.625321] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.0-2-g628b2e6-dirty-20190104_103505-linux 04/01/2014
[  103.628284] RIP: 0010:process_fetch_insn+0x1ab/0x4b0
[  103.629518] Code: 10 83 80 28 2e 00 00 01 31 d2 31 ff 48 8b 74 24 28 eb 0c 81 fa ff 0f 00 00 7f 1c 85 c0 75 18 66 66 90 0f ae e8 48 63
 ca 89 f8 <8a> 0c 31 66 66 90 83 c2 01 84 c9 75 dc 89 54 24 34 89 44 24 28 48
[  103.634032] RSP: 0018:ffff88845eb37ce0 EFLAGS: 00010246
[  103.635312] RAX: 0000000000000000 RBX: ffff888456c4e5a8 RCX: 0000000000000000
[  103.637057] RDX: 0000000000000000 RSI: 2e646c2f6374652f RDI: 0000000000000000
[  103.638795] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[  103.640556] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
[  103.642297] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[  103.644040] FS:  0000000000000000(0000) GS:ffff88846f000000(0000) knlGS:0000000000000000
[  103.646019] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  103.647436] CR2: 00007ffc79758038 CR3: 0000000463360006 CR4: 0000000000020ee0
[  103.649147] Call Trace:
[  103.649781]  ? sched_clock_cpu+0xc/0xa0
[  103.650747]  ? do_sys_open+0x5/0x220
[  103.651635]  kprobe_trace_func+0x303/0x380
[  103.652645]  ? do_sys_open+0x5/0x220
[  103.653528]  kprobe_dispatcher+0x45/0x50
[  103.654682]  ? do_sys_open+0x1/0x220
[  103.655875]  kprobe_ftrace_handler+0x90/0xf0
[  103.657282]  ftrace_ops_assist_func+0x54/0xf0
[  103.658564]  ? __call_rcu+0x1dc/0x280
[  103.659482]  0xffffffffc00000bf
[  103.660384]  ? __ia32_sys_open+0x20/0x20
[  103.661682]  ? do_sys_open+0x1/0x220
[  103.662863]  do_sys_open+0x5/0x220
[  103.663988]  do_syscall_64+0x60/0x210
[  103.665201]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  103.666862] RIP: 0033:0x7fc22fadccdd
[  103.668034] Code: 48 89 54 24 e0 41 83 e2 40 75 32 89 f0 25 00 00 41 00 3d 00 00 41 00 74 24 89 f2 b8 01 01 00 00 48 89 fe bf 9c ff ff
 ff 0f 05 <48> 3d 00 f0 ff ff 77 33 f3 c3 66 0f 1f 84 00 00 00 00 00 48 8d 44
[  103.674029] RSP: 002b:00007ffc7972c3a8 EFLAGS: 00000287 ORIG_RAX: 0000000000000101
[  103.676512] RAX: ffffffffffffffda RBX: 0000562f86147a21 RCX: 00007fc22fadccdd
[  103.678853] RDX: 0000000000080000 RSI: 00007fc22fae1428 RDI: 00000000ffffff9c
[  103.681151] RBP: ffffffffffffffff R08: 0000000000000000 R09: 0000000000000000
[  103.683489] R10: 0000000000000000 R11: 0000000000000287 R12: 00007fc22fce90a8
[  103.685774] R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000000
[  103.688056] Modules linked in:
[  103.689131] ---[ end trace 43792035c28984a1 ]---

This can be fixed by using probe_mem_read() instead.

Link: http://lkml.kernel.org/r/20190125151051.7381-1-changbin.du@gmail.com

Cc: stable@vger.kernel.org
Fixes: 9da3f2b7405 ("x86/fault: BUG() when uaccess helpers fault on kernel addresses")
Signed-off-by: Changbin Du <changbin.du@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_kprobe.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index d5fb09ebba8b..9eaf07f99212 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -861,22 +861,14 @@ static const struct file_operations kprobe_profile_ops = {
 static nokprobe_inline int
 fetch_store_strlen(unsigned long addr)
 {
-	mm_segment_t old_fs;
 	int ret, len = 0;
 	u8 c;
 
-	old_fs = get_fs();
-	set_fs(KERNEL_DS);
-	pagefault_disable();
-
 	do {
-		ret = __copy_from_user_inatomic(&c, (u8 *)addr + len, 1);
+		ret = probe_mem_read(&c, (u8 *)addr + len, 1);
 		len++;
 	} while (c && ret == 0 && len < MAX_STRING_SIZE);
 
-	pagefault_enable();
-	set_fs(old_fs);
-
 	return (ret < 0) ? ret : len;
 }
 
-- 
2.20.1

[PATCH 2/2] tracing: Fix number of entries in trace header

[Steven Rostedt]

From: Quentin Perret <quentin.perret@arm.com>

The following commit

  441dae8f2f29 ("tracing: Add support for display of tgid in trace output")

removed the call to print_event_info() from print_func_help_header_irq()
which results in the ftrace header not reporting the number of entries
written in the buffer. As this wasn't the original intent of the patch,
re-introduce the call to print_event_info() to restore the orginal
behaviour.

Link: http://lkml.kernel.org/r/20190214152950.4179-1-quentin.perret@arm.com

Acked-by: Joel Fernandes <joelaf@google.com>
Cc: stable@vger.kernel.org
Fixes: 441dae8f2f29 ("tracing: Add support for display of tgid in trace output")
Signed-off-by: Quentin Perret <quentin.perret@arm.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c521b7347482..c4238b441624 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3384,6 +3384,8 @@ static void print_func_help_header_irq(struct trace_buffer *buf, struct seq_file
 	const char tgid_space[] = "          ";
 	const char space[] = "  ";
 
+	print_event_info(buf, m);
+
 	seq_printf(m, "#                          %s  _-----=> irqs-off\n",
 		   tgid ? tgid_space : space);
 	seq_printf(m, "#                          %s / _----=> need-resched\n",
-- 
2.20.1

Re: [PATCH 0/2] [GIT PULL] tracing: Two more fixes

[Linus Torvalds]

On Fri, Feb 15, 2019 at 6:21 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>  - Have kprobes not use copy_from_user to access kernel addresses
>    as this is now considered a security issue.

No, you people are confused.

The problem isn't that it's using a user access function on kernel memory.

The problem is that it's using a user access function on a complete
garbage pointer that happens to not even be a valid pointer at all.

You get a GP fault because the code tries to access an address at
0x2e646c2f6374652f.

That's not a valid pointer on x86-64. Nothing to do with user or
kernel, everything to do with "it's garbage".

Switching over to probe_mem_read() just means that even non-canonical
address faults are ignored. But it has absolutely nothing to do with
"kernel addresses" or any security issues.

So the patch looks like it might be ok, but the explanations for it
are garbage and only confuse the issue.

Please fix the explanations, I don't want to have actively wrong
commit messages for when people start looking at things like this.

                   Linus

Re: [PATCH 0/2] [GIT PULL] tracing: Two more fixes

[Steven Rostedt]

On Fri, 15 Feb 2019 09:08:38 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Feb 15, 2019 at 6:21 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >  - Have kprobes not use copy_from_user to access kernel addresses
> >    as this is now considered a security issue.  
> 
> No, you people are confused.
> 
> The problem isn't that it's using a user access function on kernel memory.
> 
> The problem is that it's using a user access function on a complete
> garbage pointer that happens to not even be a valid pointer at all.
> 
> You get a GP fault because the code tries to access an address at
> 0x2e646c2f6374652f.
> 
> That's not a valid pointer on x86-64. Nothing to do with user or
> kernel, everything to do with "it's garbage".
> 
> Switching over to probe_mem_read() just means that even non-canonical
> address faults are ignored. But it has absolutely nothing to do with
> "kernel addresses" or any security issues.
> 
> So the patch looks like it might be ok, but the explanations for it
> are garbage and only confuse the issue.
> 
> Please fix the explanations, I don't want to have actively wrong
> commit messages for when people start looking at things like this.
> 

OK, I'll update the change log. Yeah, the bug is that we are reading
possibly bad kernel memory, which is what kprobes do.

Will update.

-- Steve