[patch 0/2] x86/process: Sanitize bound checks in get_wchan() and unify 32/64 bit

[patch 0/2] x86/process: Sanitize bound checks in get_wchan() and unify 32/64 bit

[Thomas Gleixner]

As reported by several people the bound checks in get_wchan() on x86/64bit
are wrong.

The following series addresses that problem and as a consequence
unifies the needlessly different implementations of 32 and 64 bit.

Thanks,

	tglx

---
 process.c    |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 process_32.c |   28 ----------------------------
 process_64.c |   24 ------------------------
 3 files changed, 55 insertions(+), 52 deletions(-)

[patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

[Thomas Gleixner]

[-- Attachment #1: x86-process--Fix-bound-checks-in-64bit-get_wchan --]
[-- Type: text/plain, Size: 4438 bytes --]

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 - TOP_OF_KERNEL_STACK_PADDING - 2 * sizeof(unsigned long).

   The 2 * sizeof(unsigned long) is required because the stack pointer
   points at the frame pointer. The layout on the stack is: ... IP FP
   ... IP FP. So we need to make sure that both IP and FP are in the
   bounds.

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.

Use READ_ONCE() when accessing the stack. This does not prevent a
concurrent wakeup of the task and the stack changing, but at least it
avoids TOCTOU.

Also check task state at the end of the loop. Again that does not
prevent concurrent changes, but it avoids walking for nothing.

Add proper comments while at it.

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>
---
 arch/x86/kernel/process_64.c |   52 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 10 deletions(-)

Index: tip/arch/x86/kernel/process_64.c
===================================================================
--- tip.orig/arch/x86/kernel/process_64.c
+++ tip/arch/x86/kernel/process_64.c
@@ -499,27 +499,59 @@ void set_personality_ia32(bool x32)
 }
 EXPORT_SYMBOL_GPL(set_personality_ia32);
 
+/*
+ * Called from fs/proc with a reference on @p to find the function
+ * which called into schedule(). This needs to be done carefully
+ * because the task might wake up and we might look at a stack
+ * changing under us.
+ */
 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:
+	 *
+	 * ----------- topmax = start + THREAD_SIZE - sizeof(unsigned long)
+	 * PADDING
+	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
+	 * 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 - TOP_OF_KERNEL_STACK_PADDING;
+	top -= 2 * sizeof(unsigned long);
+	bottom = start + sizeof(struct thread_info);
+
+	sp = READ_ONCE(p->thread.sp);
+	if (sp < bottom || sp > top)
+		return 0;
+
+	fp = READ_ONCE(*(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 = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
 		if (!in_sched_functions(ip))
 			return ip;
-		fp = *(u64 *)fp;
-	} while (count++ < 16);
+		fp = READ_ONCE(*(unsigned long *)fp);
+	} while (count++ < 16 && p->state != TASK_RUNNING);
 	return 0;
 }
 

[patch 2/2] x86/process: Unify 32bit and 64bit implementations of get_wchan()

[Thomas Gleixner]

[-- Attachment #1: x86-process--Unify-32bit-and-64bit-implementations-of-get_wchan --]
[-- Type: text/plain, Size: 5343 bytes --]

The stack layout and the functionality is identical. Use the 64bit
version for all of x86.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/process.c    |   55 ++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/process_32.c |   28 ---------------------
 arch/x86/kernel/process_64.c |   56 -------------------------------------------
 3 files changed, 55 insertions(+), 84 deletions(-)

Index: tip/arch/x86/kernel/process.c
===================================================================
--- tip.orig/arch/x86/kernel/process.c
+++ tip/arch/x86/kernel/process.c
@@ -506,3 +506,58 @@ unsigned long arch_randomize_brk(struct
 	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
 }
 
+/*
+ * Called from fs/proc with a reference on @p to find the function
+ * which called into schedule(). This needs to be done carefully
+ * because the task might wake up and we might look at a stack
+ * changing under us.
+ */
+unsigned long get_wchan(struct task_struct *p)
+{
+	unsigned long start, bottom, top, sp, fp, ip;
+	int count = 0;
+
+	if (!p || p == current || p->state == TASK_RUNNING)
+		return 0;
+
+	start = (unsigned long)task_stack_page(p);
+	if (!start)
+		return 0;
+
+	/*
+	 * Layout of the stack page:
+	 *
+	 * ----------- topmax = start + THREAD_SIZE - sizeof(unsigned long)
+	 * PADDING
+	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
+	 * 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 - TOP_OF_KERNEL_STACK_PADDING;
+	top -= 2 * sizeof(unsigned long);
+	bottom = start + sizeof(struct thread_info);
+
+	sp = READ_ONCE(p->thread.sp);
+	if (sp < bottom || sp > top)
+		return 0;
+
+	fp = READ_ONCE(*(unsigned long *)sp);
+	do {
+		if (fp < bottom || fp > top)
+			return 0;
+		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
+		if (!in_sched_functions(ip))
+			return ip;
+		fp = READ_ONCE(*(unsigned long *)fp);
+	} while (count++ < 16 && p->state != TASK_RUNNING);
+	return 0;
+}
Index: tip/arch/x86/kernel/process_32.c
===================================================================
--- tip.orig/arch/x86/kernel/process_32.c
+++ tip/arch/x86/kernel/process_32.c
@@ -324,31 +324,3 @@ __switch_to(struct task_struct *prev_p,
 
 	return prev_p;
 }
-
-#define top_esp                (THREAD_SIZE - sizeof(unsigned long))
-#define top_ebp                (THREAD_SIZE - 2*sizeof(unsigned long))
-
-unsigned long get_wchan(struct task_struct *p)
-{
-	unsigned long bp, sp, ip;
-	unsigned long stack_page;
-	int count = 0;
-	if (!p || p == current || p->state == TASK_RUNNING)
-		return 0;
-	stack_page = (unsigned long)task_stack_page(p);
-	sp = p->thread.sp;
-	if (!stack_page || sp < stack_page || sp > top_esp+stack_page)
-		return 0;
-	/* include/asm-i386/system.h:switch_to() pushes bp last. */
-	bp = *(unsigned long *) sp;
-	do {
-		if (bp < stack_page || bp > top_ebp+stack_page)
-			return 0;
-		ip = *(unsigned long *) (bp+4);
-		if (!in_sched_functions(ip))
-			return ip;
-		bp = *(unsigned long *) bp;
-	} while (count++ < 16);
-	return 0;
-}
-
Index: tip/arch/x86/kernel/process_64.c
===================================================================
--- tip.orig/arch/x86/kernel/process_64.c
+++ tip/arch/x86/kernel/process_64.c
@@ -499,62 +499,6 @@ void set_personality_ia32(bool x32)
 }
 EXPORT_SYMBOL_GPL(set_personality_ia32);
 
-/*
- * Called from fs/proc with a reference on @p to find the function
- * which called into schedule(). This needs to be done carefully
- * because the task might wake up and we might look at a stack
- * changing under us.
- */
-unsigned long get_wchan(struct task_struct *p)
-{
-	unsigned long start, bottom, top, sp, fp, ip;
-	int count = 0;
-
-	if (!p || p == current || p->state == TASK_RUNNING)
-		return 0;
-
-	start = (unsigned long)task_stack_page(p);
-	if (!start)
-		return 0;
-
-	/*
-	 * Layout of the stack page:
-	 *
-	 * ----------- topmax = start + THREAD_SIZE - sizeof(unsigned long)
-	 * PADDING
-	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
-	 * 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 - TOP_OF_KERNEL_STACK_PADDING;
-	top -= 2 * sizeof(unsigned long);
-	bottom = start + sizeof(struct thread_info);
-
-	sp = READ_ONCE(p->thread.sp);
-	if (sp < bottom || sp > top)
-		return 0;
-
-	fp = READ_ONCE(*(unsigned long *)sp);
-	do {
-		if (fp < bottom || fp > top)
-			return 0;
-		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
-		if (!in_sched_functions(ip))
-			return ip;
-		fp = READ_ONCE(*(unsigned long *)fp);
-	} while (count++ < 16 && p->state != TASK_RUNNING);
-	return 0;
-}
-
 long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
 {
 	int ret = 0;

Re: [patch 0/2] x86/process: Sanitize bound checks in get_wchan() and unify 32/64 bit

[Borislav Petkov]

On Wed, Sep 30, 2015 at 08:38:21AM -0000, Thomas Gleixner wrote:
> As reported by several people the bound checks in get_wchan() on x86/64bit
> are wrong.
> 
> The following series addresses that problem and as a consequence
> unifies the needlessly different implementations of 32 and 64 bit.
> 
> Thanks,
> 
> 	tglx
> 
> ---
>  process.c    |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  process_32.c |   28 ----------------------------
>  process_64.c |   24 ------------------------
>  3 files changed, 55 insertions(+), 52 deletions(-)

Hohumm, looks good to me. Especially the documentation of the stack
layout and why we're doing all that dance.

Reviewed-by: Borislav Petkov <bp@suse.de>

Should we CC: stable?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [patch 0/2] x86/process: Sanitize bound checks in get_wchan() and unify 32/64 bit

[Dmitry Vyukov]

Looks good to me.
Additional kudos for READ_ONCE'es. Data races are bad.

Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

On Wed, Sep 30, 2015 at 11:06 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Sep 30, 2015 at 08:38:21AM -0000, Thomas Gleixner wrote:
>> As reported by several people the bound checks in get_wchan() on x86/64bit
>> are wrong.
>>
>> The following series addresses that problem and as a consequence
>> unifies the needlessly different implementations of 32 and 64 bit.
>>
>> Thanks,
>>
>>       tglx
>>
>> ---
>>  process.c    |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  process_32.c |   28 ----------------------------
>>  process_64.c |   24 ------------------------
>>  3 files changed, 55 insertions(+), 52 deletions(-)
>
> Hohumm, looks good to me. Especially the documentation of the stack
> layout and why we're doing all that dance.
>
> Reviewed-by: Borislav Petkov <bp@suse.de>
>
> Should we CC: stable?
>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.

[tip:x86/urgent] x86/process: Add proper bound checks in 64bit get_wchan()

[tip-bot for Thomas Gleixner]

Commit-ID:  eddd3826a1a0190e5235703d1e666affa4d13b96
Gitweb:     http://git.kernel.org/tip/eddd3826a1a0190e5235703d1e666affa4d13b96
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 30 Sep 2015 08:38:22 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 30 Sep 2015 21:51:34 +0200

x86/process: Add proper bound checks in 64bit get_wchan()

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 - TOP_OF_KERNEL_STACK_PADDING - 2 * sizeof(unsigned long).

   The 2 * sizeof(unsigned long) is required because the stack pointer
   points at the frame pointer. The layout on the stack is: ... IP FP
   ... IP FP. So we need to make sure that both IP and FP are in the
   bounds.

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.

Use READ_ONCE() when accessing the stack. This does not prevent a
concurrent wakeup of the task and the stack changing, but at least it
avoids TOCTOU.

Also check task state at the end of the loop. Again that does not
prevent concurrent changes, but it avoids walking for nothing.

Add proper comments while at it.

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>
Reviewed-by: Borislav Petkov <bp@alien8.de>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: kasan-dev <kasan-dev@googlegroups.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Wolfram Gloger <wmglo@dent.med.uni-muenchen.de>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/20150930083302.694788319@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/process_64.c | 52 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 3c1bbcf..f1fd088 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -499,27 +499,59 @@ void set_personality_ia32(bool x32)
 }
 EXPORT_SYMBOL_GPL(set_personality_ia32);
 
+/*
+ * Called from fs/proc with a reference on @p to find the function
+ * which called into schedule(). This needs to be done carefully
+ * because the task might wake up and we might look at a stack
+ * changing under us.
+ */
 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;
+
+	/*
+	 * Layout of the stack page:
+	 *
+	 * ----------- topmax = start + THREAD_SIZE - sizeof(unsigned long)
+	 * PADDING
+	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
+	 * 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 - TOP_OF_KERNEL_STACK_PADDING;
+	top -= 2 * sizeof(unsigned long);
+	bottom = start + sizeof(struct thread_info);
+
+	sp = READ_ONCE(p->thread.sp);
+	if (sp < bottom || sp > top)
 		return 0;
-	fp = *(u64 *)(p->thread.sp);
+
+	fp = READ_ONCE(*(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 = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
 		if (!in_sched_functions(ip))
 			return ip;
-		fp = *(u64 *)fp;
-	} while (count++ < 16);
+		fp = READ_ONCE(*(unsigned long *)fp);
+	} while (count++ < 16 && p->state != TASK_RUNNING);
 	return 0;
 }
 

[tip:x86/urgent] x86/process: Unify 32bit and 64bit implementations of get_wchan()

[tip-bot for Thomas Gleixner]

Commit-ID:  7ba78053aacb89998a052843e3c56983c31d57f0
Gitweb:     http://git.kernel.org/tip/7ba78053aacb89998a052843e3c56983c31d57f0
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 30 Sep 2015 08:38:23 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 30 Sep 2015 21:51:34 +0200

x86/process: Unify 32bit and 64bit implementations of get_wchan()

The stack layout and the functionality is identical. Use the 64bit
version for all of x86.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Borislav Petkov <bp@alien8.de>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: kasan-dev <kasan-dev@googlegroups.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: Wolfram Gloger <wmglo@dent.med.uni-muenchen.de>
Link: http://lkml.kernel.org/r/20150930083302.779694618@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/process.c    | 55 +++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/process_32.c | 28 ----------------------
 arch/x86/kernel/process_64.c | 56 --------------------------------------------
 3 files changed, 55 insertions(+), 84 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 6d0e62a..39e585a 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -506,3 +506,58 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
 	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
 }
 
+/*
+ * Called from fs/proc with a reference on @p to find the function
+ * which called into schedule(). This needs to be done carefully
+ * because the task might wake up and we might look at a stack
+ * changing under us.
+ */
+unsigned long get_wchan(struct task_struct *p)
+{
+	unsigned long start, bottom, top, sp, fp, ip;
+	int count = 0;
+
+	if (!p || p == current || p->state == TASK_RUNNING)
+		return 0;
+
+	start = (unsigned long)task_stack_page(p);
+	if (!start)
+		return 0;
+
+	/*
+	 * Layout of the stack page:
+	 *
+	 * ----------- topmax = start + THREAD_SIZE - sizeof(unsigned long)
+	 * PADDING
+	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
+	 * 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 - TOP_OF_KERNEL_STACK_PADDING;
+	top -= 2 * sizeof(unsigned long);
+	bottom = start + sizeof(struct thread_info);
+
+	sp = READ_ONCE(p->thread.sp);
+	if (sp < bottom || sp > top)
+		return 0;
+
+	fp = READ_ONCE(*(unsigned long *)sp);
+	do {
+		if (fp < bottom || fp > top)
+			return 0;
+		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
+		if (!in_sched_functions(ip))
+			return ip;
+		fp = READ_ONCE(*(unsigned long *)fp);
+	} while (count++ < 16 && p->state != TASK_RUNNING);
+	return 0;
+}
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index c13df2c..737527b 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -324,31 +324,3 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 
 	return prev_p;
 }
-
-#define top_esp                (THREAD_SIZE - sizeof(unsigned long))
-#define top_ebp                (THREAD_SIZE - 2*sizeof(unsigned long))
-
-unsigned long get_wchan(struct task_struct *p)
-{
-	unsigned long bp, sp, ip;
-	unsigned long stack_page;
-	int count = 0;
-	if (!p || p == current || p->state == TASK_RUNNING)
-		return 0;
-	stack_page = (unsigned long)task_stack_page(p);
-	sp = p->thread.sp;
-	if (!stack_page || sp < stack_page || sp > top_esp+stack_page)
-		return 0;
-	/* include/asm-i386/system.h:switch_to() pushes bp last. */
-	bp = *(unsigned long *) sp;
-	do {
-		if (bp < stack_page || bp > top_ebp+stack_page)
-			return 0;
-		ip = *(unsigned long *) (bp+4);
-		if (!in_sched_functions(ip))
-			return ip;
-		bp = *(unsigned long *) bp;
-	} while (count++ < 16);
-	return 0;
-}
-
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index f1fd088..b35921a 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -499,62 +499,6 @@ void set_personality_ia32(bool x32)
 }
 EXPORT_SYMBOL_GPL(set_personality_ia32);
 
-/*
- * Called from fs/proc with a reference on @p to find the function
- * which called into schedule(). This needs to be done carefully
- * because the task might wake up and we might look at a stack
- * changing under us.
- */
-unsigned long get_wchan(struct task_struct *p)
-{
-	unsigned long start, bottom, top, sp, fp, ip;
-	int count = 0;
-
-	if (!p || p == current || p->state == TASK_RUNNING)
-		return 0;
-
-	start = (unsigned long)task_stack_page(p);
-	if (!start)
-		return 0;
-
-	/*
-	 * Layout of the stack page:
-	 *
-	 * ----------- topmax = start + THREAD_SIZE - sizeof(unsigned long)
-	 * PADDING
-	 * ----------- top = topmax - TOP_OF_KERNEL_STACK_PADDING
-	 * 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 - TOP_OF_KERNEL_STACK_PADDING;
-	top -= 2 * sizeof(unsigned long);
-	bottom = start + sizeof(struct thread_info);
-
-	sp = READ_ONCE(p->thread.sp);
-	if (sp < bottom || sp > top)
-		return 0;
-
-	fp = READ_ONCE(*(unsigned long *)sp);
-	do {
-		if (fp < bottom || fp > top)
-			return 0;
-		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
-		if (!in_sched_functions(ip))
-			return ip;
-		fp = READ_ONCE(*(unsigned long *)fp);
-	} while (count++ < 16 && p->state != TASK_RUNNING);
-	return 0;
-}
-
 long do_arch_prctl(struct task_struct *task, int code, unsigned long addr)
 {
 	int ret = 0;

Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

[Sasha Levin]

On 09/30/2015 04:38 AM, Thomas Gleixner wrote:
> 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 - TOP_OF_KERNEL_STACK_PADDING - 2 * sizeof(unsigned long).
> 
>    The 2 * sizeof(unsigned long) is required because the stack pointer
>    points at the frame pointer. The layout on the stack is: ... IP FP
>    ... IP FP. So we need to make sure that both IP and FP are in the
>    bounds.
> 
> 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.
> 
> Use READ_ONCE() when accessing the stack. This does not prevent a
> concurrent wakeup of the task and the stack changing, but at least it
> avoids TOCTOU.
> 
> Also check task state at the end of the loop. Again that does not
> prevent concurrent changes, but it avoids walking for nothing.
> 
> Add proper comments while at it.
> 
> 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>

I'm seeing a different issue with this patch:

[ 5228.736320] BUG: KASAN: out-of-bounds in get_wchan+0xf9/0x1b0 at addr ffff88049d2b7c50
[ 5228.737560] Read of size 8 by task killall/22177
[ 5228.738304] page:ffffea001274adc0 count:0 mapcount:0 mapping:          (null) index:0x0
[ 5228.739374] flags: 0x6fffff80000000()
[ 5228.739862] page dumped because: kasan: bad access detected
[ 5228.741764] CPU: 8 PID: 22177 Comm: killall Not tainted 4.3.0-rc3-next-20151002-sasha-00076-gde7fa56-dirty #2590
[ 5228.743337]  ffff882c80967828 000000007a901a83 ffff882c80967790 ffffffffacd2c8c8
[ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
[ 5228.745436]  0000000000000002 0000000000000282 ffff882c8bd00cf8 0000000000000001
[ 5228.746446] Call Trace:
[ 5228.746881] dump_stack (lib/dump_stack.c:52)
[ 5228.747720] kasan_report_error (include/linux/kasan.h:28 mm/kasan/report.c:170 mm/kasan/report.c:237)
[ 5228.748670] __asan_report_load8_noabort (mm/kasan/report.c:279)
[ 5228.750563] get_wchan (arch/x86/kernel/process.c:561)
[ 5228.751378] do_task_stat (fs/proc/array.c:458)
[ 5228.755912] proc_tgid_stat (fs/proc/array.c:565)
[ 5228.756770] proc_single_show (./arch/x86/include/asm/atomic.h:118 include/linux/sched.h:2012 fs/proc/base.c:789)
[ 5228.759066] seq_read (fs/seq_file.c:238)
[ 5228.762360] __vfs_read (fs/read_write.c:432)
[ 5228.767957] vfs_read (fs/read_write.c:454)
[ 5228.769368] SyS_read (fs/read_write.c:570 fs/read_write.c:562)
[ 5228.778344] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
[ 5228.779272] Memory state around the buggy address:
[ 5228.779971]  ffff88049d2b7b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 5228.780992]  ffff88049d2b7b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 5228.782021] >ffff88049d2b7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 5228.783066]                                                     ^
[ 5228.783936]  ffff88049d2b7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 5228.784994]  ffff88049d2b7d00: 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f3 f3 f3

        fp = READ_ONCE(*(unsigned long *)sp);
        do {
                if (fp < bottom || fp > top)
                        return 0;
                ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
                if (!in_sched_functions(ip))
                        return ip;
                fp = READ_ONCE(*(unsigned long *)fp); <=== Here
        } while (count++ < 16 && p->state != TASK_RUNNING);

Thanks,
Sasha

Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

[Andy Lutomirski]

On Fri, Oct 2, 2015 at 6:15 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
> On 09/30/2015 04:38 AM, Thomas Gleixner wrote:
>> 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 - TOP_OF_KERNEL_STACK_PADDING - 2 * sizeof(unsigned long).
>>
>>    The 2 * sizeof(unsigned long) is required because the stack pointer
>>    points at the frame pointer. The layout on the stack is: ... IP FP
>>    ... IP FP. So we need to make sure that both IP and FP are in the
>>    bounds.
>>
>> 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.
>>
>> Use READ_ONCE() when accessing the stack. This does not prevent a
>> concurrent wakeup of the task and the stack changing, but at least it
>> avoids TOCTOU.
>>
>> Also check task state at the end of the loop. Again that does not
>> prevent concurrent changes, but it avoids walking for nothing.
>>
>> Add proper comments while at it.
>>
>> 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>
>
> I'm seeing a different issue with this patch:
>
> [ 5228.736320] BUG: KASAN: out-of-bounds in get_wchan+0xf9/0x1b0 at addr ffff88049d2b7c50

This could be a real bug, but it also could plausibly be kasan not
understanding that this code can legitimately read random addresses
within the stack page.  In particular, it can read up into the entry
asm stack, which kasan might consider off-limits.  (kasan may also
consider the return address itself off limits.)

--Andy

Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

[Thomas Gleixner]

On Fri, 2 Oct 2015, Sasha Levin wrote:
> I'm seeing a different issue with this patch:
> 
> [ 5228.736320] BUG: KASAN: out-of-bounds in get_wchan+0xf9/0x1b0 at addr ffff88049d2b7c50
> [ 5228.737560] Read of size 8 by task killall/22177
> [ 5228.738304] page:ffffea001274adc0 count:0 mapcount:0 mapping:          (null) index:0x0
> [ 5228.739374] flags: 0x6fffff80000000()
> [ 5228.739862] page dumped because: kasan: bad access detected
> [ 5228.741764] CPU: 8 PID: 22177 Comm: killall Not tainted 4.3.0-rc3-next-20151002-sasha-00076-gde7fa56-dirty #2590
> [ 5228.743337]  ffff882c80967828 000000007a901a83 ffff882c80967790 ffffffffacd2c8c8
> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
> [ 5228.745436]  0000000000000002 0000000000000282 ffff882c8bd00cf8 0000000000000001
> [ 5228.746446] Call Trace:
> [ 5228.746881] dump_stack (lib/dump_stack.c:52)
> [ 5228.747720] kasan_report_error (include/linux/kasan.h:28 mm/kasan/report.c:170 mm/kasan/report.c:237)
> [ 5228.748670] __asan_report_load8_noabort (mm/kasan/report.c:279)
> [ 5228.750563] get_wchan (arch/x86/kernel/process.c:561)
> [ 5228.751378] do_task_stat (fs/proc/array.c:458)
> [ 5228.755912] proc_tgid_stat (fs/proc/array.c:565)
> [ 5228.756770] proc_single_show (./arch/x86/include/asm/atomic.h:118 include/linux/sched.h:2012 fs/proc/base.c:789)
> [ 5228.759066] seq_read (fs/seq_file.c:238)
> [ 5228.762360] __vfs_read (fs/read_write.c:432)
> [ 5228.767957] vfs_read (fs/read_write.c:454)
> [ 5228.769368] SyS_read (fs/read_write.c:570 fs/read_write.c:562)
> [ 5228.778344] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
> [ 5228.779272] Memory state around the buggy address:
> [ 5228.779971]  ffff88049d2b7b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 5228.780992]  ffff88049d2b7b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 5228.782021] >ffff88049d2b7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 5228.783066]                                                     ^
> [ 5228.783936]  ffff88049d2b7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 5228.784994]  ffff88049d2b7d00: 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f3 f3 f3
> 
>         fp = READ_ONCE(*(unsigned long *)sp);
>         do {
>                 if (fp < bottom || fp > top)
>                         return 0;
>                 ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>                 if (!in_sched_functions(ip))
>                         return ip;
>                 fp = READ_ONCE(*(unsigned long *)fp); <=== Here

Weird, we accessed 

     *(unsigned long *)(fp + sizeof(unsigned long))

a few lines above, i.e. ffff88049d2b7c58

But what's more weird is that the memory dump does not really look
like a stack at all.

ffff88049d2b7c50 is stored on the stack:

> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000

But if it is not inside the stack bounds, how do we end up
dereferencing it. Confused....

Too bad that we don't know where the actual stack is. Can you apply
the patch below so we can see where the stack area actually is?

Thanks,

	tglx

diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 5f1c6266eb30..1735cbc5e886 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -286,7 +286,7 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 		if ((i % STACKSLOTS_PER_LINE) == 0) {
 			if (i != 0)
 				pr_cont("\n");
-			printk("%s %016lx", log_lvl, *stack++);
+			printk("%s %p %016lx", log_lvl, stack, *stack++);
 		} else
 			pr_cont(" %016lx", *stack++);
 		touch_nmi_watchdog();
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index e07c94fbd0ac..473cf68984d0 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -129,6 +129,8 @@ static void print_address_description(struct kasan_access_info *info)
 			pr_err("Address belongs to variable %pS\n", addr);
 	}
 
+	pr_err("kasan: SP: %p\n", task_stack_page(current));
+
 	dump_stack();
 }
 

Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

[Andrey Ryabinin]

2015-10-03 13:54 GMT+03:00 Thomas Gleixner <tglx@linutronix.de>:
> On Fri, 2 Oct 2015, Sasha Levin wrote:
>> I'm seeing a different issue with this patch:
>>
>> [ 5228.736320] BUG: KASAN: out-of-bounds in get_wchan+0xf9/0x1b0 at addr ffff88049d2b7c50
>> [ 5228.737560] Read of size 8 by task killall/22177
>> [ 5228.738304] page:ffffea001274adc0 count:0 mapcount:0 mapping:          (null) index:0x0
>> [ 5228.739374] flags: 0x6fffff80000000()
>> [ 5228.739862] page dumped because: kasan: bad access detected
>> [ 5228.741764] CPU: 8 PID: 22177 Comm: killall Not tainted 4.3.0-rc3-next-20151002-sasha-00076-gde7fa56-dirty #2590
>> [ 5228.743337]  ffff882c80967828 000000007a901a83 ffff882c80967790 ffffffffacd2c8c8
>> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
>> [ 5228.745436]  0000000000000002 0000000000000282 ffff882c8bd00cf8 0000000000000001
>> [ 5228.746446] Call Trace:
>> [ 5228.746881] dump_stack (lib/dump_stack.c:52)
>> [ 5228.747720] kasan_report_error (include/linux/kasan.h:28 mm/kasan/report.c:170 mm/kasan/report.c:237)
>> [ 5228.748670] __asan_report_load8_noabort (mm/kasan/report.c:279)
>> [ 5228.750563] get_wchan (arch/x86/kernel/process.c:561)
>> [ 5228.751378] do_task_stat (fs/proc/array.c:458)
>> [ 5228.755912] proc_tgid_stat (fs/proc/array.c:565)
>> [ 5228.756770] proc_single_show (./arch/x86/include/asm/atomic.h:118 include/linux/sched.h:2012 fs/proc/base.c:789)
>> [ 5228.759066] seq_read (fs/seq_file.c:238)
>> [ 5228.762360] __vfs_read (fs/read_write.c:432)
>> [ 5228.767957] vfs_read (fs/read_write.c:454)
>> [ 5228.769368] SyS_read (fs/read_write.c:570 fs/read_write.c:562)
>> [ 5228.778344] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
>> [ 5228.779272] Memory state around the buggy address:
>> [ 5228.779971]  ffff88049d2b7b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [ 5228.780992]  ffff88049d2b7b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [ 5228.782021] >ffff88049d2b7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [ 5228.783066]                                                     ^
>> [ 5228.783936]  ffff88049d2b7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [ 5228.784994]  ffff88049d2b7d00: 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f3 f3 f3
>>
>>         fp = READ_ONCE(*(unsigned long *)sp);
>>         do {
>>                 if (fp < bottom || fp > top)
>>                         return 0;
>>                 ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>>                 if (!in_sched_functions(ip))
>>                         return ip;
>>                 fp = READ_ONCE(*(unsigned long *)fp); <=== Here
>
> Weird, we accessed
>
>      *(unsigned long *)(fp + sizeof(unsigned long))
>
> a few lines above, i.e. ffff88049d2b7c58
>
> But what's more weird is that the memory dump does not really look
> like a stack at all.
>
> ffff88049d2b7c50 is stored on the stack:
>
>> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
>
> But if it is not inside the stack bounds, how do we end up
> dereferencing it. Confused....
>

I'm sure that we in stack bounds here.
But we are not inside bounds of some stack variable
and KASAN doesn't like it.

KASAN changes stack frame of each function, e.g.
    void foo() {
            int a;
    }

transformed to:
    void foo() {
            char redzone1[32];
            int a;
            char redzone2[28];
            char redzone3[32];
    }

So any access to redzones will be reported.

I could make a patch which will tell KASAN to ignore get_wchan(),
but if there are any real bugs left in get_wchan() they will be ignored too.

Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

[Dmitry Vyukov]

On Sat, Oct 3, 2015 at 1:31 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote:
> 2015-10-03 13:54 GMT+03:00 Thomas Gleixner <tglx@linutronix.de>:
>> On Fri, 2 Oct 2015, Sasha Levin wrote:
>>> I'm seeing a different issue with this patch:
>>>
>>> [ 5228.736320] BUG: KASAN: out-of-bounds in get_wchan+0xf9/0x1b0 at addr ffff88049d2b7c50
>>> [ 5228.737560] Read of size 8 by task killall/22177
>>> [ 5228.738304] page:ffffea001274adc0 count:0 mapcount:0 mapping:          (null) index:0x0
>>> [ 5228.739374] flags: 0x6fffff80000000()
>>> [ 5228.739862] page dumped because: kasan: bad access detected
>>> [ 5228.741764] CPU: 8 PID: 22177 Comm: killall Not tainted 4.3.0-rc3-next-20151002-sasha-00076-gde7fa56-dirty #2590
>>> [ 5228.743337]  ffff882c80967828 000000007a901a83 ffff882c80967790 ffffffffacd2c8c8
>>> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
>>> [ 5228.745436]  0000000000000002 0000000000000282 ffff882c8bd00cf8 0000000000000001
>>> [ 5228.746446] Call Trace:
>>> [ 5228.746881] dump_stack (lib/dump_stack.c:52)
>>> [ 5228.747720] kasan_report_error (include/linux/kasan.h:28 mm/kasan/report.c:170 mm/kasan/report.c:237)
>>> [ 5228.748670] __asan_report_load8_noabort (mm/kasan/report.c:279)
>>> [ 5228.750563] get_wchan (arch/x86/kernel/process.c:561)
>>> [ 5228.751378] do_task_stat (fs/proc/array.c:458)
>>> [ 5228.755912] proc_tgid_stat (fs/proc/array.c:565)
>>> [ 5228.756770] proc_single_show (./arch/x86/include/asm/atomic.h:118 include/linux/sched.h:2012 fs/proc/base.c:789)
>>> [ 5228.759066] seq_read (fs/seq_file.c:238)
>>> [ 5228.762360] __vfs_read (fs/read_write.c:432)
>>> [ 5228.767957] vfs_read (fs/read_write.c:454)
>>> [ 5228.769368] SyS_read (fs/read_write.c:570 fs/read_write.c:562)
>>> [ 5228.778344] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
>>> [ 5228.779272] Memory state around the buggy address:
>>> [ 5228.779971]  ffff88049d2b7b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> [ 5228.780992]  ffff88049d2b7b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> [ 5228.782021] >ffff88049d2b7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> [ 5228.783066]                                                     ^
>>> [ 5228.783936]  ffff88049d2b7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> [ 5228.784994]  ffff88049d2b7d00: 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f3 f3 f3
>>>
>>>         fp = READ_ONCE(*(unsigned long *)sp);
>>>         do {
>>>                 if (fp < bottom || fp > top)
>>>                         return 0;
>>>                 ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>>>                 if (!in_sched_functions(ip))
>>>                         return ip;
>>>                 fp = READ_ONCE(*(unsigned long *)fp); <=== Here
>>
>> Weird, we accessed
>>
>>      *(unsigned long *)(fp + sizeof(unsigned long))
>>
>> a few lines above, i.e. ffff88049d2b7c58
>>
>> But what's more weird is that the memory dump does not really look
>> like a stack at all.
>>
>> ffff88049d2b7c50 is stored on the stack:
>>
>>> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
>>
>> But if it is not inside the stack bounds, how do we end up
>> dereferencing it. Confused....
>>
>
> I'm sure that we in stack bounds here.
> But we are not inside bounds of some stack variable
> and KASAN doesn't like it.

Agree.

> KASAN changes stack frame of each function, e.g.
>     void foo() {
>             int a;
>     }
>
> transformed to:
>     void foo() {
>             char redzone1[32];
>             int a;
>             char redzone2[28];
>             char redzone3[32];
>     }
>
> So any access to redzones will be reported.
>
> I could make a patch which will tell KASAN to ignore get_wchan(),
> but if there are any real bugs left in get_wchan() they will be ignored too.

Yes, we need KASAN to ignore it. False positives are not acceptable. I
wanted to mail a fix after the fix for the real bug. I am thinking
about kasan_disable_current/kasan_enable_current around the region
that does the stack accesses; the reports are rare enough, so it
should not have any significant performance effect. I don't mind if
you send the patch.

Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

[Dmitry Vyukov]

On Sun, Oct 4, 2015 at 7:10 PM, 'Kostya Serebryany' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
>
> On Sun, Oct 4, 2015 at 5:14 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> On Sat, Oct 3, 2015 at 1:31 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com>
>> wrote:
>> > 2015-10-03 13:54 GMT+03:00 Thomas Gleixner <tglx@linutronix.de>:
>> >> On Fri, 2 Oct 2015, Sasha Levin wrote:
>> >>> I'm seeing a different issue with this patch:
>> >>>
>> >>> [ 5228.736320] BUG: KASAN: out-of-bounds in get_wchan+0xf9/0x1b0 at
>> >>> addr ffff88049d2b7c50
>> >>> [ 5228.737560] Read of size 8 by task killall/22177
>> >>> [ 5228.738304] page:ffffea001274adc0 count:0 mapcount:0 mapping:
>> >>> (null) index:0x0
>> >>> [ 5228.739374] flags: 0x6fffff80000000()
>> >>> [ 5228.739862] page dumped because: kasan: bad access detected
>> >>> [ 5228.741764] CPU: 8 PID: 22177 Comm: killall Not tainted
>> >>> 4.3.0-rc3-next-20151002-sasha-00076-gde7fa56-dirty #2590
>> >>> [ 5228.743337]  ffff882c80967828 000000007a901a83 ffff882c80967790
>> >>> ffffffffacd2c8c8
>> >>> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb
>> >>> ffff882c8bd00000
>> >>> [ 5228.745436]  0000000000000002 0000000000000282 ffff882c8bd00cf8
>> >>> 0000000000000001
>> >>> [ 5228.746446] Call Trace:
>> >>> [ 5228.746881] dump_stack (lib/dump_stack.c:52)
>> >>> [ 5228.747720] kasan_report_error (include/linux/kasan.h:28
>> >>> mm/kasan/report.c:170 mm/kasan/report.c:237)
>> >>> [ 5228.748670] __asan_report_load8_noabort (mm/kasan/report.c:279)
>> >>> [ 5228.750563] get_wchan (arch/x86/kernel/process.c:561)
>> >>> [ 5228.751378] do_task_stat (fs/proc/array.c:458)
>> >>> [ 5228.755912] proc_tgid_stat (fs/proc/array.c:565)
>> >>> [ 5228.756770] proc_single_show (./arch/x86/include/asm/atomic.h:118
>> >>> include/linux/sched.h:2012 fs/proc/base.c:789)
>> >>> [ 5228.759066] seq_read (fs/seq_file.c:238)
>> >>> [ 5228.762360] __vfs_read (fs/read_write.c:432)
>> >>> [ 5228.767957] vfs_read (fs/read_write.c:454)
>> >>> [ 5228.769368] SyS_read (fs/read_write.c:570 fs/read_write.c:562)
>> >>> [ 5228.778344] entry_SYSCALL_64_fastpath
>> >>> (arch/x86/entry/entry_64.S:186)
>> >>> [ 5228.779272] Memory state around the buggy address:
>> >>> [ 5228.779971]  ffff88049d2b7b00: 00 00 00 00 00 00 00 00 00 00 00 00
>> >>> 00 00 00 00
>> >>> [ 5228.780992]  ffff88049d2b7b80: 00 00 00 00 00 00 00 00 00 00 00 00
>> >>> 00 00 00 00
>> >>> [ 5228.782021] >ffff88049d2b7c00: 00 00 00 00 00 00 00 00 00 00 00 00
>> >>> 00 00 00 00
>> >>> [ 5228.783066]                                                     ^
>> >>> [ 5228.783936]  ffff88049d2b7c80: 00 00 00 00 00 00 00 00 00 00 00 00
>> >>> 00 00 00 00
>> >>> [ 5228.784994]  ffff88049d2b7d00: 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4
>> >>> f4 f3 f3 f3
>> >>>
>> >>>         fp = READ_ONCE(*(unsigned long *)sp);
>> >>>         do {
>> >>>                 if (fp < bottom || fp > top)
>> >>>                         return 0;
>> >>>                 ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned
>> >>> long)));
>> >>>                 if (!in_sched_functions(ip))
>> >>>                         return ip;
>> >>>                 fp = READ_ONCE(*(unsigned long *)fp); <=== Here
>> >>
>> >> Weird, we accessed
>> >>
>> >>      *(unsigned long *)(fp + sizeof(unsigned long))
>> >>
>> >> a few lines above, i.e. ffff88049d2b7c58
>> >>
>> >> But what's more weird is that the memory dump does not really look
>> >> like a stack at all.
>> >>
>> >> ffff88049d2b7c50 is stored on the stack:
>> >>
>> >>> [ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb
>> >>> ffff882c8bd00000
>> >>
>> >> But if it is not inside the stack bounds, how do we end up
>> >> dereferencing it. Confused....
>> >>
>> >
>> > I'm sure that we in stack bounds here.
>> > But we are not inside bounds of some stack variable
>> > and KASAN doesn't like it.
>>
>> Agree.
>>
>> > KASAN changes stack frame of each function, e.g.
>> >     void foo() {
>> >             int a;
>> >     }
>> >
>> > transformed to:
>> >     void foo() {
>> >             char redzone1[32];
>> >             int a;
>> >             char redzone2[28];
>> >             char redzone3[32];
>> >     }
>> >
>> > So any access to redzones will be reported.
>> >
>> > I could make a patch which will tell KASAN to ignore get_wchan(),
>> > but if there are any real bugs left in get_wchan() they will be ignored
>> > too.
>>
>> Yes, we need KASAN to ignore it. False positives are not acceptable. I
>> wanted to mail a fix after the fix for the real bug. I am thinking
>> about kasan_disable_current/kasan_enable_current around the region
>
> This is very risky w/o RAII -- we can forget to do kasan_enable_current on
> one of the paths.
> And we can't do RAII in C.

Not more risky than thousands of other resource allocations in kernel
and other C programs that power the world.

In [k]tsan we check that such counters are 0 on thread end. This is
practically enough mitigate potential risks.
As far as I remember we don't do it in kasan yet.


>> that does the stack accesses; the reports are rare enough, so it
>> should not have any significant performance effect. I don't mind if
>> you send the patch.