• Thomas Gleixner's avatar
    x86/process: Add proper bound checks in 64bit get_wchan() · eddd3826
    Thomas Gleixner authored
    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: default avatarDmitry Vyukov <dvyukov@google.com>
    Reported-by: default avatarSasha Levin <sasha.levin@oracle.com>
    Based-on-patch-from: Wolfram Gloger <wmglo@dent.med.uni-muenchen.de>
    Signed-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
    Reviewed-by: default avatarBorislav Petkov <bp@alien8.de>
    Reviewed-by: default avatarDmitry Vyukov <dvyukov@google.com>
    Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
    Cc: Andy Lutomirski <luto@amacapital.net>
    Cc: Andrey Konovalov <andreyknvl@google.com>
    Cc: Kostya Serebryany <kcc@google.com>
    Cc: Alexander Potapenko <glider@google.com>
    Cc: kasan-dev <kasan-dev@googlegroups.com>
    Cc: Denys Vlasenko <dvlasenk@redhat.com>
    Cc: Andi Kleen <ak@linux.intel.com>
    Cc: Wolfram Gloger <wmglo@dent.med.uni-muenchen.de>
    Cc: stable@vger.kernel.org
    Link: http://lkml.kernel.org/r/20150930083302.694788319@linutronix.deSigned-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
    eddd3826
process_64.c 18 KB