Re: [PATCH] arch/x86: fix out-of-bounds in get_wchan()

[Andy Lutomirski]

On Mon, Sep 28, 2015 at 9:32 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> On Mon, 28 Sep 2015, Dmitry Vyukov wrote:
>
>> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx> wrote:
>> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov <dvyukov@xxxxxxxxxx>:
>> >>         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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> Reported-by: Sasha Levin <sasha.levin@xxxxxxxxxx>
> Based-on-patch-from: Wolfram Gloger <wmglo@xxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> Cc: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> ---
>  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.

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