Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1235461
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() |
| Date | 2015-09-29 20:20 +0200 |
| Message-ID | <qe7Fp-5O1-29@gated-at.bofh.it> (permalink) |
| References | <qdCBz-1h1-9@gated-at.bofh.it> <qdIQF-3Jd-3@gated-at.bofh.it> <qdJa2-4l8-17@gated-at.bofh.it> <qdJD3-4SO-9@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
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/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[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
csiph-web