linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] wchan: Fix ORC support and leaky fallback
@ 2021-09-24  6:20 Kees Cook
  2021-09-24  6:20 ` [PATCH 1/3] Revert "proc/wchan: use printk format instead of lookup_symbol_name()" Kees Cook
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Kees Cook @ 2021-09-24  6:20 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Helge Deller, Qi Zheng, Vito Caputo, Josh Poimboeuf,
	Jann Horn, Tobin C. Harding, Tycho Andersen, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Mark Rutland,
	Jens Axboe, Peter Zijlstra, Andy Lutomirski, Lai Jiangshan,
	Stefan Metzmacher, Dave Hansen, Christian Brauner, Michal Hocko,
	Eric W. Biederman, Randy Dunlap, Ohhoon Kwon, YiFei Zhu,
	kernel test robot, linux-kernel, stable, linux-hardening, x86,
	linux-fsdevel

Hi,

This attempts to solve the issues from the discussion
here[1]. Specifically:

1) wchan has been broken under ORC, seen as a failure to stack walk
   resulting in _usually_ a 0 value, since ee9f8fce9964 (v4.14).

2) wchan leaking raw addresses since 152c432b128c (v5.12).

Based on what I can see in the stack walking code, the fix should be
safe. Jann may have more thoughts, but from what I can see, the walker
pins the stack, decodes only a single step, etc.

I'd like Josh's review of Qi Zheng's patch, though. :)

It's also not clear to me what impact this had on kernel/sched/fair.c:
it would have also been seeing 0s, so this may be fixing a bug there too.

Thanks!

-Kees

[1] https://lore.kernel.org/lkml/20210924054647.v6x6risoa4jhuu6s@shells.gnugeneration.com/

Kees Cook (2):
  Revert "proc/wchan: use printk format instead of lookup_symbol_name()"
  leaking_addresses: Always print a trailing newline

Qi Zheng (1):
  x86: Fix get_wchan() to support the ORC unwinder

 arch/x86/kernel/process.c    | 51 +++---------------------------------
 fs/proc/base.c               | 19 ++++++++------
 scripts/leaking_addresses.pl |  3 ++-
 3 files changed, 16 insertions(+), 57 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/3] Revert "proc/wchan: use printk format instead of lookup_symbol_name()"
  2021-09-24  6:20 [PATCH 0/3] wchan: Fix ORC support and leaky fallback Kees Cook
@ 2021-09-24  6:20 ` Kees Cook
  2021-09-24  8:17   ` Helge Deller
  2021-09-24  6:20 ` [PATCH 2/3] leaking_addresses: Always print a trailing newline Kees Cook
  2021-09-24  6:20 ` [PATCH 3/3] x86: Fix get_wchan() to support the ORC unwinder Kees Cook
  2 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2021-09-24  6:20 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, kernel test robot, Vito Caputo, Jann Horn, stable,
	Helge Deller, Qi Zheng, Josh Poimboeuf, Tobin C. Harding,
	Tycho Andersen, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, Mark Rutland, Jens Axboe, Peter Zijlstra,
	Andy Lutomirski, Lai Jiangshan, Stefan Metzmacher, Dave Hansen,
	Christian Brauner, Michal Hocko, Eric W. Biederman, Randy Dunlap,
	Ohhoon Kwon, YiFei Zhu, linux-kernel, linux-hardening, x86,
	linux-fsdevel

This reverts commit 152c432b128cb043fc107e8f211195fe94b2159c.

When a kernel address couldn't be symbolized for /proc/$pid/wchan, it
would leak the raw value, a potential information exposure. This is a
regression compared to the safer pre-v5.12 behavior.

Reported-by: kernel test robot <oliver.sang@intel.com>
Link: https://lore.kernel.org/all/20210103142726.GC30643@xsang-OptiPlex-9020/
Reported-by: Vito Caputo <vcaputo@pengaru.com>
Link: https://lore.kernel.org/lkml/20210921193249.el476vlhg5k6lfcq@shells.gnugeneration.com/
Reported-by: Jann Horn <jannh@google.com>
Link: https://lore.kernel.org/lkml/CAG48ez2zC=+PuNgezH53HBPZ8CXU5H=vkWx7nJs60G8RXt3w0Q@mail.gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/proc/base.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 533d5836eb9a..1f394095eb88 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -67,6 +67,7 @@
 #include <linux/mm.h>
 #include <linux/swap.h>
 #include <linux/rcupdate.h>
+#include <linux/kallsyms.h>
 #include <linux/stacktrace.h>
 #include <linux/resource.h>
 #include <linux/module.h>
@@ -386,17 +387,19 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
 			  struct pid *pid, struct task_struct *task)
 {
 	unsigned long wchan;
+	char symname[KSYM_NAME_LEN];
 
-	if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
-		wchan = get_wchan(task);
-	else
-		wchan = 0;
+	if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
+		goto print0;
 
-	if (wchan)
-		seq_printf(m, "%ps", (void *) wchan);
-	else
-		seq_putc(m, '0');
+	wchan = get_wchan(task);
+	if (wchan && !lookup_symbol_name(wchan, symname)) {
+		seq_puts(m, symname);
+		return 0;
+	}
 
+print0:
+	seq_putc(m, '0');
 	return 0;
 }
 #endif /* CONFIG_KALLSYMS */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/3] leaking_addresses: Always print a trailing newline
  2021-09-24  6:20 [PATCH 0/3] wchan: Fix ORC support and leaky fallback Kees Cook
  2021-09-24  6:20 ` [PATCH 1/3] Revert "proc/wchan: use printk format instead of lookup_symbol_name()" Kees Cook
@ 2021-09-24  6:20 ` Kees Cook
  2021-09-24  6:20 ` [PATCH 3/3] x86: Fix get_wchan() to support the ORC unwinder Kees Cook
  2 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2021-09-24  6:20 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Tobin C. Harding, Tycho Andersen, Helge Deller,
	Qi Zheng, Vito Caputo, Josh Poimboeuf, Jann Horn,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Mark Rutland, Jens Axboe, Peter Zijlstra, Andy Lutomirski,
	Lai Jiangshan, Stefan Metzmacher, Dave Hansen, Christian Brauner,
	Michal Hocko, Eric W. Biederman, Randy Dunlap, Ohhoon Kwon,
	YiFei Zhu, kernel test robot, linux-kernel, stable,
	linux-hardening, x86, linux-fsdevel

For files that lack trailing newlines and match a leaking address (e.g.
wchan[1]), the leaking_addresses.pl report would run together with the
net line, making things look corrupted.

Unconditionally remove the newline on input, and write it back out on
output.

[1] https://lore.kernel.org/all/20210103142726.GC30643@xsang-OptiPlex-9020/

Cc: "Tobin C. Harding" <me@tobin.cc>
Cc: Tycho Andersen <tycho@tycho.pizza>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/leaking_addresses.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/leaking_addresses.pl b/scripts/leaking_addresses.pl
index b2d8b8aa2d99..8f636a23bc3f 100755
--- a/scripts/leaking_addresses.pl
+++ b/scripts/leaking_addresses.pl
@@ -455,8 +455,9 @@ sub parse_file
 
 	open my $fh, "<", $file or return;
 	while ( <$fh> ) {
+		chomp;
 		if (may_leak_address($_)) {
-			print $file . ': ' . $_;
+			printf("$file: $_\n");
 		}
 	}
 	close $fh;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 3/3] x86: Fix get_wchan() to support the ORC unwinder
  2021-09-24  6:20 [PATCH 0/3] wchan: Fix ORC support and leaky fallback Kees Cook
  2021-09-24  6:20 ` [PATCH 1/3] Revert "proc/wchan: use printk format instead of lookup_symbol_name()" Kees Cook
  2021-09-24  6:20 ` [PATCH 2/3] leaking_addresses: Always print a trailing newline Kees Cook
@ 2021-09-24  6:20 ` Kees Cook
  2 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2021-09-24  6:20 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Qi Zheng, stable, Helge Deller, Vito Caputo,
	Josh Poimboeuf, Jann Horn, Tobin C. Harding, Tycho Andersen,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Mark Rutland, Jens Axboe, Peter Zijlstra, Andy Lutomirski,
	Lai Jiangshan, Stefan Metzmacher, Dave Hansen, Christian Brauner,
	Michal Hocko, Eric W. Biederman, Randy Dunlap, Ohhoon Kwon,
	YiFei Zhu, kernel test robot, linux-kernel, linux-hardening, x86,
	linux-fsdevel

From: Qi Zheng <zhengqi.arch@bytedance.com>

Currently, the kernel CONFIG_UNWINDER_ORC option is enabled by default
on x86, but the implementation of get_wchan() is still based on the frame
pointer unwinder, so the /proc/<pid>/wchan usually returned 0 regardless
of whether the task <pid> is running.

Reimplement get_wchan() by calling stack_trace_save_tsk(), which is
adapted to the ORC and frame pointer unwinders.

Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
Link: https://lore.kernel.org/r/20210831083625.59554-1-zhengqi.arch@bytedance.com
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/kernel/process.c | 51 +++------------------------------------
 1 file changed, 3 insertions(+), 48 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 1d9463e3096b..e645925f9f02 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -944,58 +944,13 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
  */
 unsigned long get_wchan(struct task_struct *p)
 {
-	unsigned long start, bottom, top, sp, fp, ip, ret = 0;
-	int count = 0;
+	unsigned long entry = 0;
 
 	if (p == current || task_is_running(p))
 		return 0;
 
-	if (!try_get_task_stack(p))
-		return 0;
-
-	start = (unsigned long)task_stack_page(p);
-	if (!start)
-		goto out;
-
-	/*
-	 * Layout of the stack page:
-	 *
-	 * ----------- topmax = start + THREAD_SIZE - sizeof(unsigned long)
-	 * PADDING
-	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
-	 * stack
-	 * ----------- bottom = start
-	 *
-	 * The tasks stack pointer points at the location where the
-	 * framepointer is stored. The data on the stack is:
-	 * ... IP FP ... IP FP
-	 *
-	 * We need to read FP and IP, so we need to adjust the upper
-	 * bound by another unsigned long.
-	 */
-	top = start + THREAD_SIZE - TOP_OF_KERNEL_STACK_PADDING;
-	top -= 2 * sizeof(unsigned long);
-	bottom = start;
-
-	sp = READ_ONCE(p->thread.sp);
-	if (sp < bottom || sp > top)
-		goto out;
-
-	fp = READ_ONCE_NOCHECK(((struct inactive_task_frame *)sp)->bp);
-	do {
-		if (fp < bottom || fp > top)
-			goto out;
-		ip = READ_ONCE_NOCHECK(*(unsigned long *)(fp + sizeof(unsigned long)));
-		if (!in_sched_functions(ip)) {
-			ret = ip;
-			goto out;
-		}
-		fp = READ_ONCE_NOCHECK(*(unsigned long *)fp);
-	} while (count++ < 16 && !task_is_running(p));
-
-out:
-	put_task_stack(p);
-	return ret;
+	stack_trace_save_tsk(p, &entry, 1, 0);
+	return entry;
 }
 
 long do_arch_prctl_common(struct task_struct *task, int option,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/3] Revert "proc/wchan: use printk format instead of lookup_symbol_name()"
  2021-09-24  6:20 ` [PATCH 1/3] Revert "proc/wchan: use printk format instead of lookup_symbol_name()" Kees Cook
@ 2021-09-24  8:17   ` Helge Deller
  0 siblings, 0 replies; 5+ messages in thread
From: Helge Deller @ 2021-09-24  8:17 UTC (permalink / raw)
  To: Kees Cook, Andrew Morton
  Cc: kernel test robot, Vito Caputo, Jann Horn, stable, Qi Zheng,
	Josh Poimboeuf, Tobin C. Harding, Tycho Andersen,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Mark Rutland, Jens Axboe, Peter Zijlstra, Andy Lutomirski,
	Lai Jiangshan, Stefan Metzmacher, Dave Hansen, Christian Brauner,
	Michal Hocko, Eric W. Biederman, Randy Dunlap, Ohhoon Kwon,
	YiFei Zhu, linux-kernel, linux-hardening, x86, linux-fsdevel

On 9/24/21 8:20 AM, Kees Cook wrote:
> This reverts commit 152c432b128cb043fc107e8f211195fe94b2159c.
>
> When a kernel address couldn't be symbolized for /proc/$pid/wchan, it
> would leak the raw value, a potential information exposure. This is a
> regression compared to the safer pre-v5.12 behavior.

Instead of reverting, another possibility might be to depend on
CONFIG_KALLSYMS before using the %ps format specifier and print "0" otherwise.
If it can't be symbolized it's most likely not a valid kernel address
and as such wouldn't leak anything....
But well,
Acked-by: Helge Deller <deller@gmx.de>

Helge

> Reported-by: kernel test robot <oliver.sang@intel.com>
> Link: https://lore.kernel.org/all/20210103142726.GC30643@xsang-OptiPlex-9020/
> Reported-by: Vito Caputo <vcaputo@pengaru.com>
> Link: https://lore.kernel.org/lkml/20210921193249.el476vlhg5k6lfcq@shells.gnugeneration.com/
> Reported-by: Jann Horn <jannh@google.com>
> Link: https://lore.kernel.org/lkml/CAG48ez2zC=+PuNgezH53HBPZ8CXU5H=vkWx7nJs60G8RXt3w0Q@mail.gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   fs/proc/base.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 533d5836eb9a..1f394095eb88 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -67,6 +67,7 @@
>   #include <linux/mm.h>
>   #include <linux/swap.h>
>   #include <linux/rcupdate.h>
> +#include <linux/kallsyms.h>
>   #include <linux/stacktrace.h>
>   #include <linux/resource.h>
>   #include <linux/module.h>
> @@ -386,17 +387,19 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
>   			  struct pid *pid, struct task_struct *task)
>   {
>   	unsigned long wchan;
> +	char symname[KSYM_NAME_LEN];
>
> -	if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
> -		wchan = get_wchan(task);
> -	else
> -		wchan = 0;
> +	if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
> +		goto print0;
>
> -	if (wchan)
> -		seq_printf(m, "%ps", (void *) wchan);
> -	else
> -		seq_putc(m, '0');
> +	wchan = get_wchan(task);
> +	if (wchan && !lookup_symbol_name(wchan, symname)) {
> +		seq_puts(m, symname);
> +		return 0;
> +	}
>
> +print0:
> +	seq_putc(m, '0');
>   	return 0;
>   }
>   #endif /* CONFIG_KALLSYMS */
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-09-24  8:19 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-24  6:20 [PATCH 0/3] wchan: Fix ORC support and leaky fallback Kees Cook
2021-09-24  6:20 ` [PATCH 1/3] Revert "proc/wchan: use printk format instead of lookup_symbol_name()" Kees Cook
2021-09-24  8:17   ` Helge Deller
2021-09-24  6:20 ` [PATCH 2/3] leaking_addresses: Always print a trailing newline Kees Cook
2021-09-24  6:20 ` [PATCH 3/3] x86: Fix get_wchan() to support the ORC unwinder Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).