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

[Sasha Levin <sasha.levin@oracle.com>]

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
[ 5228.737560] Read of size 8 by task killall/22177
[ 5228.738304] page:ffffea001274adc0 count:0 mapcount:0 mapping:          (null) index:0x0
[ 5228.739374] flags: 0x6fffff80000000()
[ 5228.739862] page dumped because: kasan: bad access detected
[ 5228.741764] CPU: 8 PID: 22177 Comm: killall Not tainted 4.3.0-rc3-next-20151002-sasha-00076-gde7fa56-dirty #2590
[ 5228.743337]  ffff882c80967828 000000007a901a83 ffff882c80967790 ffffffffacd2c8c8
[ 5228.744409]  ffff88049d2b7c50 ffff882c80967818 ffffffffab74befb ffff882c8bd00000
[ 5228.745436]  0000000000000002 0000000000000282 ffff882c8bd00cf8 0000000000000001
[ 5228.746446] Call Trace:
[ 5228.746881] dump_stack (lib/dump_stack.c:52)
[ 5228.747720] kasan_report_error (include/linux/kasan.h:28 mm/kasan/report.c:170 mm/kasan/report.c:237)
[ 5228.748670] __asan_report_load8_noabort (mm/kasan/report.c:279)
[ 5228.750563] get_wchan (arch/x86/kernel/process.c:561)
[ 5228.751378] do_task_stat (fs/proc/array.c:458)
[ 5228.755912] proc_tgid_stat (fs/proc/array.c:565)
[ 5228.756770] proc_single_show (./arch/x86/include/asm/atomic.h:118 include/linux/sched.h:2012 fs/proc/base.c:789)
[ 5228.759066] seq_read (fs/seq_file.c:238)
[ 5228.762360] __vfs_read (fs/read_write.c:432)
[ 5228.767957] vfs_read (fs/read_write.c:454)
[ 5228.769368] SyS_read (fs/read_write.c:570 fs/read_write.c:562)
[ 5228.778344] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:186)
[ 5228.779272] Memory state around the buggy address:
[ 5228.779971]  ffff88049d2b7b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 5228.780992]  ffff88049d2b7b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 5228.782021] >ffff88049d2b7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 5228.783066]                                                     ^
[ 5228.783936]  ffff88049d2b7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 5228.784994]  ffff88049d2b7d00: 00 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f3 f3 f3

        fp = READ_ONCE(*(unsigned long *)sp);
        do {
                if (fp < bottom || fp > top)
                        return 0;
                ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
                if (!in_sched_functions(ip))
                        return ip;
                fp = READ_ONCE(*(unsigned long *)fp); <=== Here
        } while (count++ < 16 && p->state != TASK_RUNNING);

Thanks,
Sasha