Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1238714

Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan()

Show key headers only | View raw

On Fri, Oct 2, 2015 at 6:15 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
> On 09/30/2015 04:38 AM, Thomas Gleixner wrote:
>> 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 - TOP_OF_KERNEL_STACK_PADDING - 2 * sizeof(unsigned long).
>>
>>    The 2 * sizeof(unsigned long) is required because the stack pointer
>>    points at the frame pointer. The layout on the stack is: ... IP FP
>>    ... IP FP. So we need to make sure that both IP and FP are in the
>>    bounds.
>>
>> 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.
>>
>> Use READ_ONCE() when accessing the stack. This does not prevent a
>> concurrent wakeup of the task and the stack changing, but at least it
>> avoids TOCTOU.
>>
>> Also check task state at the end of the loop. Again that does not
>> prevent concurrent changes, but it avoids walking for nothing.
>>
>> Add proper comments while at it.
>>
>> 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>
>
> I'm seeing a different issue with this patch:
>
> [ 5228.736320] BUG: KASAN: out-of-bounds in get_wchan+0xf9/0x1b0 at addr ffff88049d2b7c50

This could be a real bug, but it also could plausibly be kasan not
understanding that this code can legitimately read random addresses
within the stack page.  In particular, it can read up into the entry
asm stack, which kasan might consider off-limits.  (kasan may also
consider the return address itself off limits.)

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[patch 0/2] x86/process: Sanitize bound checks in get_wchan() and  unify 32/64 bit Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 10:40 +0200
  [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan() Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 10:40 +0200
    [tip:x86/urgent] x86/process:   Add proper bound checks in 64bit get_wchan() tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2015-09-30 22:00 +0200
    Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan() Sasha Levin <sasha.levin@oracle.com> - 2015-10-03 03:20 +0200
      Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan() Andy Lutomirski <luto@amacapital.net> - 2015-10-03 03:40 +0200
      Re: [patch 1/2] x86/process: Add proper bound checks in 64bit  get_wchan() Thomas Gleixner <tglx@linutronix.de> - 2015-10-03 13:00 +0200
        Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan() Andrey Ryabinin <ryabinin.a.a@gmail.com> - 2015-10-03 13:40 +0200
          Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-10-04 14:20 +0200
            Re: [patch 1/2] x86/process: Add proper bound checks in 64bit get_wchan() Dmitry Vyukov <dvyukov@gmail.com> - 2015-10-04 20:10 +0200
  [patch 2/2] x86/process: Unify 32bit and 64bit implementations of  get_wchan() Thomas Gleixner <tglx@linutronix.de> - 2015-09-30 10:40 +0200
    [tip:x86/urgent] x86/process:   Unify 32bit and 64bit implementations of get_wchan() tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2015-09-30 22:10 +0200
  Re: [patch 0/2] x86/process: Sanitize bound checks in get_wchan()  and unify 32/64 bit Borislav Petkov <bp@alien8.de> - 2015-09-30 11:10 +0200
    Re: [patch 0/2] x86/process: Sanitize bound checks in get_wchan() and  unify 32/64 bit Dmitry Vyukov <dvyukov@google.com> - 2015-09-30 11:20 +0200

csiph-web