[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 15 Dec 2017 18:28:03 +0300
From: Alexander Popov <alex.popov@...ux.com>
To: "Dmitry V. Levin" <ldv@...linux.org>
Cc: kernel-hardening@...ts.openwall.com, KeesCook <keescook@...omium.org>,
Ingo Molnar <mingo@...nel.org>, Andy Lutomirski <luto@...nel.org>,
Tycho Andersen <tycho@...ho.ws>, Laura Abbott <labbott@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [PATCH RFC v6 2/6] gcc-plugins: Add STACKLEAK
plugin for tracking the kernel stack
On 12.12.2017 03:09, Dmitry V. Levin wrote:
> On Wed, Dec 06, 2017 at 02:33:43AM +0300, Alexander Popov wrote:
>> The STACKLEAK feature erases the kernel stack before returning from
>> syscalls. That reduces the information which kernel stack leak bugs can
>> reveal and blocks some uninitialized stack variable attacks. Moreover,
>> STACKLEAK provides runtime checks for kernel stack overflow detection.
>>
>> This commit introduces the STACKLEAK gcc plugin. It is needed for:
>> - tracking the lowest border of the kernel stack, which is important
>> for the code erasing the used part of the kernel stack at the end
>> of syscalls (comes in a separate commit);
>> - checking that alloca calls don't cause stack overflow.
>>
>> So this plugin instruments the kernel code inserting:
>> - the check_alloca() call before alloca and the track_stack() call
>> after it;
>> - the track_stack() call for the functions with a stack frame size
>> greater than or equal to CONFIG_STACKLEAK_TRACK_MIN_SIZE.
>>
>> The STACKLEAK feature is ported from grsecurity/PaX. More information at:
>> https://grsecurity.net/
>> https://pax.grsecurity.net/
>>
>> This code is modified from Brad Spengler/PaX Team's code in the last
>> public patch of grsecurity/PaX based on our understanding of the code.
>> Changes or omissions from the original code are ours and don't reflect
>> the original grsecurity/PaX code.
>>
>> Signed-off-by: Alexander Popov <alex.popov@...ux.com>
>> ---
>> arch/Kconfig | 15 ++
>> arch/x86/kernel/dumpstack.c | 15 ++
>> fs/exec.c | 25 ++
>> scripts/Makefile.gcc-plugins | 3 +
>> scripts/gcc-plugins/stackleak_plugin.c | 470 +++++++++++++++++++++++++++++++++
>> 5 files changed, 528 insertions(+)
>> create mode 100644 scripts/gcc-plugins/stackleak_plugin.c
>>
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index 721fdae..ba8e67b 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -528,6 +528,8 @@ config GCC_PLUGIN_STACKLEAK
>> bool "Erase the kernel stack before returning from syscalls"
>> depends on GCC_PLUGINS
>> depends on HAVE_ARCH_STACKLEAK
>> + imply VMAP_STACK
>> + imply SCHED_STACK_END_CHECK
>> help
>> This option makes the kernel erase the kernel stack before it
>> returns from a system call. That reduces the information which
>> @@ -544,6 +546,19 @@ config GCC_PLUGIN_STACKLEAK
>> * https://grsecurity.net/
>> * https://pax.grsecurity.net/
>>
>> +config STACKLEAK_TRACK_MIN_SIZE
>> + int "Minimum stack frame size of functions tracked by STACKLEAK"
>> + default 100
>> + range 0 4096
>> + depends on GCC_PLUGIN_STACKLEAK
>> + help
>> + The STACKLEAK gcc plugin instruments the kernel code for tracking
>> + the lowest border of the kernel stack (and for some other purposes).
>> + It inserts the track_stack() call for the functions with a stack
>> + frame size greater than or equal to this parameter. Be careful with
>> + this setting, don't break the poison search in erase_kstack.
>> + If unsure, leave the default value 100.
>> +
>
> I don't think the warning is scaring enough. As erase_kstack (both 64-bit
> and 32-bit versions) checks for 128 consequent bytes of STACKLEAK_POISON,
> it would be a bad idea to raise STACKLEAK_TRACK_MIN_SIZE to a value higher
> than 120. Perhaps there has to be a consistency check that
> STACKLEAK_TRACK_MIN_SIZE does not break assumptions made in erase_kstack.
Thanks, I agree. In v7 I'll introduce a macro for the poison search depth and
add a BUILD_BUG_ON(). It would be better than this scary remark in Kconfig.
>> config HAVE_CC_STACKPROTECTOR
>> bool
>> help
>> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
>> index f13b4c0..5a9b6cc 100644
>> --- a/arch/x86/kernel/dumpstack.c
>> +++ b/arch/x86/kernel/dumpstack.c
>> @@ -315,3 +315,18 @@ static int __init code_bytes_setup(char *s)
>> return 1;
>> }
>> __setup("code_bytes=", code_bytes_setup);
>> +
>> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>> +void __used check_alloca(unsigned long size)
>> +{
>> + unsigned long sp = (unsigned long)&sp;
>> + struct stack_info stack_info = {0};
>> + unsigned long visit_mask = 0;
>> + unsigned long stack_left;
>> +
>> + BUG_ON(get_stack_info(&sp, current, &stack_info, &visit_mask));
>> + stack_left = sp - (unsigned long)stack_info.begin;
>> + BUG_ON(stack_left < 256 || size >= stack_left - 256);
>> +}
>> +EXPORT_SYMBOL(check_alloca);
>> +#endif
>
> I think some rationale has to be given why 256 was chosen as the minimal
> size of stack space left after alloca.
Unfortunately, I can't provide such a rationale. This value just looks sane to
me. I'll introduce a macro instead of the hardcoded number.
Thanks again for the review!
Alexander
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.
