Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 21 Feb 2019 16:52:52 +0900
From:   Masami Hiramatsu <mhiramat@kernel.org>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        stable@vger.kernel.org, Changbin Du <changbin.du@gmail.com>
Subject: Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access
 kernel memory that can fault
Message-Id: <20190221165252.4a9033b3348f30f9d973dbc4@kernel.org>
In-Reply-To: <20190215174945.557218316@goodmis.org>
References: <20190215174712.372898450@goodmis.org>
        <20190215174945.557218316@goodmis.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 15 Feb 2019 12:47:13 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> 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, as it can handle faulting
> kernel memory addresses, which kprobes can legitimately do.

Basically OK to me.
Could you use probe_kernel_read() in this context, since probe_mem_read() is a
wrapper function for template code.

With that change,

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

And for the long term, I need to find more efficient (or smarter) way to do it,
like strnlen_user() does.

Thank you,

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


-- 
Masami Hiramatsu <mhiramat@kernel.org>