subject:"Re\: \[PATCH\] arch\/x86\: fix out\-of\-bounds in get

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

2015-09-30 Thread Thomas Gleixner

On Tue, 29 Sep 2015, Andy Lutomirski wrote:
> I'm be vaguely amazed if this isn't an exploitable info leak even
> without the out of bounds thing.

The info leak happens in fs/proc, where we happily print arbitrary
"IP" values, if we cant resolve a symbol.

> Can we really not find a way to do this without walking the stack?

We would have to add a 'store wait channel' mechanism to all functions
which are the primary entry points to scheduling. Not impossible, but
not pretty either.

If we want to prevent the stack changing under us, we'd need to take
p->pi_lock and do the task != RUNNING check and the walk under it. I
don't think we want to do that, unless there is a compelling reason to
do so.

Thanks,

tglx

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

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

2015-09-30 Thread Thomas Gleixner

On Tue, 29 Sep 2015, Andy Lutomirski wrote:
> I'm be vaguely amazed if this isn't an exploitable info leak even
> without the out of bounds thing.

The info leak happens in fs/proc, where we happily print arbitrary
"IP" values, if we cant resolve a symbol.

> Can we really not find a way to do this without walking the stack?

We would have to add a 'store wait channel' mechanism to all functions
which are the primary entry points to scheduling. Not impossible, but
not pretty either.

If we want to prevent the stack changing under us, we'd need to take
p->pi_lock and do the task != RUNNING check and the walk under it. I
don't think we want to do that, unless there is a compelling reason to
do so.

Thanks,

tglx

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

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

2015-09-29 Thread Borislav Petkov

On Tue, Sep 29, 2015 at 11:30:33AM -0700, Andy Lutomirski wrote:
> Also, I like Borislav's READ_ONCE suggestion.  Let's avoid TOCTOU due
> to optimization.

Dmitry's original patch did READ_ONCE already.

> Re: a CVE: if anyone wants a CVE, ask oss-security. It's unclear to me
> exactly how one might exploit this.

I was just asking - it possibly leaking sensitive info and all.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-29 Thread Andy Lutomirski

On Tue, Sep 29, 2015 at 11:15 AM, Andy Lutomirski  wrote:
> On Mon, Sep 28, 2015 at 9:32 AM, Thomas Gleixner  wrote:
>> On Mon, 28 Sep 2015, Dmitry Vyukov wrote:
>>
>>> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin  
>>> wrote:
>>> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
>>> >> 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 
>> 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 88002e28
>> [ 124.576801] 88002e28 is located 131938492886538 bytes to
>> the left of 28857600-byte region [81282e0a, 82e0830a)
>> [ 124.578633] Accessed by thread T10915:
>> [ 124.579295] inlined in describe_heap_address
>> ./arch/x86/mm/asan/report.c:164
>> [ 124.579295] #0 810dd277 in asan_report_error
>> ./arch/x86/mm/asan/report.c:278
>> [ 124.580137] #1 810dc6a0 in asan_check_region
>> ./arch/x86/mm/asan/asan.c:37
>> [ 124.581050] #2 810dd423 in __tsan_read8 ??:0
>> [ 124.581893] #3 8107c093 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 
>> Reported-by: Sasha Levin 
>> Based-on-patch-from: Wolfram Gloger 
>> Signed-off-by: Thomas Gleixner 
>> Cc: Andrey Ryabinin 
>> Cc: Andy Lutomirski 
>> Cc: Andrey Konovalov 
>> Cc: x...@kernel.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 {

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

2015-09-29 Thread Andy Lutomirski

On Mon, Sep 28, 2015 at 9:32 AM, Thomas Gleixner  wrote:
> On Mon, 28 Sep 2015, Dmitry Vyukov wrote:
>
>> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin  
>> wrote:
>> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
>> >> 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 
> 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 88002e28
> [ 124.576801] 88002e28 is located 131938492886538 bytes to
> the left of 28857600-byte region [81282e0a, 82e0830a)
> [ 124.578633] Accessed by thread T10915:
> [ 124.579295] inlined in describe_heap_address
> ./arch/x86/mm/asan/report.c:164
> [ 124.579295] #0 810dd277 in asan_report_error
> ./arch/x86/mm/asan/report.c:278
> [ 124.580137] #1 810dc6a0 in asan_check_region
> ./arch/x86/mm/asan/asan.c:37
> [ 124.581050] #2 810dd423 in __tsan_read8 ??:0
> [ 124.581893] #3 8107c093 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 
> Reported-by: Sasha Levin 
> Based-on-patch-from: Wolfram Gloger 
> Signed-off-by: Thomas Gleixner 
> Cc: Andrey Ryabinin 
> Cc: Andy Lutomirski 
> Cc: Andrey Konovalov 
> Cc: x...@kernel.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;
> -

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

2015-09-29 Thread Andy Lutomirski

On Mon, Sep 28, 2015 at 9:32 AM, Thomas Gleixner  wrote:
> On Mon, 28 Sep 2015, Dmitry Vyukov wrote:
>
>> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin  
>> wrote:
>> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
>> >> 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 
> 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 88002e28
> [ 124.576801] 88002e28 is located 131938492886538 bytes to
> the left of 28857600-byte region [81282e0a, 82e0830a)
> [ 124.578633] Accessed by thread T10915:
> [ 124.579295] inlined in describe_heap_address
> ./arch/x86/mm/asan/report.c:164
> [ 124.579295] #0 810dd277 in asan_report_error
> ./arch/x86/mm/asan/report.c:278
> [ 124.580137] #1 810dc6a0 in asan_check_region
> ./arch/x86/mm/asan/asan.c:37
> [ 124.581050] #2 810dd423 in __tsan_read8 ??:0
> [ 124.581893] #3 8107c093 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 
> Reported-by: Sasha Levin 
> Based-on-patch-from: Wolfram Gloger 
> Signed-off-by: Thomas Gleixner 
> Cc: Andrey Ryabinin 
> Cc: Andy Lutomirski 
> Cc: Andrey Konovalov 
> Cc: x...@kernel.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;
> +
> +

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

2015-09-29 Thread Andy Lutomirski

On Tue, Sep 29, 2015 at 11:15 AM, Andy Lutomirski  wrote:
> On Mon, Sep 28, 2015 at 9:32 AM, Thomas Gleixner  wrote:
>> On Mon, 28 Sep 2015, Dmitry Vyukov wrote:
>>
>>> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin  
>>> wrote:
>>> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
>>> >> 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 
>> 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 88002e28
>> [ 124.576801] 88002e28 is located 131938492886538 bytes to
>> the left of 28857600-byte region [81282e0a, 82e0830a)
>> [ 124.578633] Accessed by thread T10915:
>> [ 124.579295] inlined in describe_heap_address
>> ./arch/x86/mm/asan/report.c:164
>> [ 124.579295] #0 810dd277 in asan_report_error
>> ./arch/x86/mm/asan/report.c:278
>> [ 124.580137] #1 810dc6a0 in asan_check_region
>> ./arch/x86/mm/asan/asan.c:37
>> [ 124.581050] #2 810dd423 in __tsan_read8 ??:0
>> [ 124.581893] #3 8107c093 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 
>> Reported-by: Sasha Levin 
>> Based-on-patch-from: Wolfram Gloger 
>> Signed-off-by: Thomas Gleixner 
>> Cc: Andrey Ryabinin 
>> Cc: Andy Lutomirski 
>> Cc: Andrey Konovalov 
>> Cc: x...@kernel.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 =

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

2015-09-29 Thread Borislav Petkov

On Tue, Sep 29, 2015 at 11:30:33AM -0700, Andy Lutomirski wrote:
> Also, I like Borislav's READ_ONCE suggestion.  Let's avoid TOCTOU due
> to optimization.

Dmitry's original patch did READ_ONCE already.

> Re: a CVE: if anyone wants a CVE, ask oss-security. It's unclear to me
> exactly how one might exploit this.

I was just asking - it possibly leaking sensitive info and all.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Thomas Gleixner

On Mon, 28 Sep 2015, Dmitry Vyukov wrote:

> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin  
> wrote:
> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
> >> 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 
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 88002e28
[ 124.576801] 88002e28 is located 131938492886538 bytes to
the left of 28857600-byte region [81282e0a, 82e0830a)
[ 124.578633] Accessed by thread T10915:
[ 124.579295] inlined in describe_heap_address
./arch/x86/mm/asan/report.c:164
[ 124.579295] #0 810dd277 in asan_report_error
./arch/x86/mm/asan/report.c:278
[ 124.580137] #1 810dc6a0 in asan_check_region
./arch/x86/mm/asan/asan.c:37
[ 124.581050] #2 810dd423 in __tsan_read8 ??:0
[ 124.581893] #3 8107c093 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 
Reported-by: Sasha Levin 
Based-on-patch-from: Wolfram Gloger 
Signed-off-by: Thomas Gleixner 
Cc: Andrey Ryabinin 
Cc: Andy Lutomirski 
Cc: Andrey Konovalov 
Cc: x...@kernel.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
+* --- 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);
return 0;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to

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

2015-09-28 Thread Dmitry Vyukov

On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin  wrote:
> 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
>> 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.

>> return 0;
>> -   fp = *(u64 *)(p->thread.sp);
>> +   fp = READ_ONCE(*(u64 *)fp);
>> do {
>> if (fp < (unsigned long)stack ||
>> -   fp >= (unsigned long)stack+THREAD_SIZE)
>> +   fp+8 >= (unsigned long)stack+THREAD_SIZE)
>
> The same as above;
> 'fp+8 +sizeof(u64) >= ...'
>
>> return 0;
>> -   ip = *(u64 *)(fp+8);
>> +   ip = READ_ONCE(*(u64 *)(fp+8));
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Andrey Ryabinin

2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
> 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"

> return 0;
> -   fp = *(u64 *)(p->thread.sp);
> +   fp = READ_ONCE(*(u64 *)fp);
> do {
> if (fp < (unsigned long)stack ||
> -   fp >= (unsigned long)stack+THREAD_SIZE)
> +   fp+8 >= (unsigned long)stack+THREAD_SIZE)

The same as above;
'fp+8 +sizeof(u64) >= ...'

> return 0;
> -   ip = *(u64 *)(fp+8);
> +   ip = READ_ONCE(*(u64 *)(fp+8));
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Borislav Petkov

On Mon, Sep 28, 2015 at 12:33:09PM +0200, Dmitry Vyukov wrote:
> I have not checked, but I would expect that it is caller's
> responsibility.

Looks like it: proc, for example, does get_pid_task()->get_task_struct().

> There is generally no way to magically resurrect a
> pointer to a freed object passed in.

Right.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Dmitry Vyukov

On Mon, Sep 28, 2015 at 12:23 PM, Borislav Petkov  wrote:
> On Mon, Sep 28, 2015 at 11:49:18AM +0200, Dmitry Vyukov wrote:
>> Original code did:
>>
>>  if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
>>return 0;
>>  fp = *(u64 *)(p->thread.sp);
>>
>> p->thread.sp can change concurrently.
>> So we could check that p->thread.sp is within stack bounds, but then
>> dereference another value (which is already outside of bounds).
>
> Right, we do deref it. I realized that after hitting "Send" :\
>
> Which begs another, probably also stupid, question:
>
> What guarantees the task won't disappear after we've checked p?
>
> I.e., after this:
>
> if (!p || p == current || p->state == TASK_RUNNING)
> return 0;


I have not checked, but I would expect that it is caller's
responsibility. There is generally no way to magically resurrect a
pointer to a freed object passed in.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Borislav Petkov

On Mon, Sep 28, 2015 at 11:54:43AM +0200, Dmitry Vyukov wrote:
> Dunno. Should it? Most likely the data will be leaked iff it is
> in_sched_functions.

.. if not in_sched_functions...

> Never requested a CVE number before. If you insist, I guess I can
> contact MITRE.

Just wondering - generally the leaking to userspace things do get a CVE
number but let's see what the others think.

luto will know - he loves to open CVEs! :-P

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Borislav Petkov

On Mon, Sep 28, 2015 at 11:49:18AM +0200, Dmitry Vyukov wrote:
> Original code did:
> 
>  if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
>return 0;
>  fp = *(u64 *)(p->thread.sp);
> 
> p->thread.sp can change concurrently.
> So we could check that p->thread.sp is within stack bounds, but then
> dereference another value (which is already outside of bounds).

Right, we do deref it. I realized that after hitting "Send" :\

Which begs another, probably also stupid, question:

What guarantees the task won't disappear after we've checked p?

I.e., after this:

if (!p || p == current || p->state == TASK_RUNNING)
return 0;

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Dmitry Vyukov

On Mon, Sep 28, 2015 at 11:37 AM, Borislav Petkov  wrote:
> On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
>> get_wchan() checks that fp is within stack bounds,
>> but then dereferences fp+8. This can crash kernel
>> or leak sensitive information. Also the function
>> operates on a potentially running stack, but does
>> not use READ_ONCE. As the result it can check that
>> one value is within stack bounds, but then deref
>> another value.
>>
>> Fix the bounds check and use READ_ONCE for all
>> volatile data.
>>
>> The bug was discovered with KASAN.
>>
>> Signed-off-by: Dmitry Vyukov 
>> ---
>> FTR, here is the KASAN report:
>>
>> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address 
>> 88002e28
>> [  124.578633] Accessed by thread T10915:
>> [  124.581050]   #2 810dd423 in __tsan_read8 ??:0
>> [  124.581893]   #3 8107c093 in get_wchan 
>> ./arch/x86/kernel/process_64.c:444
>> [  124.582763]   #4 81342108 in do_task_stat array.c:0
>> [  124.583634]   #5 81342dcc in proc_tgid_stat ??:0
>> [  124.584548]   #6 8133c984 in proc_single_show base.c:0
>> [  124.585461]   #7 812d18cc in seq_read ./fs/seq_file.c:222
>> [  124.586313]   #8 8129e503 in vfs_read ??:0
>> [  124.587137]   #9 8129f800 in SyS_read ??:0
>> [  124.587827]   #10 81929bf5 in sysenter_dispatch 
>> ./arch/x86/ia32/ia32entry.S:164
>> [  124.588738]
>> [  124.593434] Shadow bytes around the buggy address:
>> [  124.594270]   88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.595339]   88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.596453]   88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.597466]   88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.598501]   88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.599629] =>88002e28:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 
>> 00 00
>> [  124.600873]   88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.601892]   88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa 
>> fa fa
>> [  124.603037]   88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 
>> fa fa
>> [  124.604047]   88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd 
>> fd fd
>> [  124.605054]   88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd 
>> fa fa
>> [  124.605993] Shadow byte legend (one shadow byte represents 8 application 
>> bytes):
>> [  124.606958]   Addressable:   00
>> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
>> [  124.608219]   Heap redzone:  fa
>> [  124.608724]   Heap kmalloc redzone:  fb
>> [  124.609249]   Freed heap region: fd
>> [  124.609753]   Shadow gap:fe
>> [  124.610292] 
>> =
>> ---
>>  arch/x86/kernel/process_64.c | 12 +++-
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 71d7849..a1fce34 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>>   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)
>> + /* 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)
>>   return 0;
>> - fp = *(u64 *)(p->thread.sp);
>> + fp = READ_ONCE(*(u64 *)fp);
>
> Why isn't this:
>
> fp = READ_ONCE(*(u64 *)p->thread.sp);
>
> like the original code did?
>
> Actually, the original code looks fishy to me too - it did access live
> stack three times. And shouldn't we be accessing it only once?
>
> I.e.,
>
> fp_st = READ_ONCE(p->thread.sp);
> if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
> return 0;
> fp = *(u64 *)fp_st;
>
> Hmm?
>
> Maybe I'm not completely clear on how the whole locking happens here
> because we do
>
> if (!p || p == current || p->state == TASK_RUNNING)
> return 0;
>
> earlier but apparently we can become TASK_RUNNING after the check...
>
> Also, shouldn't this one have a CVE number assigned or so due to the
> leakage potential?

Dunno. Should it? Most likely the data will be leaked iff it is
in_sched_functions. Never requested a CVE number before. If you
insist, I guess I can contact MITRE.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Dmitry Vyukov

On Mon, Sep 28, 2015 at 11:37 AM, Borislav Petkov  wrote:
> On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
>> get_wchan() checks that fp is within stack bounds,
>> but then dereferences fp+8. This can crash kernel
>> or leak sensitive information. Also the function
>> operates on a potentially running stack, but does
>> not use READ_ONCE. As the result it can check that
>> one value is within stack bounds, but then deref
>> another value.
>>
>> Fix the bounds check and use READ_ONCE for all
>> volatile data.
>>
>> The bug was discovered with KASAN.
>>
>> Signed-off-by: Dmitry Vyukov 
>> ---
>> FTR, here is the KASAN report:
>>
>> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address 
>> 88002e28
>> [  124.578633] Accessed by thread T10915:
>> [  124.581050]   #2 810dd423 in __tsan_read8 ??:0
>> [  124.581893]   #3 8107c093 in get_wchan 
>> ./arch/x86/kernel/process_64.c:444
>> [  124.582763]   #4 81342108 in do_task_stat array.c:0
>> [  124.583634]   #5 81342dcc in proc_tgid_stat ??:0
>> [  124.584548]   #6 8133c984 in proc_single_show base.c:0
>> [  124.585461]   #7 812d18cc in seq_read ./fs/seq_file.c:222
>> [  124.586313]   #8 8129e503 in vfs_read ??:0
>> [  124.587137]   #9 8129f800 in SyS_read ??:0
>> [  124.587827]   #10 81929bf5 in sysenter_dispatch 
>> ./arch/x86/ia32/ia32entry.S:164
>> [  124.588738]
>> [  124.593434] Shadow bytes around the buggy address:
>> [  124.594270]   88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.595339]   88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.596453]   88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.597466]   88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.598501]   88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.599629] =>88002e28:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 
>> 00 00
>> [  124.600873]   88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.601892]   88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa 
>> fa fa
>> [  124.603037]   88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 
>> fa fa
>> [  124.604047]   88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd 
>> fd fd
>> [  124.605054]   88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd 
>> fa fa
>> [  124.605993] Shadow byte legend (one shadow byte represents 8 application 
>> bytes):
>> [  124.606958]   Addressable:   00
>> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
>> [  124.608219]   Heap redzone:  fa
>> [  124.608724]   Heap kmalloc redzone:  fb
>> [  124.609249]   Freed heap region: fd
>> [  124.609753]   Shadow gap:fe
>> [  124.610292] 
>> =
>> ---
>>  arch/x86/kernel/process_64.c | 12 +++-
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 71d7849..a1fce34 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>>   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)
>> + /* 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)
>>   return 0;
>> - fp = *(u64 *)(p->thread.sp);
>> + fp = READ_ONCE(*(u64 *)fp);
>
> Why isn't this:
>
> fp = READ_ONCE(*(u64 *)p->thread.sp);
>
> like the original code did?


Original code did:

 if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
   return 0;
 fp = *(u64 *)(p->thread.sp);

p->thread.sp can change concurrently.
So we could check that p->thread.sp is within stack bounds, but then
dereference another value (which is already outside of bounds).




> Actually, the original code looks fishy to me too - it did access live
> stack three times. And shouldn't we be accessing it only once?
>
> I.e.,
>
> fp_st = READ_ONCE(p->thread.sp);
> if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
> return 0;
> fp = *(u64 *)fp_st;
>
> Hmm?

That's what my patch does.


> Maybe I'm not completely clear on how the whole locking happens here
> because we do
>
> if (!p || p == current || p->state == TASK_RUNNING)
> return 0;
>
> earlier but apparently we can become TASK_RUNNING after the check...
>
> Also, shouldn't this one have a CVE number assigned or so due to the
> leakage potential?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel"

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

2015-09-28 Thread Borislav Petkov

On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
> get_wchan() checks that fp is within stack bounds,
> but then dereferences fp+8. This can crash kernel
> or leak sensitive information. Also the function
> operates on a potentially running stack, but does
> not use READ_ONCE. As the result it can check that
> one value is within stack bounds, but then deref
> another value.
> 
> Fix the bounds check and use READ_ONCE for all
> volatile data.
> 
> The bug was discovered with KASAN.
> 
> Signed-off-by: Dmitry Vyukov 
> ---
> FTR, here is the KASAN report:
> 
> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address 
> 88002e28
> [  124.578633] Accessed by thread T10915:
> [  124.581050]   #2 810dd423 in __tsan_read8 ??:0
> [  124.581893]   #3 8107c093 in get_wchan 
> ./arch/x86/kernel/process_64.c:444
> [  124.582763]   #4 81342108 in do_task_stat array.c:0
> [  124.583634]   #5 81342dcc in proc_tgid_stat ??:0
> [  124.584548]   #6 8133c984 in proc_single_show base.c:0
> [  124.585461]   #7 812d18cc in seq_read ./fs/seq_file.c:222
> [  124.586313]   #8 8129e503 in vfs_read ??:0
> [  124.587137]   #9 8129f800 in SyS_read ??:0
> [  124.587827]   #10 81929bf5 in sysenter_dispatch 
> ./arch/x86/ia32/ia32entry.S:164
> [  124.588738]
> [  124.593434] Shadow bytes around the buggy address:
> [  124.594270]   88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.595339]   88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.596453]   88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.597466]   88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.598501]   88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.599629] =>88002e28:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 
> 00 00
> [  124.600873]   88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.601892]   88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa 
> fa fa
> [  124.603037]   88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 
> fa fa
> [  124.604047]   88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd 
> fd fd
> [  124.605054]   88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd 
> fa fa
> [  124.605993] Shadow byte legend (one shadow byte represents 8 application 
> bytes):
> [  124.606958]   Addressable:   00
> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
> [  124.608219]   Heap redzone:  fa
> [  124.608724]   Heap kmalloc redzone:  fb
> [  124.609249]   Freed heap region: fd
> [  124.609753]   Shadow gap:fe
> [  124.610292] 
> =
> ---
>  arch/x86/kernel/process_64.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 71d7849..a1fce34 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>   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)
> + /* 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)
>   return 0;
> - fp = *(u64 *)(p->thread.sp);
> + fp = READ_ONCE(*(u64 *)fp);

Why isn't this:

fp = READ_ONCE(*(u64 *)p->thread.sp);

like the original code did?

Actually, the original code looks fishy to me too - it did access live
stack three times. And shouldn't we be accessing it only once?

I.e.,

fp_st = READ_ONCE(p->thread.sp);
if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
return 0;
fp = *(u64 *)fp_st;

Hmm?

Maybe I'm not completely clear on how the whole locking happens here
because we do

if (!p || p == current || p->state == TASK_RUNNING)
return 0;

earlier but apparently we can become TASK_RUNNING after the check...

Also, shouldn't this one have a CVE number assigned or so due to the
leakage potential?

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Thomas Gleixner

On Mon, 28 Sep 2015, Dmitry Vyukov wrote:

> On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin  
> wrote:
> > 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
> >> 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 
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 88002e28
[ 124.576801] 88002e28 is located 131938492886538 bytes to
the left of 28857600-byte region [81282e0a, 82e0830a)
[ 124.578633] Accessed by thread T10915:
[ 124.579295] inlined in describe_heap_address
./arch/x86/mm/asan/report.c:164
[ 124.579295] #0 810dd277 in asan_report_error
./arch/x86/mm/asan/report.c:278
[ 124.580137] #1 810dc6a0 in asan_check_region
./arch/x86/mm/asan/asan.c:37
[ 124.581050] #2 810dd423 in __tsan_read8 ??:0
[ 124.581893] #3 8107c093 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 
Reported-by: Sasha Levin 
Based-on-patch-from: Wolfram Gloger 
Signed-off-by: Thomas Gleixner 
Cc: Andrey Ryabinin 
Cc: Andy Lutomirski 
Cc: Andrey Konovalov 
Cc: x...@kernel.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
+* --- 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 =

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

2015-09-28 Thread Dmitry Vyukov

On Mon, Sep 28, 2015 at 5:40 PM, Andrey Ryabinin  wrote:
> 2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
>> 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.

>> return 0;
>> -   fp = *(u64 *)(p->thread.sp);
>> +   fp = READ_ONCE(*(u64 *)fp);
>> do {
>> if (fp < (unsigned long)stack ||
>> -   fp >= (unsigned long)stack+THREAD_SIZE)
>> +   fp+8 >= (unsigned long)stack+THREAD_SIZE)
>
> The same as above;
> 'fp+8 +sizeof(u64) >= ...'
>
>> return 0;
>> -   ip = *(u64 *)(fp+8);
>> +   ip = READ_ONCE(*(u64 *)(fp+8));
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Andrey Ryabinin

2015-09-28 12:00 GMT+03:00 Dmitry Vyukov :
> 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"

> return 0;
> -   fp = *(u64 *)(p->thread.sp);
> +   fp = READ_ONCE(*(u64 *)fp);
> do {
> if (fp < (unsigned long)stack ||
> -   fp >= (unsigned long)stack+THREAD_SIZE)
> +   fp+8 >= (unsigned long)stack+THREAD_SIZE)

The same as above;
'fp+8 +sizeof(u64) >= ...'

> return 0;
> -   ip = *(u64 *)(fp+8);
> +   ip = READ_ONCE(*(u64 *)(fp+8));
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Borislav Petkov

On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
> get_wchan() checks that fp is within stack bounds,
> but then dereferences fp+8. This can crash kernel
> or leak sensitive information. Also the function
> operates on a potentially running stack, but does
> not use READ_ONCE. As the result it can check that
> one value is within stack bounds, but then deref
> another value.
> 
> Fix the bounds check and use READ_ONCE for all
> volatile data.
> 
> The bug was discovered with KASAN.
> 
> Signed-off-by: Dmitry Vyukov 
> ---
> FTR, here is the KASAN report:
> 
> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address 
> 88002e28
> [  124.578633] Accessed by thread T10915:
> [  124.581050]   #2 810dd423 in __tsan_read8 ??:0
> [  124.581893]   #3 8107c093 in get_wchan 
> ./arch/x86/kernel/process_64.c:444
> [  124.582763]   #4 81342108 in do_task_stat array.c:0
> [  124.583634]   #5 81342dcc in proc_tgid_stat ??:0
> [  124.584548]   #6 8133c984 in proc_single_show base.c:0
> [  124.585461]   #7 812d18cc in seq_read ./fs/seq_file.c:222
> [  124.586313]   #8 8129e503 in vfs_read ??:0
> [  124.587137]   #9 8129f800 in SyS_read ??:0
> [  124.587827]   #10 81929bf5 in sysenter_dispatch 
> ./arch/x86/ia32/ia32entry.S:164
> [  124.588738]
> [  124.593434] Shadow bytes around the buggy address:
> [  124.594270]   88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.595339]   88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.596453]   88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.597466]   88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.598501]   88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.599629] =>88002e28:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 
> 00 00
> [  124.600873]   88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00
> [  124.601892]   88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa 
> fa fa
> [  124.603037]   88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 
> fa fa
> [  124.604047]   88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd 
> fd fd
> [  124.605054]   88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd 
> fa fa
> [  124.605993] Shadow byte legend (one shadow byte represents 8 application 
> bytes):
> [  124.606958]   Addressable:   00
> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
> [  124.608219]   Heap redzone:  fa
> [  124.608724]   Heap kmalloc redzone:  fb
> [  124.609249]   Freed heap region: fd
> [  124.609753]   Shadow gap:fe
> [  124.610292] 
> =
> ---
>  arch/x86/kernel/process_64.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 71d7849..a1fce34 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>   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)
> + /* 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)
>   return 0;
> - fp = *(u64 *)(p->thread.sp);
> + fp = READ_ONCE(*(u64 *)fp);

Why isn't this:

fp = READ_ONCE(*(u64 *)p->thread.sp);

like the original code did?

Actually, the original code looks fishy to me too - it did access live
stack three times. And shouldn't we be accessing it only once?

I.e.,

fp_st = READ_ONCE(p->thread.sp);
if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
return 0;
fp = *(u64 *)fp_st;

Hmm?

Maybe I'm not completely clear on how the whole locking happens here
because we do

if (!p || p == current || p->state == TASK_RUNNING)
return 0;

earlier but apparently we can become TASK_RUNNING after the check...

Also, shouldn't this one have a CVE number assigned or so due to the
leakage potential?

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Dmitry Vyukov

On Mon, Sep 28, 2015 at 11:37 AM, Borislav Petkov  wrote:
> On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
>> get_wchan() checks that fp is within stack bounds,
>> but then dereferences fp+8. This can crash kernel
>> or leak sensitive information. Also the function
>> operates on a potentially running stack, but does
>> not use READ_ONCE. As the result it can check that
>> one value is within stack bounds, but then deref
>> another value.
>>
>> Fix the bounds check and use READ_ONCE for all
>> volatile data.
>>
>> The bug was discovered with KASAN.
>>
>> Signed-off-by: Dmitry Vyukov 
>> ---
>> FTR, here is the KASAN report:
>>
>> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address 
>> 88002e28
>> [  124.578633] Accessed by thread T10915:
>> [  124.581050]   #2 810dd423 in __tsan_read8 ??:0
>> [  124.581893]   #3 8107c093 in get_wchan 
>> ./arch/x86/kernel/process_64.c:444
>> [  124.582763]   #4 81342108 in do_task_stat array.c:0
>> [  124.583634]   #5 81342dcc in proc_tgid_stat ??:0
>> [  124.584548]   #6 8133c984 in proc_single_show base.c:0
>> [  124.585461]   #7 812d18cc in seq_read ./fs/seq_file.c:222
>> [  124.586313]   #8 8129e503 in vfs_read ??:0
>> [  124.587137]   #9 8129f800 in SyS_read ??:0
>> [  124.587827]   #10 81929bf5 in sysenter_dispatch 
>> ./arch/x86/ia32/ia32entry.S:164
>> [  124.588738]
>> [  124.593434] Shadow bytes around the buggy address:
>> [  124.594270]   88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.595339]   88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.596453]   88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.597466]   88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.598501]   88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.599629] =>88002e28:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 
>> 00 00
>> [  124.600873]   88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.601892]   88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa 
>> fa fa
>> [  124.603037]   88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 
>> fa fa
>> [  124.604047]   88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd 
>> fd fd
>> [  124.605054]   88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd 
>> fa fa
>> [  124.605993] Shadow byte legend (one shadow byte represents 8 application 
>> bytes):
>> [  124.606958]   Addressable:   00
>> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
>> [  124.608219]   Heap redzone:  fa
>> [  124.608724]   Heap kmalloc redzone:  fb
>> [  124.609249]   Freed heap region: fd
>> [  124.609753]   Shadow gap:fe
>> [  124.610292] 
>> =
>> ---
>>  arch/x86/kernel/process_64.c | 12 +++-
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 71d7849..a1fce34 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>>   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)
>> + /* 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)
>>   return 0;
>> - fp = *(u64 *)(p->thread.sp);
>> + fp = READ_ONCE(*(u64 *)fp);
>
> Why isn't this:
>
> fp = READ_ONCE(*(u64 *)p->thread.sp);
>
> like the original code did?
>
> Actually, the original code looks fishy to me too - it did access live
> stack three times. And shouldn't we be accessing it only once?
>
> I.e.,
>
> fp_st = READ_ONCE(p->thread.sp);
> if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
> return 0;
> fp = *(u64 *)fp_st;
>
> Hmm?
>
> Maybe I'm not completely clear on how the whole locking happens here
> because we do
>
> if (!p || p == current || p->state == TASK_RUNNING)
> return 0;
>
> earlier but apparently we can become TASK_RUNNING after the check...
>
> Also, shouldn't this one have a CVE number assigned or so due to the
> leakage potential?

Dunno. Should it? Most likely the data will be leaked iff it is
in_sched_functions. Never requested a CVE number before. If you
insist, I guess I can contact MITRE.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at

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

2015-09-28 Thread Borislav Petkov

On Mon, Sep 28, 2015 at 12:33:09PM +0200, Dmitry Vyukov wrote:
> I have not checked, but I would expect that it is caller's
> responsibility.

Looks like it: proc, for example, does get_pid_task()->get_task_struct().

> There is generally no way to magically resurrect a
> pointer to a freed object passed in.

Right.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Dmitry Vyukov

On Mon, Sep 28, 2015 at 12:23 PM, Borislav Petkov  wrote:
> On Mon, Sep 28, 2015 at 11:49:18AM +0200, Dmitry Vyukov wrote:
>> Original code did:
>>
>>  if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
>>return 0;
>>  fp = *(u64 *)(p->thread.sp);
>>
>> p->thread.sp can change concurrently.
>> So we could check that p->thread.sp is within stack bounds, but then
>> dereference another value (which is already outside of bounds).
>
> Right, we do deref it. I realized that after hitting "Send" :\
>
> Which begs another, probably also stupid, question:
>
> What guarantees the task won't disappear after we've checked p?
>
> I.e., after this:
>
> if (!p || p == current || p->state == TASK_RUNNING)
> return 0;


I have not checked, but I would expect that it is caller's
responsibility. There is generally no way to magically resurrect a
pointer to a freed object passed in.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Borislav Petkov

On Mon, Sep 28, 2015 at 11:54:43AM +0200, Dmitry Vyukov wrote:
> Dunno. Should it? Most likely the data will be leaked iff it is
> in_sched_functions.

.. if not in_sched_functions...

> Never requested a CVE number before. If you insist, I guess I can
> contact MITRE.

Just wondering - generally the leaking to userspace things do get a CVE
number but let's see what the others think.

luto will know - he loves to open CVEs! :-P

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-09-28 Thread Dmitry Vyukov

On Mon, Sep 28, 2015 at 11:37 AM, Borislav Petkov  wrote:
> On Mon, Sep 28, 2015 at 11:00:39AM +0200, Dmitry Vyukov wrote:
>> get_wchan() checks that fp is within stack bounds,
>> but then dereferences fp+8. This can crash kernel
>> or leak sensitive information. Also the function
>> operates on a potentially running stack, but does
>> not use READ_ONCE. As the result it can check that
>> one value is within stack bounds, but then deref
>> another value.
>>
>> Fix the bounds check and use READ_ONCE for all
>> volatile data.
>>
>> The bug was discovered with KASAN.
>>
>> Signed-off-by: Dmitry Vyukov 
>> ---
>> FTR, here is the KASAN report:
>>
>> [  124.575597] ERROR: AddressSanitizer: heap-buffer-overflow on address 
>> 88002e28
>> [  124.578633] Accessed by thread T10915:
>> [  124.581050]   #2 810dd423 in __tsan_read8 ??:0
>> [  124.581893]   #3 8107c093 in get_wchan 
>> ./arch/x86/kernel/process_64.c:444
>> [  124.582763]   #4 81342108 in do_task_stat array.c:0
>> [  124.583634]   #5 81342dcc in proc_tgid_stat ??:0
>> [  124.584548]   #6 8133c984 in proc_single_show base.c:0
>> [  124.585461]   #7 812d18cc in seq_read ./fs/seq_file.c:222
>> [  124.586313]   #8 8129e503 in vfs_read ??:0
>> [  124.587137]   #9 8129f800 in SyS_read ??:0
>> [  124.587827]   #10 81929bf5 in sysenter_dispatch 
>> ./arch/x86/ia32/ia32entry.S:164
>> [  124.588738]
>> [  124.593434] Shadow bytes around the buggy address:
>> [  124.594270]   88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.595339]   88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.596453]   88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.597466]   88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.598501]   88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.599629] =>88002e28:[fa]fa fa fa fa fa fa fa fa fa 00 00 00 00 
>> 00 00
>> [  124.600873]   88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> 00 00
>> [  124.601892]   88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa 
>> fa fa
>> [  124.603037]   88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 
>> fa fa
>> [  124.604047]   88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd fd fd 
>> fd fd
>> [  124.605054]   88002e280280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd 
>> fa fa
>> [  124.605993] Shadow byte legend (one shadow byte represents 8 application 
>> bytes):
>> [  124.606958]   Addressable:   00
>> [  124.607483]   Partially addressable: 01 02 03 04 05 06 07
>> [  124.608219]   Heap redzone:  fa
>> [  124.608724]   Heap kmalloc redzone:  fb
>> [  124.609249]   Freed heap region: fd
>> [  124.609753]   Shadow gap:fe
>> [  124.610292] 
>> =
>> ---
>>  arch/x86/kernel/process_64.c | 12 +++-
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 71d7849..a1fce34 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -506,17 +506,19 @@ unsigned long get_wchan(struct task_struct *p)
>>   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)
>> + /* 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)
>>   return 0;
>> - fp = *(u64 *)(p->thread.sp);
>> + fp = READ_ONCE(*(u64 *)fp);
>
> Why isn't this:
>
> fp = READ_ONCE(*(u64 *)p->thread.sp);
>
> like the original code did?


Original code did:

 if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
   return 0;
 fp = *(u64 *)(p->thread.sp);

p->thread.sp can change concurrently.
So we could check that p->thread.sp is within stack bounds, but then
dereference another value (which is already outside of bounds).




> Actually, the original code looks fishy to me too - it did access live
> stack three times. And shouldn't we be accessing it only once?
>
> I.e.,
>
> fp_st = READ_ONCE(p->thread.sp);
> if (fp_st < stack || fp_st >= stack + THREAD_SIZE)
> return 0;
> fp = *(u64 *)fp_st;
>
> Hmm?

That's what my patch does.


> Maybe I'm not completely clear on how the whole locking happens here
> because we do
>
> if (!p || p == current || p->state == TASK_RUNNING)
> return 0;
>
> earlier but apparently we can become TASK_RUNNING after the check...
>
> Also, shouldn't this one have a CVE number assigned or so due to the
> leakage potential?
--
To unsubscribe from this list: send

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

2015-09-28 Thread Borislav Petkov

On Mon, Sep 28, 2015 at 11:49:18AM +0200, Dmitry Vyukov wrote:
> Original code did:
> 
>  if (p->thread.sp < stack || p->thread.sp >= stack+THREAD_SIZE)
>return 0;
>  fp = *(u64 *)(p->thread.sp);
> 
> p->thread.sp can change concurrently.
> So we could check that p->thread.sp is within stack bounds, but then
> dereference another value (which is already outside of bounds).

Right, we do deref it. I realized that after hitting "Send" :\

Which begs another, probably also stupid, question:

What guarantees the task won't disappear after we've checked p?

I.e., after this:

if (!p || p == current || p->state == TASK_RUNNING)
return 0;

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

28 matches

Site Navigation

Mail list logo

Footer information