[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 25 Jan 2017 18:44:44 +0400
From: Keun-O Park <kpark3469@...il.com>
To: Will Deacon <will.deacon@....com>
Cc: kernel-hardening@...ts.openwall.com, catalin.marinas@....com,
Kees Cook <keescook@...omium.org>, mark.rutland@....com, james.morse@....com,
panand@...hat.com, keun-o.park@...kmatter.ae, takahiro.akashi@...aro.org
Subject: Re: [PATCH] arm64: usercopy: Implement stack frame object validation
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>
>> ---
>> 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.
>
> 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.
Thanks.
BR
Sahara
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.
