From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [PATCH RFC v8 0/6] Introduce the STACKLEAK feature and a test for it References: <1518804657-24905-1-git-send-email-alex.popov@linux.com> From: Laura Abbott Message-ID: <22f5c157-8b01-5e0a-69e1-44939cc73d7e@redhat.com> Date: Wed, 21 Feb 2018 17:43:06 -0800 MIME-Version: 1.0 In-Reply-To: <1518804657-24905-1-git-send-email-alex.popov@linux.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit To: Alexander Popov , kernel-hardening@lists.openwall.com, Kees Cook , PaX Team , Brad Spengler , Ingo Molnar , Andy Lutomirski , Tycho Andersen , Mark Rutland , Ard Biesheuvel , Borislav Petkov , Thomas Gleixner , "H . Peter Anvin" , Peter Zijlstra , "Dmitry V . Levin" , x86@kernel.org List-ID: On 02/16/2018 10:10 AM, Alexander Popov wrote: > This is the 8th version of the patch series introducing STACKLEAK to the > mainline kernel. I've made some minor improvements while waiting for the > next review by x86 maintainers. > > STACKLEAK is a security feature developed by Grsecurity/PaX (kudos to them), > which: > - reduces the information that can be revealed through kernel stack leak bugs; > - blocks some uninitialized stack variable attacks (e.g. CVE-2010-2963); > - introduces some runtime checks for kernel stack overflow detection. > > STACKLEAK instrumentation statistics > ==================================== > > These numbers were obtained for the 4th version of the patch series. > > Size of vmlinux (x86_64_defconfig): > file size: > - STACKLEAK disabled: 35014784 bytes > - STACKLEAK enabled: 35044952 bytes (+0.086%) > .text section size (calculated by size utility): > - STACKLEAK disabled: 10752983 > - STACKLEAK enabled: 11062221 (+2.876%) > > The readelf utility shows 45602 functions in vmlinux. The STACKLEAK gcc plugin > inserted 36 check_alloca() calls and 1265 track_stack() calls (42274 calls are > inserted during GIMPLE pass and 41009 calls are deleted during RTL pass). > So 2.853% of functions are instrumented. > > STACKLEAK performance impact > ============================ > > The STACKLEAK description in Kconfig includes: > "The tradeoff is the performance impact: on a single CPU system kernel > compilation sees a 1% slowdown, other systems and workloads may vary and you are > advised to test this feature on your expected workload before deploying it". > > Here are the results of a brief performance test on x86_64 (for the 2nd version > of the patch). The numbers are very different because the STACKLEAK performance > penalty depends on the workloads. > > Hardware: Intel Core i7-4770, 16 GB RAM > > Test #1: building the Linux kernel with Ubuntu config (time make -j9) > Result on 4.11-rc8: > real 32m14.893s > user 237m30.886s > sys 11m12.273s > Result on 4.11-rc8+stackleak: > real 32m26.881s (+0.62%) > user 238m38.926s (+0.48%) > sys 11m36.426s (+3.59%) > > Test #2: hackbench -s 4096 -l 2000 -g 15 -f 25 -P > Average result on 4.11-rc8: 8.71s > Average result on 4.11-rc8+stackleak: 9.08s (+4.29%) > > Changelog > ========= > > Changes in v8 > > 1. Renamed the STACKLEAK tests to STACKLEAK_ALLOCA and STACKLEAK_DEEP_RECURSION. > Fixed some obsolete comments there. > > 2. Added the comments describing stack erasing on x86_32, because it differs > a lot from one on x86_64 since v7. > > Changes in v7 > > 1. Improved erase_kstack() on x86_64. Now it detects which stack we are > currently using. If we are on the thread stack, erase_kstack() writes the > poison value up to the stack pointer. If we are not on the thread stack (we > are on the trampoline stack introduced by Andy Lutomirski), erase_kstack() > writes the poison value up to the cpu_current_top_of_stack. > > N.B. Right now there are two situations when erase_kstack() is called > on the thread stack: > - from sysret32_from_system_call in entry_64_compat.S; > - from syscall_trace_enter() (see the 3rd patch of this series). > > 2. Introduced STACKLEAK_POISON_CHECK_DEPTH macro and BUILD_BUG_ON() checking > its consistency with CONFIG_STACKLEAK_TRACK_MIN_SIZE. > > 3. Added "disable" option for the STACKLEAK gcc plugin (asked by Laura Abbott). > > 4. Improved CONFIG_STACKLEAK_METRICS. Now /proc//stack_depth shows > the maximum kernel stack consumption for the current and previous syscalls. > Although this information is not precise, it can be useful for estimating > the STACKLEAK performance impact for different workloads. > > 5. Removed redundant erase_kstack() calls from syscall_trace_enter(). > There is no need to erase the stack in syscall_trace_enter() when it > returns -1 (thanks to Dmitry Levin for noticing that). > > 6. Introduced MIN_STACK_LEFT to get rid of hardcoded numbers in check_alloca(). > > Changes in v6 > > 1. Examined syscall entry/exit paths. > - Added missing erase_kstack() call at ret_from_fork() for x86_32. > - Added missing erase_kstack() call at syscall_trace_enter(). > - Solved questions previously marked with TODO. > > 2. Rebased onto v4.15-rc2, which includes Andy Lutomirski's entry changes. > Andy removed sp0 from thread_struct for x86_64, which was the only issue > during rebasing. > > 3. Removed the recursive BUG() in track_stack() that was caused by the code > instrumentation. Instead, CONFIG_GCC_PLUGIN_STACKLEAK now implies > CONFIG_VMAP_STACK and CONFIG_SCHED_STACK_END_CHECK, which seems to be > an optimal solution. > > 4. Put stack erasing in syscall_trace_enter() into a separate patch and > fixed my mistake with secure_computing() (found by Tycho Andersen). > > 5. After some experiments, kept the asm implementation of erase_kstack(), > because it gives a full control over the stack for clearing it neatly > and doesn't offend KASAN. > > 6. Improved the comments describing STACKLEAK. > > Changes in v5 (mostly according to the feedback from Ingo Molnar) > > 1. Introduced the CONFIG_STACKLEAK_METRICS providing STACKLEAK information > about tasks via the /proc file system. That information can be useful for > estimating the STACKLEAK performance impact for different workloads. > In particular, /proc//lowest_stack shows the current lowest_stack > value and its final value from the previous syscall. > > 2. Introduced a single check_alloca() implementation working for both > x86_64 and x86_32. > > 3. Fixed coding style issues and did some refactoring in the STACKLEAK > gcc plugin. > > 4. Put the gcc plugin and the kernel stack erasing into separate (working) > patches. > > Changes in v4 > > 1. Introduced the CONFIG_STACKLEAK_TRACK_MIN_SIZE parameter instead of > hard-coded track-lowest-sp. > > 2. Carefully looked into the assertions in track_stack() and check_alloca(). > - Fixed the incorrect BUG() condition in track_stack(), thanks to Tycho > Andersen. Also disabled that check if CONFIG_VMAP_STACK is enabled. > - Fixed the surplus and erroneous code for calculating stack_left in > check_alloca() on x86_64. That code repeats the work which is already > done in get_stack_info() and it misses the fact that different > exception stacks on x86_64 have different size. > > 3. Introduced two lkdtm tests for the STACKLEAK feature (developed together > with Tycho Andersen). > > 4. Added info about STACKLEAK to Documentation/security/self-protection.rst. > > 5. Improved the comments. > > 6. Decided not to change "unsigned long sp = (unsigned long)&sp" to > current_stack_pointer. The original variant is more platform independent > since current_stack_pointer has different type on x86 and arm. > > Changes in v3 > > 1. Added the detailed comments describing the STACKLEAK gcc plugin. > Read the plugin from the bottom up, like you do for Linux kernel drivers. > Additional information: > - gcc internals documentation available from gcc sources; > - wonderful slides by Diego Novillo > https://www.airs.com/dnovillo/200711-GCC-Internals/ > - nice introduction to gcc plugins at LWN > https://lwn.net/Articles/457543/ > > 2. Improved the commit message and Kconfig description according to the > feedback from Kees Cook. Also added the original notice describing > the performance impact of the STACKLEAK feature. > > 3. Removed the arch-specific ix86_cmodel check in stackleak_track_stack_gate(). > It caused skipping the kernel code instrumentation for i386. > > 4. Fixed a minor mistake in stackleak_tree_instrument_execute(). > First versions of the plugin used ENTRY_BLOCK_PTR_FOR_FN(cfun)->next_bb > to get the basic block with the function prologue. That was not correct > since the control flow graph edge from ENTRY_BLOCK_PTR doesn't always > go to that basic block. > > So later it was changed to single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun)), > but not completely. next_bb was still used for entry_bb assignment, > which could cause the wrong value of the prologue_instrumented variable. > > I've reported this issue to Grsecurity and proposed the fix for it, but > unfortunately didn't get any reply. > > 5. Introduced the STACKLEAK_POISON macro and renamed the config option > according to the feedback from Kees Cook. > > Ideas for further work > ====================== > > - Think of erasing stack on the way out of exception handlers (idea by > Andy Lutomirski). > - Think of erasing the kernel stack after invoking EFI runtime services > (idea by Mark Rutland). > - Try to port STACKLEAK to arm64 (Laura Abbott is working on it). > > > Alexander Popov (6): > x86/entry: Add STACKLEAK erasing the kernel stack at the end of > syscalls > gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack > x86/entry: Erase kernel stack in syscall_trace_enter() > lkdtm: Add a test for STACKLEAK > fs/proc: Show STACKLEAK metrics in the /proc file system > doc: self-protection: Add information about STACKLEAK feature > > Documentation/security/self-protection.rst | 23 +- > arch/Kconfig | 53 ++++ > arch/x86/Kconfig | 1 + > arch/x86/entry/common.c | 7 + > arch/x86/entry/entry_32.S | 93 ++++++ > arch/x86/entry/entry_64.S | 113 +++++++ > arch/x86/entry/entry_64_compat.S | 11 + > arch/x86/include/asm/processor.h | 7 + > arch/x86/kernel/asm-offsets.c | 11 + > arch/x86/kernel/dumpstack.c | 20 ++ > arch/x86/kernel/process_32.c | 8 + > arch/x86/kernel/process_64.c | 8 + > drivers/misc/Makefile | 3 + > drivers/misc/lkdtm.h | 4 + > drivers/misc/lkdtm_core.c | 2 + > drivers/misc/lkdtm_stackleak.c | 136 +++++++++ > fs/exec.c | 33 ++ > fs/proc/base.c | 18 ++ > include/linux/compiler.h | 6 + > scripts/Makefile.gcc-plugins | 3 + > scripts/gcc-plugins/stackleak_plugin.c | 471 +++++++++++++++++++++++++++++ > 21 files changed, 1022 insertions(+), 9 deletions(-) > create mode 100644 drivers/misc/lkdtm_stackleak.c > create mode 100644 scripts/gcc-plugins/stackleak_plugin.c > This doesn't work with gcc-8. One of the errors is a minor API change which can be fixed up but a bigger problem is they changed get_frame_size /* Return size needed for stack frame based on slots so far allocated. This size counts from zero. It is not rounded to STACK_BOUNDARY; the caller may have to do that. */ extern poly_int64 get_frame_size (void); This is described at https://gcc.gnu.org/onlinedocs//gccint/poly_005fint.html but I'm still trying to come up with a good solution. Feel free to beat me to fixing it... Thanks, Laura