| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CALCETrWWsFB+aWKhadoaftNNjU=gRa15+7PXOR=3yrVzXuar6w@mail.gmail.com>
Date: Tue, 29 Sep 2015 11:15:19 -0700
From: Andy Lutomirski <luto@...capital.net>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Dmitry Vyukov <dvyukov@...gle.com>,
Andrey Ryabinin <ryabinin.a.a@...il.com>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
Andy Lutomirski <luto@...nel.org>,
Borislav Petkov <bp@...en8.de>,
Denys Vlasenko <dvlasenk@...hat.com>,
"x86@...nel.org" <x86@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
Kostya Serebryany <kcc@...gle.com>,
Alexander Potapenko <glider@...gle.com>,
Andrey Konovalov <andreyknvl@...gle.com>,
Sasha Levin <sasha.levin@...cle.com>,
Andi Kleen <ak@...ux.intel.com>,
kasan-dev <kasan-dev@...glegroups.com>
Subject: Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan()
On Mon, Sep 28, 2015 at 9:32 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Mon, 28 Sep 2015, Dmitry Vyukov wrote:
>
>> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin <ryabinin.a.a@...il.com> wrote:
>> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov <dvyukov@...gle.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@...utronix.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@...gle.com>
> Reported-by: Sasha Levin <sasha.levin@...cle.com>
> Based-on-patch-from: Wolfram Gloger <wmglo@...t.med.uni-muenchen.de>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: Andrey Ryabinin <ryabinin.a.a@...il.com>
> Cc: Andy Lutomirski <luto@...capital.net>
> Cc: Andrey Konovalov <andreyknvl@...gle.com>
> Cc: x86@...nel.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists