Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1233957 > unrolled thread

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

Started byDmitry Vyukov <dvyukov@google.com>
First post2015-09-28 11:10 +0200
Last post2015-09-30 10:10 +0200
Articles 20 on this page of 30 — 8 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 11:10 +0200
    Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-28 11:40 +0200
      Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 11:50 +0200
        Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-28 12:30 +0200
          Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 12:40 +0200
            Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-28 13:00 +0200
      Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 12:00 +0200
        Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-28 12:40 +0200
    Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-09-28 17:50 +0200
      Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-09-28 18:10 +0200
        Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Thomas Gleixner <tglx@linutronix.de> - 2015-09-28 18:40 +0200
          Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2015-09-29 20:20 +0200
            Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2015-09-29 20:40 +0200
              Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-09-29 20:50 +0200
            [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-09-30 09:20 +0200
              Re: [PATCH] fs/proc: Don't expose absolute kernel addresses via  wchan Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 09:40 +0200
                [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-09-30 16:00 +0200
                  Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via  wchan Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 22:40 +0200
                  Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan Kees Cook <keescook@chromium.org> - 2015-09-30 23:30 +0200
                    Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via  wchan Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 23:40 +0200
                    [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel  addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 10:00 +0200
                      Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel  addresses via wchan Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-10-01 11:00 +0200
                        Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel  addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 11:30 +0200
                          Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel  addresses via wchan Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-10-01 12:20 +0200
                            Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel  addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 12:40 +0200
                              Re: [PATCH v3] fs/proc, core/debug: Don't expose absolute kernel  addresses via wchan Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-10-01 12:50 +0200
                                [PATCH v5] fs/proc, core/debug: Don't expose absolute kernel  addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 13:00 +0200
                      [PATCH v4] fs/proc, core/debug: Don't expose absolute kernel  addresses via wchan Ingo Molnar <mingo@kernel.org> - 2015-10-01 11:40 +0200
                  [tip:core/debug] fs/proc, core/debug: Don'  t expose absolute kernel addresses via wchan tip-bot for Ingo Molnar <tipbot@zytor.com> - 2015-10-01 15:00 +0200
            Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 10:10 +0200

Page 1 of 2  [1] 2  Next page →

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

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-28 11:10 +0200
Subject[PATCH] arch/x86: fix out-of-bounds in get_wchan()
Message-ID<qdCBz-1h1-9@gated-at.bofh.it>
get_wchan() checks that fp is within stack bounds,
but then dereferences fp+8. This can crash kernel
or leak sensitive information. Also the function
operates on a potentially running stack, but does
not use READ_ONCE. As the result it can check that
one value is within stack bounds, but then deref
another value.

Fix the bounds check and use READ_ONCE for all
volatile data.

The bug was discovered with KASAN.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
---
FTR, here is the KASAN report:

[  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000
[  124.578633] Accessed by thread T10915:
[  124.581050]   #2 ffffffff810dd423 in __tsan_read8 ??:0
[  124.581893]   #3 ffffffff8107c093 in get_wchan ./arch/x86/kernel/process_64.c:444
[  124.582763]   #4 ffffffff81342108 in do_task_stat array.c:0
[  124.583634]   #5 ffffffff81342dcc in proc_tgid_stat ??:0
[  124.584548]   #6 ffffffff8133c984 in proc_single_show base.c:0
[  124.585461]   #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222
[  124.586313]   #8 ffffffff8129e503 in vfs_read ??:0
[  124.587137]   #9 ffffffff8129f800 in SyS_read ??:0
[  124.587827]   #10 ffffffff81929bf5 in sysenter_dispatch ./arch/x86/ia32/ia32entry.S:164
[  124.588738]
[  124.593434] Shadow bytes around the buggy address:
[  124.594270]   ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  124.595339]   ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  124.596453]   ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  124.597466]   ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  124.598501]   ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00
[  124.600873]   ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  124.601892]   ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
[  124.603037]   ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
[  124.604047]   ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd
[  124.605054]   ffff88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
[  124.605993] Shadow byte legend (one shadow byte represents 8 application bytes):
[  124.606958]   Addressable:   00
[  124.607483]   Partially addressable: 01 02 03 04 05 06 07
[  124.608219]   Heap redzone:  fa
[  124.608724]   Heap kmalloc redzone:  fb
[  124.609249]   Freed heap region: fd
[  124.609753]   Shadow gap:fe
[  124.610292] =========================================================================
---
 arch/x86/kernel/process_64.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 71d7849..a1fce34 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
 	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)
+	/* 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)
 		return 0;
-	fp = *(u64 *)(p->thread.sp);
+	fp = READ_ONCE(*(u64 *)fp);
 	do {
 		if (fp < (unsigned long)stack ||
-		    fp >= (unsigned long)stack+THREAD_SIZE)
+		    fp+8 >= (unsigned long)stack+THREAD_SIZE)
 			return 0;
-		ip = *(u64 *)(fp+8);
+		ip = READ_ONCE(*(u64 *)(fp+8));
 		if (!in_sched_functions(ip))
 			return ip;
-		fp = *(u64 *)fp;
+		fp = READ_ONCE(*(u64 *)fp);
 	} while (count++ < 16);
 	return 0;
 }
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1233974

FromBorislav Petkov <bp@alien8.de>
Date2015-09-28 11:40 +0200
Message-ID<qdD4C-1Mi-23@gated-at.bofh.it>
In reply to#1233957
On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
> get_wchan() checks that fp is within stack bounds,
> but then dereferences fp+8. This can crash kernel
> or leak sensitive information. Also the function
> operates on a potentially running stack, but does
> not use READ_ONCE. As the result it can check that
> one value is within stack bounds, but then deref
> another value.
> 
> Fix the bounds check and use READ_ONCE for all
> volatile data.
> 
> The bug was discovered with KASAN.
> 
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> ---
> FTR, here is the KASAN report:
> 
> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000
> [  124.578633] Accessed by thread T10915:
> [  124.581050]   #2 ffffffff810dd423 in __tsan_read8 ??:0
> [  124.581893]   #3 ffffffff8107c093 in get_wchan ./arch/x86/kernel/process_64.c:444
> [  124.582763]   #4 ffffffff81342108 in do_task_stat array.c:0
> [  124.583634]   #5 ffffffff81342dcc in proc_tgid_stat ??:0
> [  124.584548]   #6 ffffffff8133c984 in proc_single_show base.c:0
> [  124.585461]   #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222
> [  124.586313]   #8 ffffffff8129e503 in vfs_read ??:0
> [  124.587137]   #9 ffffffff8129f800 in SyS_read ??:0
> [  124.587827]   #10 ffffffff81929bf5 in sysenter_dispatch ./arch/x86/ia32/ia32entry.S:164
> [  124.588738]
> [  124.593434] Shadow bytes around the buggy address:
> [  124.594270]   ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [  124.595339]   ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [  124.596453]   ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [  124.597466]   ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [  124.598501]   ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [  124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00
> [  124.600873]   ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [  124.601892]   ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> [  124.603037]   ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> [  124.604047]   ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd
> [  124.605054]   ffff88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
> [  124.605993] Shadow byte legend (one shadow byte represents 8 application bytes):
> [  124.606958]   Addressable:   00
> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
> [  124.608219]   Heap redzone:  fa
> [  124.608724]   Heap kmalloc redzone:  fb
> [  124.609249]   Freed heap region: fd
> [  124.609753]   Shadow gap:fe
> [  124.610292] =========================================================================
> ---
>  arch/x86/kernel/process_64.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 71d7849..a1fce34 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>  	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)
> +	/* 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)
>  		return 0;
> -	fp = *(u64 *)(p->thread.sp);
> +	fp = READ_ONCE(*(u64 *)fp);

Why isn't this:

	fp = READ_ONCE(*(u64 *)p->thread.sp);

like the original code did?

Actually, the original code looks fishy to me too - it did access live
stack three times. And shouldn't we be accessing it only once?

I.e.,

	fp_st = READ_ONCE(p->thread.sp);
	if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
		return 0;
	fp = *(u64 *)fp_st;

Hmm?

Maybe I'm not completely clear on how the whole locking happens here
because we do

        if (!p || p == current || p->state == TASK_RUNNING)
                return 0;

earlier but apparently we can become TASK_RUNNING after the check...

Also, shouldn't this one have a CVE number assigned or so due to the
leakage potential?

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233987

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-28 11:50 +0200
Message-ID<qdDei-1XB-29@gated-at.bofh.it>
In reply to#1233974
On Mon, Sep 28, 2015 at 11:37 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
>> get_wchan() checks that fp is within stack bounds,
>> but then dereferences fp+8. This can crash kernel
>> or leak sensitive information. Also the function
>> operates on a potentially running stack, but does
>> not use READ_ONCE. As the result it can check that
>> one value is within stack bounds, but then deref
>> another value.
>>
>> Fix the bounds check and use READ_ONCE for all
>> volatile data.
>>
>> The bug was discovered with KASAN.
>>
>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>> ---
>> FTR, here is the KASAN report:
>>
>> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000
>> [  124.578633] Accessed by thread T10915:
>> [  124.581050]   #2 ffffffff810dd423 in __tsan_read8 ??:0
>> [  124.581893]   #3 ffffffff8107c093 in get_wchan ./arch/x86/kernel/process_64.c:444
>> [  124.582763]   #4 ffffffff81342108 in do_task_stat array.c:0
>> [  124.583634]   #5 ffffffff81342dcc in proc_tgid_stat ??:0
>> [  124.584548]   #6 ffffffff8133c984 in proc_single_show base.c:0
>> [  124.585461]   #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222
>> [  124.586313]   #8 ffffffff8129e503 in vfs_read ??:0
>> [  124.587137]   #9 ffffffff8129f800 in SyS_read ??:0
>> [  124.587827]   #10 ffffffff81929bf5 in sysenter_dispatch ./arch/x86/ia32/ia32entry.S:164
>> [  124.588738]
>> [  124.593434] Shadow bytes around the buggy address:
>> [  124.594270]   ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.595339]   ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.596453]   ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.597466]   ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.598501]   ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00
>> [  124.600873]   ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.601892]   ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> [  124.603037]   ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> [  124.604047]   ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd
>> [  124.605054]   ffff88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
>> [  124.605993] Shadow byte legend (one shadow byte represents 8 application bytes):
>> [  124.606958]   Addressable:   00
>> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
>> [  124.608219]   Heap redzone:  fa
>> [  124.608724]   Heap kmalloc redzone:  fb
>> [  124.609249]   Freed heap region: fd
>> [  124.609753]   Shadow gap:fe
>> [  124.610292] =========================================================================
>> ---
>>  arch/x86/kernel/process_64.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 71d7849..a1fce34 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>>       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)
>> +     /* 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)
>>               return 0;
>> -     fp = *(u64 *)(p->thread.sp);
>> +     fp = READ_ONCE(*(u64 *)fp);
>
> Why isn't this:
>
>         fp = READ_ONCE(*(u64 *)p->thread.sp);
>
> like the original code did?


Original code did:

     if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
           return 0;
     fp = *(u64 *)(p->thread.sp);

p->thread.sp can change concurrently.
So we could check that p->thread.sp is within stack bounds, but then
dereference another value (which is already outside of bounds).




> Actually, the original code looks fishy to me too - it did access live
> stack three times. And shouldn't we be accessing it only once?
>
> I.e.,
>
>         fp_st = READ_ONCE(p->thread.sp);
>         if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
>                 return 0;
>         fp = *(u64 *)fp_st;
>
> Hmm?

That's what my patch does.


> Maybe I'm not completely clear on how the whole locking happens here
> because we do
>
>         if (!p || p == current || p->state == TASK_RUNNING)
>                 return 0;
>
> earlier but apparently we can become TASK_RUNNING after the check...
>
> Also, shouldn't this one have a CVE number assigned or so due to the
> leakage potential?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234015

FromBorislav Petkov <bp@alien8.de>
Date2015-09-28 12:30 +0200
Message-ID<qdDR0-2VP-15@gated-at.bofh.it>
In reply to#1233987
On Mon, Sep 28, 2015 at 11:49:18AM +0200, Dmitry Vyukov wrote:
> Original code did:
> 
>      if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
>            return 0;
>      fp = *(u64 *)(p->thread.sp);
> 
> p->thread.sp can change concurrently.
> So we could check that p->thread.sp is within stack bounds, but then
> dereference another value (which is already outside of bounds).

Right, we do deref it. I realized that after hitting "Send" :\

Which begs another, probably also stupid, question:

What guarantees the task won't disappear after we've checked p?

I.e., after this:

        if (!p || p == current || p->state == TASK_RUNNING)
                return 0;

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234020

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-28 12:40 +0200
Message-ID<qdE0F-37c-11@gated-at.bofh.it>
In reply to#1234015
On Mon, Sep 28, 2015 at 12:23 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Sep 28, 2015 at 11:49:18AM +0200, Dmitry Vyukov wrote:
>> Original code did:
>>
>>      if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
>>            return 0;
>>      fp = *(u64 *)(p->thread.sp);
>>
>> p->thread.sp can change concurrently.
>> So we could check that p->thread.sp is within stack bounds, but then
>> dereference another value (which is already outside of bounds).
>
> Right, we do deref it. I realized that after hitting "Send" :\
>
> Which begs another, probably also stupid, question:
>
> What guarantees the task won't disappear after we've checked p?
>
> I.e., after this:
>
>         if (!p || p == current || p->state == TASK_RUNNING)
>                 return 0;


I have not checked, but I would expect that it is caller's
responsibility. There is generally no way to magically resurrect a
pointer to a freed object passed in.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234046

FromBorislav Petkov <bp@alien8.de>
Date2015-09-28 13:00 +0200
Message-ID<qdEk2-3tU-3@gated-at.bofh.it>
In reply to#1234020
On Mon, Sep 28, 2015 at 12:33:09PM +0200, Dmitry Vyukov wrote:
> I have not checked, but I would expect that it is caller's
> responsibility.

Looks like it: proc, for example, does get_pid_task()->get_task_struct().

> There is generally no way to magically resurrect a
> pointer to a freed object passed in.

Right.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233997

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-28 12:00 +0200
Message-ID<qdDnX-28T-1@gated-at.bofh.it>
In reply to#1233974
On Mon, Sep 28, 2015 at 11:37 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
>> get_wchan() checks that fp is within stack bounds,
>> but then dereferences fp+8. This can crash kernel
>> or leak sensitive information. Also the function
>> operates on a potentially running stack, but does
>> not use READ_ONCE. As the result it can check that
>> one value is within stack bounds, but then deref
>> another value.
>>
>> Fix the bounds check and use READ_ONCE for all
>> volatile data.
>>
>> The bug was discovered with KASAN.
>>
>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>> ---
>> FTR, here is the KASAN report:
>>
>> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address ffff88002e280000
>> [  124.578633] Accessed by thread T10915:
>> [  124.581050]   #2 ffffffff810dd423 in __tsan_read8 ??:0
>> [  124.581893]   #3 ffffffff8107c093 in get_wchan ./arch/x86/kernel/process_64.c:444
>> [  124.582763]   #4 ffffffff81342108 in do_task_stat array.c:0
>> [  124.583634]   #5 ffffffff81342dcc in proc_tgid_stat ??:0
>> [  124.584548]   #6 ffffffff8133c984 in proc_single_show base.c:0
>> [  124.585461]   #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222
>> [  124.586313]   #8 ffffffff8129e503 in vfs_read ??:0
>> [  124.587137]   #9 ffffffff8129f800 in SyS_read ??:0
>> [  124.587827]   #10 ffffffff81929bf5 in sysenter_dispatch ./arch/x86/ia32/ia32entry.S:164
>> [  124.588738]
>> [  124.593434] Shadow bytes around the buggy address:
>> [  124.594270]   ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.595339]   ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.596453]   ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.597466]   ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.598501]   ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00
>> [  124.600873]   ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> [  124.601892]   ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> [  124.603037]   ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> [  124.604047]   ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd
>> [  124.605054]   ffff88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
>> [  124.605993] Shadow byte legend (one shadow byte represents 8 application bytes):
>> [  124.606958]   Addressable:   00
>> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
>> [  124.608219]   Heap redzone:  fa
>> [  124.608724]   Heap kmalloc redzone:  fb
>> [  124.609249]   Freed heap region: fd
>> [  124.609753]   Shadow gap:fe
>> [  124.610292] =========================================================================
>> ---
>>  arch/x86/kernel/process_64.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 71d7849..a1fce34 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>>       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)
>> +     /* 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)
>>               return 0;
>> -     fp = *(u64 *)(p->thread.sp);
>> +     fp = READ_ONCE(*(u64 *)fp);
>
> Why isn't this:
>
>         fp = READ_ONCE(*(u64 *)p->thread.sp);
>
> like the original code did?
>
> Actually, the original code looks fishy to me too - it did access live
> stack three times. And shouldn't we be accessing it only once?
>
> I.e.,
>
>         fp_st = READ_ONCE(p->thread.sp);
>         if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
>                 return 0;
>         fp = *(u64 *)fp_st;
>
> Hmm?
>
> Maybe I'm not completely clear on how the whole locking happens here
> because we do
>
>         if (!p || p == current || p->state == TASK_RUNNING)
>                 return 0;
>
> earlier but apparently we can become TASK_RUNNING after the check...
>
> Also, shouldn't this one have a CVE number assigned or so due to the
> leakage potential?

Dunno. Should it? Most likely the data will be leaked iff it is
in_sched_functions. Never requested a CVE number before. If you
insist, I guess I can contact MITRE.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234021

FromBorislav Petkov <bp@alien8.de>
Date2015-09-28 12:40 +0200
Message-ID<qdE0F-37c-13@gated-at.bofh.it>
In reply to#1233997
On Mon, Sep 28, 2015 at 11:54:43AM +0200, Dmitry Vyukov wrote:
> Dunno. Should it? Most likely the data will be leaked iff it is
> in_sched_functions.

.. if not in_sched_functions...

> Never requested a CVE number before. If you insist, I guess I can
> contact MITRE.

Just wondering - generally the leaking to userspace things do get a CVE
number but let's see what the others think.

luto will know - he loves to open CVEs! :-P

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234260

FromAndrey Ryabinin <ryabinin.a.a@gmail.com>
Date2015-09-28 17:50 +0200
Message-ID<qdIQF-3Jd-3@gated-at.bofh.it>
In reply to#1233957
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"

>                 return 0;
> -       fp = *(u64 *)(p->thread.sp);
> +       fp = READ_ONCE(*(u64 *)fp);
>         do {
>                 if (fp < (unsigned long)stack ||
> -                   fp >= (unsigned long)stack+THREAD_SIZE)
> +                   fp+8 >= (unsigned long)stack+THREAD_SIZE)

The same as above;
        'fp+8 +sizeof(u64) >= ...'

>                         return 0;
> -               ip = *(u64 *)(fp+8);
> +               ip = READ_ONCE(*(u64 *)(fp+8));
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234270

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-28 18:10 +0200
Message-ID<qdJa2-4l8-17@gated-at.bofh.it>
In reply to#1234260
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.

>>                 return 0;
>> -       fp = *(u64 *)(p->thread.sp);
>> +       fp = READ_ONCE(*(u64 *)fp);
>>         do {
>>                 if (fp < (unsigned long)stack ||
>> -                   fp >= (unsigned long)stack+THREAD_SIZE)
>> +                   fp+8 >= (unsigned long)stack+THREAD_SIZE)
>
> The same as above;
>         'fp+8 +sizeof(u64) >= ...'
>
>>                         return 0;
>> -               ip = *(u64 *)(fp+8);
>> +               ip = READ_ONCE(*(u64 *)(fp+8));
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234284

FromThomas Gleixner <tglx@linutronix.de>
Date2015-09-28 18:40 +0200
Message-ID<qdJD3-4SO-9@gated-at.bofh.it>
In reply to#1234270
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;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235461

FromAndy Lutomirski <luto@amacapital.net>
Date2015-09-29 20:20 +0200
Message-ID<qe7Fp-5O1-29@gated-at.bofh.it>
In reply to#1234284
On Mon, Sep 28, 2015 at 9:32 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 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

There's TOP_OF_KERNEL_STACK_PADDING in here, too.  Arguably the
padding is still in bounds, though.  Also, I think you mean "start +",
not "start =".

> +        * ----------- 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);

I'm be vaguely amazed if this isn't an exploitable info leak even
without the out of bounds thing.  Can we really not find a way to do
this without walking the stack?

The bounds checking looks okay, though.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235471

FromAndy Lutomirski <luto@amacapital.net>
Date2015-09-29 20:40 +0200
Message-ID<qe7YJ-6an-1@gated-at.bofh.it>
In reply to#1235461
On Tue, Sep 29, 2015 at 11:15 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Sep 28, 2015 at 9:32 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> 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
>
> There's TOP_OF_KERNEL_STACK_PADDING in here, too.  Arguably the
> padding is still in bounds, though.  Also, I think you mean "start +",
> not "start =".
>
>> +        * ----------- 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);
>
> I'm be vaguely amazed if this isn't an exploitable info leak even
> without the out of bounds thing.  Can we really not find a way to do
> this without walking the stack?
>
> The bounds checking looks okay, though.

Also, I like Borislav's READ_ONCE suggestion.  Let's avoid TOCTOU due
to optimization.

Re: a CVE: if anyone wants a CVE, ask oss-security.  It's unclear to
me exactly how one might exploit this.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235485

FromBorislav Petkov <bp@alien8.de>
Date2015-09-29 20:50 +0200
Message-ID<qe88q-6lN-13@gated-at.bofh.it>
In reply to#1235471
On Tue, Sep 29, 2015 at 11:30:33AM -0700, Andy Lutomirski wrote:
> Also, I like Borislav's READ_ONCE suggestion.  Let's avoid TOCTOU due
> to optimization.

Dmitry's original patch did READ_ONCE already.

> Re: a CVE: if anyone wants a CVE, ask oss-security. It's unclear to me
> exactly how one might exploit this.

I was just asking - it possibly leaking sensitive info and all.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235823 — [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan

FromIngo Molnar <mingo@kernel.org>
Date2015-09-30 09:20 +0200
Subject[PATCH] fs/proc: Don't expose absolute kernel addresses via wchan
Message-ID<qejQd-6nP-3@gated-at.bofh.it>
In reply to#1235461
* Andy Lutomirski <luto@amacapital.net> wrote:

> > +        * ----------- 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);
> 
> I'm be vaguely amazed if this isn't an exploitable info leak even
> without the out of bounds thing.  Can we really not find a way to do
> this without walking the stack?

So wchan leaks absolute kernel addresses to unprivileged user-space, of kernel 
functions that sleep:

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

        wchan = get_wchan(task);

        if (lookup_symbol_name(wchan, symname) < 0) {
                if (!ptrace_may_access(task, PTRACE_MODE_READ))
                        return 0;
                seq_printf(m, "%lu", wchan);
        } else {
                seq_printf(m, "%s", symname);
        }

        return 0;
}

So for example it trivially leaks the KASLR offset to any local attacker:

  fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35)
  ffffffff8123b380

Most real-life uses of wchan are symbolic:

  ps -eo pid:10,tid:10,wchan:30,comm

and procps uses /proc/PID/wchan, not the absolute address in /proc/PID/stat:

  triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
  open("/proc/30833/wchan", O_RDONLY)     = 6

So shouldn't we try to set all numeric output to 0 and only allow symbolic output 
via /proc/PID/wchan?

These days there's very little legitimate reason user-space would be interested in 
the absolute address. The absolute address is mostly historic: from the days when 
we didn't have kallsyms and user-space procps had to do the decoding itself via 
the System.map.

( The absolute sleep address can generally still be profiled via perf, by tasks 
  with sufficient privileges. )

I.e. how about something like the patch below? (completely untested.)

Thanks,

	Ingo

======================>

 fs/proc/array.c | 2 +-
 fs/proc/base.c  | 7 +------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index f60f0121e331..99082730b2ac 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL);
 	seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL);
 	seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL);
-	seq_put_decimal_ull(m, ' ', wchan);
+	seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */
 	seq_put_decimal_ull(m, ' ', 0);
 	seq_put_decimal_ull(m, ' ', 0);
 	seq_put_decimal_ll(m, ' ', task->exit_signal);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b25eee4cead5..2fdbf303e3eb 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -430,13 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
 
 	wchan = get_wchan(task);
 
-	if (lookup_symbol_name(wchan, symname) < 0) {
-		if (!ptrace_may_access(task, PTRACE_MODE_READ))
-			return 0;
-		seq_printf(m, "%lu", wchan);
-	} else {
+	if (!lookup_symbol_name(wchan, symname))
 		seq_printf(m, "%s", symname);
-	}
 
 	return 0;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235839 — Re: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan

FromThomas Gleixner <tglx@linutronix.de>
Date2015-09-30 09:40 +0200
SubjectRe: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan
Message-ID<qek9B-6Kx-39@gated-at.bofh.it>
In reply to#1235823
On Wed, 30 Sep 2015, Ingo Molnar wrote:
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index f60f0121e331..99082730b2ac 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>  	seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL);
>  	seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL);
>  	seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL);
> -	seq_put_decimal_ull(m, ' ', wchan);
> +	seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */

That should get rid of all wchan usage in do_task_stat()

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236322 — [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan

FromIngo Molnar <mingo@kernel.org>
Date2015-09-30 16:00 +0200
Subject[PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan
Message-ID<qeq5l-6MN-41@gated-at.bofh.it>
In reply to#1235839
* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 30 Sep 2015, Ingo Molnar wrote:
> > diff --git a/fs/proc/array.c b/fs/proc/array.c
> > index f60f0121e331..99082730b2ac 100644
> > --- a/fs/proc/array.c
> > +++ b/fs/proc/array.c
> > @@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
> >  	seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL);
> >  	seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL);
> >  	seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL);
> > -	seq_put_decimal_ull(m, ' ', wchan);
> > +	seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */
> 
> That should get rid of all wchan usage in do_task_stat()

Indeed - updated patch attached.

Thanks,

	Ingo

================================>
From 985037cd05b379240dd381b29c2525758c665bb0 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Wed, 30 Sep 2015 09:15:37 +0200
Subject: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan

So wchan leaks absolute kernel addresses to unprivileged
user-space, of kernel  functions that sleep:

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

        wchan = get_wchan(task);

        if (lookup_symbol_name(wchan, symname) < 0) {
                if (!ptrace_may_access(task, PTRACE_MODE_READ))
                        return 0;
                seq_printf(m, "%lu", wchan);
        } else {
                seq_printf(m, "%s", symname);
        }

        return 0;
}

So for example it trivially leaks the KASLR offset to any local
attacker:

  fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35)
  ffffffff8123b380

Most real-life uses of wchan are symbolic:

  ps -eo pid:10,tid:10,wchan:30,comm

and procps uses /proc/PID/wchan, not the absolute address in
/proc/PID/stat:

  triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
  open("/proc/30833/wchan", O_RDONLY)     = 6

These days there's very little legitimate reason user-space
would be interested in  the absolute address. The absolute
address is mostly historic: from the days when  we didn't have
kallsyms and user-space procps had to do the decoding itself via
the System.map.

So this patch sets all numeric output to 0 and keeps the
symbolic output in /proc/PID/wchan.

( The absolute sleep address can generally still be profiled via
  perf, by tasks with sufficient privileges. )

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: kasan-dev <kasan-dev@googlegroups.com>
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/20150930071537.GA19048@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 fs/proc/array.c | 6 ++----
 fs/proc/base.c  | 7 +------
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index f60f0121e331..ad5ad1e376ad 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
 static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 			struct pid *pid, struct task_struct *task, int whole)
 {
-	unsigned long vsize, eip, esp, wchan = ~0UL;
+	unsigned long vsize, eip, esp;
 	int priority, nice;
 	int tty_pgrp = -1, tty_nr = 0;
 	sigset_t sigign, sigcatch;
@@ -454,8 +454,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 		unlock_task_sighand(task, &flags);
 	}
 
-	if (permitted && (!whole || num_threads < 2))
-		wchan = get_wchan(task);
 	if (!whole) {
 		min_flt = task->min_flt;
 		maj_flt = task->maj_flt;
@@ -507,7 +505,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL);
 	seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL);
 	seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL);
-	seq_put_decimal_ull(m, ' ', wchan);
+	seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */
 	seq_put_decimal_ull(m, ' ', 0);
 	seq_put_decimal_ull(m, ' ', 0);
 	seq_put_decimal_ll(m, ' ', task->exit_signal);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index b25eee4cead5..2fdbf303e3eb 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -430,13 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
 
 	wchan = get_wchan(task);
 
-	if (lookup_symbol_name(wchan, symname) < 0) {
-		if (!ptrace_may_access(task, PTRACE_MODE_READ))
-			return 0;
-		seq_printf(m, "%lu", wchan);
-	} else {
+	if (!lookup_symbol_name(wchan, symname))
 		seq_printf(m, "%s", symname);
-	}
 
 	return 0;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236738 — Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan

FromThomas Gleixner <tglx@linutronix.de>
Date2015-09-30 22:40 +0200
SubjectRe: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan
Message-ID<qewkp-7rR-3@gated-at.bofh.it>
In reply to#1236322
On Wed, 30 Sep 2015, Ingo Molnar wrote:
> These days there's very little legitimate reason user-space
> would be interested in  the absolute address. The absolute
> address is mostly historic: from the days when  we didn't have
> kallsyms and user-space procps had to do the decoding itself via
> the System.map.
> 
> So this patch sets all numeric output to 0 and keeps the
> symbolic output in /proc/PID/wchan.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236772 — Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan

FromKees Cook <keescook@chromium.org>
Date2015-09-30 23:30 +0200
SubjectRe: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan
Message-ID<qex6O-aB-5@gated-at.bofh.it>
In reply to#1236322
On Wed, Sep 30, 2015 at 6:59 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> On Wed, 30 Sep 2015, Ingo Molnar wrote:
>> > diff --git a/fs/proc/array.c b/fs/proc/array.c
>> > index f60f0121e331..99082730b2ac 100644
>> > --- a/fs/proc/array.c
>> > +++ b/fs/proc/array.c
>> > @@ -507,7 +507,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>> >     seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL);
>> >     seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL);
>> >     seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL);
>> > -   seq_put_decimal_ull(m, ' ', wchan);
>> > +   seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */
>>
>> That should get rid of all wchan usage in do_task_stat()
>
> Indeed - updated patch attached.
>
> Thanks,
>
>         Ingo
>
> ================================>
> From 985037cd05b379240dd381b29c2525758c665bb0 Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@kernel.org>
> Date: Wed, 30 Sep 2015 09:15:37 +0200
> Subject: [PATCH] fs/proc: Don't expose absolute kernel addresses via wchan
>
> So wchan leaks absolute kernel addresses to unprivileged
> user-space, of kernel  functions that sleep:
>
> 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];
>
>         wchan = get_wchan(task);
>
>         if (lookup_symbol_name(wchan, symname) < 0) {
>                 if (!ptrace_may_access(task, PTRACE_MODE_READ))
>                         return 0;
>                 seq_printf(m, "%lu", wchan);
>         } else {
>                 seq_printf(m, "%s", symname);
>         }
>
>         return 0;
> }
>
> So for example it trivially leaks the KASLR offset to any local
> attacker:
>
>   fomalhaut:~> printf "%016lx\n" $(cat /proc/$$/stat | cut -d' ' -f35)
>   ffffffff8123b380
>
> Most real-life uses of wchan are symbolic:
>
>   ps -eo pid:10,tid:10,wchan:30,comm
>
> and procps uses /proc/PID/wchan, not the absolute address in
> /proc/PID/stat:
>
>   triton:~/tip> strace -f ps -eo pid:10,tid:10,wchan:30,comm 2>&1 | grep wchan | tail -1
>   open("/proc/30833/wchan", O_RDONLY)     = 6
>
> These days there's very little legitimate reason user-space
> would be interested in  the absolute address. The absolute
> address is mostly historic: from the days when  we didn't have
> kallsyms and user-space procps had to do the decoding itself via
> the System.map.
>
> So this patch sets all numeric output to 0 and keeps the
> symbolic output in /proc/PID/wchan.
>
> ( The absolute sleep address can generally still be profiled via
>   perf, by tasks with sufficient privileges. )
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Kostya Serebryany <kcc@google.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: kasan-dev <kasan-dev@googlegroups.com>
> Cc: linux-kernel@vger.kernel.org
> Link: http://lkml.kernel.org/r/20150930071537.GA19048@gmail.com
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  fs/proc/array.c | 6 ++----
>  fs/proc/base.c  | 7 +------
>  2 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index f60f0121e331..ad5ad1e376ad 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -375,7 +375,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>  static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>                         struct pid *pid, struct task_struct *task, int whole)
>  {
> -       unsigned long vsize, eip, esp, wchan = ~0UL;
> +       unsigned long vsize, eip, esp;
>         int priority, nice;
>         int tty_pgrp = -1, tty_nr = 0;
>         sigset_t sigign, sigcatch;
> @@ -454,8 +454,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>                 unlock_task_sighand(task, &flags);
>         }
>
> -       if (permitted && (!whole || num_threads < 2))
> -               wchan = get_wchan(task);
>         if (!whole) {
>                 min_flt = task->min_flt;
>                 maj_flt = task->maj_flt;
> @@ -507,7 +505,7 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
>         seq_put_decimal_ull(m, ' ', task->blocked.sig[0] & 0x7fffffffUL);
>         seq_put_decimal_ull(m, ' ', sigign.sig[0] & 0x7fffffffUL);
>         seq_put_decimal_ull(m, ' ', sigcatch.sig[0] & 0x7fffffffUL);
> -       seq_put_decimal_ull(m, ' ', wchan);
> +       seq_puts(m, " 0"); /* Used to be numeric wchan - replaced by /proc/PID/wchan */

Probably should also update Documentation/filesystems/proc.txt with
something like:

--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -310,7 +310,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7)
   blocked       bitmap of blocked signals
   sigign        bitmap of ignored signals
   sigcatch      bitmap of caught signals
-  wchan         address where process went to sleep
+  0             (place holder, was wchan, see /proc/PID/wchan instead)
   0             (place holder)
   0             (place holder)
   exit_signal   signal to send to parent thread on exit

>         seq_put_decimal_ull(m, ' ', 0);
>         seq_put_decimal_ull(m, ' ', 0);
>         seq_put_decimal_ll(m, ' ', task->exit_signal);
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index b25eee4cead5..2fdbf303e3eb 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -430,13 +430,8 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
>
>         wchan = get_wchan(task);
>
> -       if (lookup_symbol_name(wchan, symname) < 0) {
> -               if (!ptrace_may_access(task, PTRACE_MODE_READ))
> -                       return 0;
> -               seq_printf(m, "%lu", wchan);
> -       } else {
> +       if (!lookup_symbol_name(wchan, symname))
>                 seq_printf(m, "%s", symname);
> -       }
>
>         return 0;
>  }

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1236786 — Re: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan

FromThomas Gleixner <tglx@linutronix.de>
Date2015-09-30 23:40 +0200
SubjectRe: [PATCH v2] fs/proc: Don't expose absolute kernel addresses via wchan
Message-ID<qexgu-oR-27@gated-at.bofh.it>
In reply to#1236772
On Wed, 30 Sep 2015, Kees Cook wrote:
> Probably should also update Documentation/filesystems/proc.txt with
> something like:
> 
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -310,7 +310,7 @@ Table 1-4: Contents of the stat files (as of 2.6.30-rc7)
>    blocked       bitmap of blocked signals
>    sigign        bitmap of ignored signals
>    sigcatch      bitmap of caught signals
> -  wchan         address where process went to sleep
> +  0             (place holder, was wchan, see /proc/PID/wchan instead)
>    0             (place holder)
>    0             (place holder)
>    exit_signal   signal to send to parent thread on exit

Good catch!

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web