| 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: <CACT4Y+aA1A0jgDwzQhsxKgY36ELd58Bn+CP8S4kOhCdqr_GNjg@mail.gmail.com> Date: Mon, 28 Sep 2015 11:54:43 +0200 From: Dmitry Vyukov <dvyukov@...gle.com> To: Borislav Petkov <bp@...en8.de> Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>, luto@...nel.org, 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>, Andrey Ryabinin <ryabinin.a.a@...il.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 11:37 AM, Borislav Petkov <bp@...en8.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@...gle.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@...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