Messages in this thread | | | From | Andy Lutomirski <> | Date | Tue, 29 Sep 2015 11:30:33 -0700 | Subject | Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan() |
| |
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
| |