Hi,
We are working on a memory error detector AddressSanitizer for Linux
kernel (https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel),
it can detect use-after-free and buffer-overflow errors.
Here is a new report from the tool:
[ 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
[ 124.582763] #4 ffffffff81342108 in do_task_stat array.c:0
[ 124.583634] #5 ffffffff81342dcc in proc_tgid_stat ??:0
[ 124.584548] #6 ffffffff8133c984 in proc_single_show base.c:0
[ 124.585461] #7 ffffffff812d18cc in seq_read ./fs/seq_file.c:222
[ 124.586313] #8 ffffffff8129e503 in vfs_read ??:0
[ 124.587137] #9 ffffffff8129f800 in SyS_read ??:0
[ 124.587827] #10 ffffffff81929bf5 in sysenter_dispatch
./arch/x86/ia32/ia32entry.S:164
[ 124.588738]
[ 124.588964] Allocated by thread T0:
[ 124.592379] #4 ffffffff00000008 in __per_cpu_start ??:0
[ 124.593217]
[ 124.593434] Shadow bytes around the buggy address:
[ 124.594270] ffff88002e27fd80: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 124.595339] ffff88002e27fe00: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 124.596453] ffff88002e27fe80: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 124.597466] ffff88002e27ff00: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 124.598501] ffff88002e27ff80: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 124.599629] =>ffff88002e280000:[fa]fa fa fa fa fa fa fa fa fa 00 00
00 00 00 00
[ 124.600873] ffff88002e280080: 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00
[ 124.601892] ffff88002e280100: 00 fa fa fa fa fa fa fa fa fa fa fa
fa fa fa fa
[ 124.603037] ffff88002e280180: fa fa fa fa fa fa fa fa fa fa fa fa
fa fa fa fa
[ 124.604047] ffff88002e280200: fa fa fa fa fa fa fa fd fd fd fd fd
fd fd fd fd
[ 124.605054] ffff88002e280280: 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] =========================================================================
Indeed, get_wchan ensures that fp<stack+THREAD_SIZE, but then dereferences fp+8:
434 if (fp < (unsigned long)stack ||
435 fp >= (unsigned long)stack+THREAD_SIZE)
436 return 0;
437 ip = *(u64 *)(fp+8);
It must check that fp+8<stack+THREAD_SIZE.
As far as I see, the bug can lead to garbage return values or in the
worst case to crash.
> Indeed, get_wchan ensures that fp<stack+THREAD_SIZE, but then dereferences fp+8:
>
> 434 if (fp < (unsigned long)stack ||
> 435 fp >= (unsigned long)stack+THREAD_SIZE)
> 436 return 0;
> 437 ip = *(u64 *)(fp+8);
>
> It must check that fp+8<stack+THREAD_SIZE.
> As far as I see, the bug can lead to garbage return values or in the
> worst case to crash.
Thanks for the report.
The change looks good to me. Can you please submit a formal signed off patch
to [email protected] ?
-Andi
--
[email protected] -- Speaking for myself only
On Wed, Sep 11, 2013 at 2:06 AM, Andi Kleen <[email protected]> wrote:
>> Indeed, get_wchan ensures that fp<stack+THREAD_SIZE, but then dereferences fp+8:
>>
>> 434 if (fp < (unsigned long)stack ||
>> 435 fp >= (unsigned long)stack+THREAD_SIZE)
>> 436 return 0;
>> 437 ip = *(u64 *)(fp+8);
>>
>> It must check that fp+8<stack+THREAD_SIZE.
>> As far as I see, the bug can lead to garbage return values or in the
>> worst case to crash.
>
> Thanks for the report.
>
> The change looks good to me. Can you please submit a formal signed off patch
> to [email protected] ?
Hi Andi,
Wolfram has a patch for it.
Wolfram, please send your patch to [email protected].
On Wed, Sep 11, 2013 at 10:16:32PM +0400, Dmitry Vyukov wrote:
> On Wed, Sep 11, 2013 at 2:06 AM, Andi Kleen <[email protected]> wrote:
> >> Indeed, get_wchan ensures that fp<stack+THREAD_SIZE, but then dereferences fp+8:
> >>
> >> 434 if (fp < (unsigned long)stack ||
> >> 435 fp >= (unsigned long)stack+THREAD_SIZE)
> >> 436 return 0;
> >> 437 ip = *(u64 *)(fp+8);
> >>
> >> It must check that fp+8<stack+THREAD_SIZE.
> >> As far as I see, the bug can lead to garbage return values or in the
> >> worst case to crash.
> >
> > Thanks for the report.
> >
> > The change looks good to me. Can you please submit a formal signed off patch
> > to [email protected] ?
Can you CC this to lkml as well? x86@ isn't a public list, IIUC.
Please note that these bounds checks aren't correct to begin with. Since
a pointer is being dereferenced, the end boundry must be reduced by
sizeof(unsigned long) as well.
It looks like process_32.c suffers the same problems, too.
-Kees
--
Kees Cook @outflux.net
Kees Cook <[email protected]> writes:
> Please note that these bounds checks aren't correct to begin with. Since
> a pointer is being dereferenced, the end boundry must be reduced by
> sizeof(unsigned long) as well.
>
> It looks like process_32.c suffers the same problems, too.
I can't see the end boundary problem in process_32.c. The end checks
are properly reduced with the top_esp and top_ebp macros.
All I can see in process_32.c is that the check
if (bp < stack_page || bp > top_ebp+stack_page)
could be replaced by:
if (bp < stack_page-sizeof(unsigned long) || bp > top_ebp+stack_page)
but that is a relaxation and not an over/underrun fix.
Can you elaborate what problem you see in process_32.c?
Regards,
Wolfram.
On Sat, Sep 28, 2013 at 09:51:14PM +0200, Wolfram Gloger wrote:
> Kees Cook <[email protected]> writes:
>
> > Please note that these bounds checks aren't correct to begin with. Since
> > a pointer is being dereferenced, the end boundry must be reduced by
> > sizeof(unsigned long) as well.
> >
> > It looks like process_32.c suffers the same problems, too.
>
> I can't see the end boundary problem in process_32.c. The end checks
> are properly reduced with the top_esp and top_ebp macros.
>
> All I can see in process_32.c is that the check
>
> if (bp < stack_page || bp > top_ebp+stack_page)
>
> could be replaced by:
>
> if (bp < stack_page-sizeof(unsigned long) || bp > top_ebp+stack_page)
>
> but that is a relaxation and not an over/underrun fix.
>
> Can you elaborate what problem you see in process_32.c?
Ah, yes, sorry, this appears to only be a problem in process_64.c. I didn't look
closely enough. I see now that top_esp and top_ebp correctly reduce the size of
THREAD_SIZE. Thanks!
-Kees
--
Kees Cook @outflux.net