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