| 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: <CALCETrW5-_JFO_9F9bt2F1s5N_dseffYxCTX_SHJL39UcMyrFQ@mail.gmail.com>
Date: Tue, 29 Sep 2015 11:30:33 -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 Tue, Sep 29, 2015 at 11:15 AM, Andy Lutomirski <luto@...capital.net> wrote:
> 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.
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
--
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