Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 25 Jan 2017 16:58:15 -0800
From: Kees Cook <keescook@...omium.org>
To: Keun-O Park <kpark3469@...il.com>
Cc: Will Deacon <will.deacon@....com>, 
	"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, 
	Catalin Marinas <catalin.marinas@....com>, Mark Rutland <mark.rutland@....com>, 
	James Morse <james.morse@....com>, Pratyush Anand <panand@...hat.com>, keun-o.park@...kmatter.ae, 
	AKASHI Takahiro <takahiro.akashi@...aro.org>
Subject: Re: [PATCH] arm64: usercopy: Implement stack frame object validation

On Wed, Jan 25, 2017 at 6:44 AM, Keun-O Park <kpark3469@...il.com> wrote:
> On Wed, Jan 25, 2017 at 5:54 PM, Will Deacon <will.deacon@....com> wrote:
>> [Adding Akashi, since he'a had fun and games with arm64 stack unwinding
>>  and I bet he can find a problem with this patch!]
>>
>> On Wed, Jan 25, 2017 at 05:46:23PM +0400, kpark3469@...il.com wrote:
>>> From: Sahara <keun-o.park@...kmatter.ae>
>>>
>>> This implements arch_within_stack_frames() for arm64 that should
>>> validate if a given object is contained by a kernel stack frame.
>>>
>>> Signed-off-by: Sahara <keun-o.park@...kmatter.ae>

Awesome! Thanks for working on this!

>>> ---
>>>  arch/arm64/Kconfig                   |  1 +
>>>  arch/arm64/include/asm/thread_info.h | 55 ++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 56 insertions(+)
>>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index 1117421..8bf70b4 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -97,6 +97,7 @@ config ARM64
>>>       select HAVE_SYSCALL_TRACEPOINTS
>>>       select HAVE_KPROBES
>>>       select HAVE_KRETPROBES if HAVE_KPROBES
>>> +     select HAVE_ARCH_WITHIN_STACK_FRAMES
>>>       select IOMMU_DMA if IOMMU_SUPPORT
>>>       select IRQ_DOMAIN
>>>       select IRQ_FORCED_THREADING
>>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
>>> index 46c3b93..f610c44 100644
>>> --- a/arch/arm64/include/asm/thread_info.h
>>> +++ b/arch/arm64/include/asm/thread_info.h
>>> @@ -68,7 +68,62 @@ struct thread_info {
>>>  #define thread_saved_fp(tsk) \
>>>       ((unsigned long)(tsk->thread.cpu_context.fp))
>>>
>>> +/*
>>> + * Walks up the stack frames to make sure that the specified object is
>>> + * entirely contained by a single stack frame.
>>> + *
>>> + * Returns:
>>> + *            1 if within a frame
>>> + *           -1 if placed across a frame boundary (or outside stack)
>>> + *            0 unable to determine (no frame pointers, etc)
>>> + */
>>> +static inline int arch_within_stack_frames(const void * const stack,
>>> +                                        const void * const stackend,
>>> +                                        const void *obj, unsigned long len)
>>> +{
>>> +#if defined(CONFIG_FRAME_POINTER)
>>> +     const void *oldframe;
>>> +     const void *callee_fp = NULL;
>>> +     const void *caller_fp = NULL;
>>> +
>>> +     oldframe = __builtin_frame_address(1);
>>> +     if (oldframe) {
>>> +             callee_fp = __builtin_frame_address(2);
>>> +             if (callee_fp)
>>> +                     caller_fp = __builtin_frame_address(3);
>>> +     }
>>> +     /*
>>> +      * low ----------------------------------------------> high
>>> +      * [callee_fp][lr][args][local vars][caller_fp'][lr']
>>> +      *                ^----------------^
>>> +      *               allow copies only within here
>>> +      */
>>
>> Which compilers have you tested this with? The GCC folks don't guarantee a
>> frame layout, and they have changed it in the past, so I suspect this is
>> pretty fragile. In particularly, if __builtin_frame_address just points
>> at the frame record, then I don't think you can make assumptions about the
>> placement of local variables and arguments with respect to that.

How often has it changed in the past? That seems like a strange thing
to change; either it's aligned and efficiently organized, or ... not?

>>
>> Will
>
> $ aarch64-linux-android-gcc --version
> aarch64-linux-android-gcc (GCC) 4.9 20150123 (prerelease)
>
> I tested this with aosp 7.1 android toolchain on Pixel.
> Maybe I need a suggestion to make this robust.

I wonder if some kind of BUILD_BUG_ON() magic could be used to
validate the relative positions of things on the stack? Or in the
worst case, a boot-time BUG() check...

Did you happen to test the lkdtm USERCOPY_STACK_FRAME_TO and
USERCOPY_STACK_FRAME_FROM tests to make sure they tripped correctly?

-Kees

-- 
Kees Cook
Nexus Security

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.