Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan()

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, 28 Sep 2015, Dmitry Vyukov wrote:

> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov <dvyukov@google.com>:
> >>         stack = (unsigned long)task_stack_page(p);
> >> -       if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
> >> +       /* The task can be already running at this point, so tread carefully. */
> >> +       fp = READ_ONCE(p->thread.sp);
> >> +       if (fp < stack || fp >= stack+THREAD_SIZE)
> >
> > Since we deference fp, it should be "|| fp + sizeof(u64) >= stack + THREAD_SIZE"
> 
> Good point.
> I guess it should be "|| fp + sizeof(u64) > stack + THREAD_SIZE",
> because == is OK if we add 8.
> 

This whole mess with +8 and -16 and whatever is just crap. And all of
it completely undocumented. Proper version below.

Thanks,

	tglx

8<-------------------------------

Subject: x86/process: Add proper bound checks in 64bit get_wchan()
From: Thomas Gleixner <tglx@linutronix.de>
Date: Mon, 28 Sep 2015 17:16:52 +0200

Dmitry Vyukov reported the following using trinity and the memory
error detector AddressSanitizer
(https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel).

[ 124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on
address ffff88002e280000
[ 124.576801] ffff88002e280000 is located 131938492886538 bytes to
the left of 28857600-byte region [ffffffff81282e0a, ffffffff82e0830a)
[ 124.578633] Accessed by thread T10915:
[ 124.579295] inlined in describe_heap_address
./arch/x86/mm/asan/report.c:164
[ 124.579295] #0 ffffffff810dd277 in asan_report_error
./arch/x86/mm/asan/report.c:278
[ 124.580137] #1 ffffffff810dc6a0 in asan_check_region
./arch/x86/mm/asan/asan.c:37
[ 124.581050] #2 ffffffff810dd423 in __tsan_read8 ??:0
[ 124.581893] #3 ffffffff8107c093 in get_wchan
./arch/x86/kernel/process_64.c:444

The address checks in the 64bit implementation of get_wchan() are
wrong in several ways:

 - The lower bound of the stack is not the start of the stack
   page. It's the start of the stack page plus sizeof (struct
   thread_info)

 - The upper bound must be top of stack minus 2 * sizeof(unsigned
   long). This is required because the stack pointer points at the
   frame pointer. The layout on the stack is: ... IP FP ... IP FP.

Fix the bound checks and get rid of the mix of numeric constants, u64
and unsigned long. Making all unsigned long allows us to use the same
function for 32bit as well.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Reported-by: Sasha Levin <sasha.levin@oracle.com>
Based-on-patch-from: Wolfram Gloger <wmglo@dent.med.uni-muenchen.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: x86@kernel.org
---
 arch/x86/kernel/process_64.c |   41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

Index: tip/arch/x86/kernel/process_64.c
===================================================================
--- tip.orig/arch/x86/kernel/process_64.c
+++ tip/arch/x86/kernel/process_64.c
@@ -501,24 +501,47 @@ EXPORT_SYMBOL_GPL(set_personality_ia32);
 
 unsigned long get_wchan(struct task_struct *p)
 {
-	unsigned long stack;
-	u64 fp, ip;
+	unsigned long start, bottom, top, sp, fp, ip;
 	int count = 0;
 
 	if (!p || p == current || p->state == TASK_RUNNING)
 		return 0;
-	stack = (unsigned long)task_stack_page(p);
-	if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
+
+	start = (unsigned long)task_stack_page(p);
+	if (!start)
 		return 0;
-	fp = *(u64 *)(p->thread.sp);
+
+	/*
+	 * Layout of the stack page:
+	 *
+	 * ----------- top = start = THREAD_SIZE - sizeof(unsigned long)
+	 * stack
+	 * ----------- bottom = start + sizeof(thread_info)
+	 * thread_info
+	 * ----------- 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 - 2 * sizeof(unsigned long);
+	bottom = start + sizeof(struct thread_info);
+
+	sp = p->thread.sp;
+	if (sp < bottom || sp > top)
+		return 0;
+
+	fp = *(unsigned long *)sp;
 	do {
-		if (fp < (unsigned long)stack ||
-		    fp >= (unsigned long)stack+THREAD_SIZE)
+		if (fp < bottom || fp > top)
 			return 0;
-		ip = *(u64 *)(fp+8);
+		ip = *(unsigned long *)(fp + sizeof(unsigned long));
 		if (!in_sched_functions(ip))
 			return ip;
-		fp = *(u64 *)fp;
+		fp = *(unsigned long *)fp;
 	} while (count++ < 16);
 	return 0;
 }