[kernel-hardening] [PATCH RFC v6 0/6] Introduce the STACKLEAK feature and a test for it

[kernel-hardening] [PATCH RFC v6 0/6] Introduce the STACKLEAK feature and a test for it

[Alexander Popov]

This is the 6th version of the patch series introducing STACKLEAK to the
mainline kernel. 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%)

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/<pid>/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                    |  19 +-
 arch/x86/entry/entry_32.S                  |  69 +++++
 arch/x86/entry/entry_64.S                  |  93 ++++++
 arch/x86/entry/entry_64_compat.S           |   8 +
 arch/x86/include/asm/processor.h           |   7 +
 arch/x86/kernel/asm-offsets.c              |  14 +
 arch/x86/kernel/dumpstack.c                |  15 +
 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             | 139 +++++++++
 fs/exec.c                                  |  25 ++
 fs/proc/base.c                             |  14 +
 include/linux/compiler.h                   |   5 +
 scripts/Makefile.gcc-plugins               |   3 +
 scripts/gcc-plugins/stackleak_plugin.c     | 470 +++++++++++++++++++++++++++++
 21 files changed, 971 insertions(+), 12 deletions(-)
 create mode 100644 drivers/misc/lkdtm_stackleak.c
 create mode 100644 scripts/gcc-plugins/stackleak_plugin.c

-- 
2.7.4

[kernel-hardening] [PATCH RFC v6 1/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls

[Alexander Popov]

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 architecture-specific code filling the used
part of the kernel stack with a poison value before returning to the
userspace. Full STACKLEAK feature also contains the gcc plugin which
comes in a separate commit.

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@linux.com>
---
 arch/Kconfig                     | 27 ++++++++++++
 arch/x86/Kconfig                 |  1 +
 arch/x86/entry/entry_32.S        | 65 +++++++++++++++++++++++++++++
 arch/x86/entry/entry_64.S        | 89 ++++++++++++++++++++++++++++++++++++++++
 arch/x86/entry/entry_64_compat.S |  8 ++++
 arch/x86/include/asm/processor.h |  4 ++
 arch/x86/kernel/asm-offsets.c    | 11 +++++
 arch/x86/kernel/process_32.c     |  5 +++
 arch/x86/kernel/process_64.c     |  5 +++
 include/linux/compiler.h         |  5 +++
 10 files changed, 220 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index 400b9e1..721fdae 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -387,6 +387,13 @@ config SECCOMP_FILTER
 
 	  See Documentation/prctl/seccomp_filter.txt for details.
 
+config HAVE_ARCH_STACKLEAK
+	bool
+	help
+	  An architecture should select this if it has the code which
+	  fills the used part of the kernel stack with the STACKLEAK_POISON
+	  value before returning from system calls.
+
 config HAVE_GCC_PLUGINS
 	bool
 	help
@@ -517,6 +524,26 @@ config GCC_PLUGIN_RANDSTRUCT_PERFORMANCE
 	  in structures.  This reduces the performance hit of RANDSTRUCT
 	  at the cost of weakened randomization.
 
+config GCC_PLUGIN_STACKLEAK
+	bool "Erase the kernel stack before returning from syscalls"
+	depends on GCC_PLUGINS
+	depends on HAVE_ARCH_STACKLEAK
+	help
+	  This option makes the kernel erase the kernel stack before it
+	  returns from a system call. That reduces the information which
+	  kernel stack leak bugs can reveal and blocks some uninitialized
+	  stack variable attacks. This option also provides runtime checks
+	  for kernel stack overflow detection.
+
+	  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.
+
+	  This plugin was ported from grsecurity/PaX. More information at:
+	   * https://grsecurity.net/
+	   * https://pax.grsecurity.net/
+
 config HAVE_CC_STACKPROTECTOR
 	bool
 	help
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 8eed3f9..6646fcb 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -116,6 +116,7 @@ config X86
 	select HAVE_ARCH_MMAP_RND_COMPAT_BITS	if MMU && COMPAT
 	select HAVE_ARCH_COMPAT_MMAP_BASES	if MMU && COMPAT
 	select HAVE_ARCH_SECCOMP_FILTER
+	select HAVE_ARCH_STACKLEAK
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD if X86_64
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 4838037..8e4f815 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -76,6 +76,66 @@
 #endif
 .endm
 
+.macro erase_kstack
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	call erase_kstack
+#endif
+.endm
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+/* For the detailed comments, see erase_kstack in entry_64.S */
+ENTRY(erase_kstack)
+	pushl	%edi
+	pushl	%ecx
+	pushl	%eax
+	pushl	%ebp
+
+	movl	PER_CPU_VAR(current_task), %ebp
+	mov	TASK_lowest_stack(%ebp), %edi
+	mov	$STACKLEAK_POISON, %eax
+	std
+
+1:
+	mov	%edi, %ecx
+	and	$THREAD_SIZE_asm - 1, %ecx
+	shr	$2, %ecx
+	repne	scasl
+	jecxz	2f
+
+	cmp	$32, %ecx
+	jc	2f
+
+	mov	$32, %ecx
+	repe	scasl
+	jecxz	2f
+	jne	1b
+
+2:
+	cld
+	or	$2*4, %edi
+	mov	%esp, %ecx
+	sub	%edi, %ecx
+
+	cmp	$THREAD_SIZE_asm, %ecx
+	jb	3f
+	ud2
+
+3:
+	shr	$2, %ecx
+	rep	stosl
+
+	mov	TASK_thread_sp0(%ebp), %edi
+	sub	$128, %edi
+	mov	%edi, TASK_lowest_stack(%ebp)
+
+	popl	%ebp
+	popl	%eax
+	popl	%ecx
+	popl	%edi
+	ret
+ENDPROC(erase_kstack)
+#endif
+
 /*
  * User gs save/restore
  *
@@ -286,6 +346,7 @@ ENTRY(ret_from_fork)
 	/* When we fork, we trace the syscall return in the child, too. */
 	movl    %esp, %eax
 	call    syscall_return_slowpath
+	erase_kstack
 	jmp     restore_all
 
 	/* kernel thread */
@@ -446,6 +507,8 @@ ENTRY(entry_SYSENTER_32)
 	ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
 		    "jmp .Lsyscall_32_done", X86_FEATURE_XENPV
 
+	erase_kstack
+
 /* Opportunistic SYSEXIT */
 	TRACE_IRQS_ON			/* User mode traces as IRQs on. */
 	movl	PT_EIP(%esp), %edx	/* pt_regs->ip */
@@ -532,6 +595,8 @@ ENTRY(entry_INT80_32)
 	call	do_int80_syscall_32
 .Lsyscall_32_done:
 
+	erase_kstack
+
 restore_all:
 	TRACE_IRQS_IRET
 .Lrestore_all_notrace:
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index f81d50d..94f659d 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -64,6 +64,90 @@ END(native_usergs_sysret64)
 	TRACE_IRQS_FLAGS EFLAGS(%rsp)
 .endm
 
+.macro erase_kstack
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	call erase_kstack
+#endif
+.endm
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+ENTRY(erase_kstack)
+	pushq	%rdi
+	pushq	%rcx
+	pushq	%rax
+	pushq	%r11
+
+	movq	PER_CPU_VAR(current_task), %r11
+	mov	TASK_lowest_stack(%r11), %rdi
+	mov	$STACKLEAK_POISON, %rax
+	std
+
+	/*
+	 * Let's search for the poison value in the stack.
+	 * Start from the lowest_stack and go to the bottom (see std above).
+	 */
+1:
+	mov	%edi, %ecx
+	and	$THREAD_SIZE_asm - 1, %ecx
+	shr	$3, %ecx
+	repne	scasq
+	jecxz	2f	/* Didn't find it. Go to poisoning. */
+
+	/*
+	 * Found the poison value in the stack. Go to poisoning if there are
+	 * less than 16 qwords left.
+	 */
+	cmp	$16, %ecx
+	jc	2f
+
+	/*
+	 * Check that 16 further qwords contain poison (avoid false positives).
+	 * If so, the part of the stack below the address in %rdi is likely
+	 * to be poisoned. Otherwise we need to search deeper.
+	 */
+	mov	$16, %ecx
+	repe	scasq
+	jecxz	2f	/* Poison the upper part of the stack. */
+	jne	1b	/* Search deeper. */
+
+2:
+	/*
+	 * Prepare the counter for poisoning the kernel stack between
+	 * %rdi and %rsp. Two qwords at the bottom of the stack are reserved
+	 * and should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+	 */
+	cld
+	or	$2*8, %rdi
+	mov	%esp, %ecx
+	sub	%edi, %ecx
+
+	/* Check that the counter value is sane. */
+	cmp	$THREAD_SIZE_asm, %rcx
+	jb	3f
+	ud2
+
+3:
+	/*
+	 * So let's write the poison value to the kernel stack. Start from the
+	 * address in %rdi and move up (see cld above) to the address in %rsp
+	 * (not included, used memory).
+	 */
+	shr	$3, %ecx
+	rep	stosq
+
+	/* Set the lowest_stack value to the top_of_stack - 256. */
+	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rdi
+	sub	$256, %rdi
+	mov	%rdi, TASK_lowest_stack(%r11)
+
+	popq	%r11
+	popq	%rax
+	popq	%rcx
+	popq	%rdi
+	ret
+ENDPROC(erase_kstack)
+#endif
+
 /*
  * When dynamic function tracer is enabled it will add a breakpoint
  * to all locations that it is about to modify, sync CPUs, update
@@ -221,6 +305,8 @@ entry_SYSCALL_64_fastpath:
 	testl	$_TIF_ALLWORK_MASK, TASK_TI_flags(%r11)
 	jnz	1f
 
+	erase_kstack
+
 	LOCKDEP_SYS_EXIT
 	TRACE_IRQS_ON		/* user mode is traced as IRQs on */
 	movq	RIP(%rsp), %rcx
@@ -249,6 +335,8 @@ entry_SYSCALL64_slow_path:
 	call	do_syscall_64		/* returns with IRQs disabled */
 
 return_from_SYSCALL_64:
+	erase_kstack
+
 	TRACE_IRQS_IRETQ		/* we're about to change IF */
 
 	/*
@@ -432,6 +520,7 @@ ENTRY(ret_from_fork)
 	UNWIND_HINT_REGS
 	movq	%rsp, %rdi
 	call	syscall_return_slowpath	/* returns with IRQs disabled */
+	erase_kstack
 	TRACE_IRQS_ON			/* user mode is traced as IRQS on */
 	jmp	swapgs_restore_regs_and_return_to_usermode
 
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 568e130..8f3b13b 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -19,6 +19,12 @@
 
 	.section .entry.text, "ax"
 
+	.macro erase_kstack
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	call erase_kstack
+#endif
+	.endm
+
 /*
  * 32-bit SYSENTER entry.
  *
@@ -229,6 +235,7 @@ GLOBAL(entry_SYSCALL_compat_after_hwframe)
 
 	/* Opportunistic SYSRET */
 sysret32_from_system_call:
+	erase_kstack
 	TRACE_IRQS_ON			/* User mode traces as IRQs on. */
 	movq	RBX(%rsp), %rbx		/* pt_regs->rbx */
 	movq	RBP(%rsp), %rbp		/* pt_regs->rbp */
@@ -336,6 +343,7 @@ ENTRY(entry_INT80_compat)
 .Lsyscall_32_done:
 
 	/* Go back to user mode. */
+	erase_kstack
 	TRACE_IRQS_ON
 	jmp	swapgs_restore_regs_and_return_to_usermode
 END(entry_INT80_compat)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index cc16fa8..520508d 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -481,6 +481,10 @@ struct thread_struct {
 
 	mm_segment_t		addr_limit;
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	unsigned long		lowest_stack;
+#endif
+
 	unsigned int		sig_on_uaccess_err:1;
 	unsigned int		uaccess_err:1;	/* uaccess failed */
 
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 8ea7827..692c10e 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -38,6 +38,12 @@ void common(void) {
 	BLANK();
 	OFFSET(TASK_TI_flags, task_struct, thread_info.flags);
 	OFFSET(TASK_addr_limit, task_struct, thread.addr_limit);
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	OFFSET(TASK_lowest_stack, task_struct, thread.lowest_stack);
+# ifdef CONFIG_X86_32
+	OFFSET(TASK_thread_sp0, task_struct, thread.sp0);
+# endif
+#endif
 
 	BLANK();
 	OFFSET(crypto_tfm_ctx_offset, crypto_tfm, __crt_ctx);
@@ -74,6 +80,11 @@ void common(void) {
 	OFFSET(PV_MMU_read_cr2, pv_mmu_ops, read_cr2);
 #endif
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	BLANK();
+	DEFINE(THREAD_SIZE_asm, THREAD_SIZE);
+#endif
+
 #ifdef CONFIG_XEN
 	BLANK();
 	OFFSET(XEN_vcpu_info_mask, vcpu_info, evtchn_upcall_mask);
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 45bf0c5..2bea3bf 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -136,6 +136,11 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	p->thread.sp0 = (unsigned long) (childregs+1);
 	memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	p->thread.lowest_stack = (unsigned long)task_stack_page(p) +
+						2 * sizeof(unsigned long);
+#endif
+
 	if (unlikely(p->flags & PF_KTHREAD)) {
 		/* kernel thread */
 		memset(childregs, 0, sizeof(struct pt_regs));
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index eeeb34f..1641463 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -282,6 +282,11 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	p->thread.sp = (unsigned long) fork_frame;
 	p->thread.io_bitmap_ptr = NULL;
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	p->thread.lowest_stack = (unsigned long)task_stack_page(p) +
+						2 * sizeof(unsigned long);
+#endif
+
 	savesegment(gs, p->thread.gsindex);
 	p->thread.gsbase = p->thread.gsindex ? 0 : me->thread.gsbase;
 	savesegment(fs, p->thread.fsindex);
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 188ed9f..4e543d0 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -352,4 +352,9 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 	(volatile typeof(x) *)&(x); })
 #define ACCESS_ONCE(x) (*__ACCESS_ONCE(x))
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+/* It points to the unused hole in the virtual memory map  */
+# define STACKLEAK_POISON -0xBEEF
+#endif
+
 #endif /* __LINUX_COMPILER_H */
-- 
2.7.4

[kernel-hardening] [PATCH RFC v6 2/6] gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack

[Alexander Popov]

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@linux.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.
+
 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
diff --git a/fs/exec.c b/fs/exec.c
index 6be2aa0..2b819bb 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1963,3 +1963,28 @@ COMPAT_SYSCALL_DEFINE5(execveat, int, fd,
 				  argv, envp, flags);
 }
 #endif
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used track_stack(void)
+{
+	/*
+	 * N.B. The arch-specific part of the STACKLEAK feature fills the
+	 * kernel stack with the poison value, which has the register width.
+	 * That code assumes that the value of thread.lowest_stack is aligned
+	 * on the register width boundary.
+	 *
+	 * That is true for x86 and x86_64 because of the kernel stack
+	 * alignment on these platforms (for details, see cc_stack_align in
+	 * arch/x86/Makefile). Take care of that when you port STACKLEAK to
+	 * new platforms.
+	 */
+	unsigned long sp = (unsigned long)&sp;
+
+	if (sp < current->thread.lowest_stack &&
+	    sp >= (unsigned long)task_stack_page(current) +
+					2 * sizeof(unsigned long)) {
+		current->thread.lowest_stack = sp;
+	}
+}
+EXPORT_SYMBOL(track_stack);
+#endif /* CONFIG_GCC_PLUGIN_STACKLEAK */
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index b2a95af..8d6070f 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -35,6 +35,9 @@ ifdef CONFIG_GCC_PLUGINS
   gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_RANDSTRUCT)	+= -DRANDSTRUCT_PLUGIN
   gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_RANDSTRUCT_PERFORMANCE)	+= -fplugin-arg-randomize_layout_plugin-performance-mode
 
+  gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK)	+= stackleak_plugin.so
+  gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK)	+= -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
+
   GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
 
   export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c
new file mode 100644
index 0000000..1461d88
--- /dev/null
+++ b/scripts/gcc-plugins/stackleak_plugin.c
@@ -0,0 +1,470 @@
+/*
+ * Copyright 2011-2017 by the PaX Team <pageexec@freemail.hu>
+ * Modified by Alexander Popov <alex.popov@linux.com>
+ * Licensed under the GPL v2
+ *
+ * Note: the choice of the license means that the compilation process is
+ * NOT 'eligible' as defined by gcc's library exception to the GPL v3,
+ * but for the kernel it doesn't matter since it doesn't link against
+ * any of the gcc libraries
+ *
+ * This gcc plugin is needed for tracking the lowest border of the kernel stack
+ * and checking that alloca calls don't cause stack overflow. It 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 the "track-min-size" plugin parameter.
+ *
+ * This plugin is ported from grsecurity/PaX. For more information see:
+ *   https://grsecurity.net/
+ *   https://pax.grsecurity.net/
+ *
+ * Debugging:
+ *  - use fprintf() to stderr, debug_generic_expr(), debug_gimple_stmt()
+ *     and print_rtl();
+ *  - add "-fdump-tree-all -fdump-rtl-all" to the plugin CFLAGS in
+ *     Makefile.gcc-plugins to see the verbose dumps of the gcc passes;
+ *  - use gcc -E to understand the preprocessing shenanigans;
+ *  - use gcc with enabled CFG/GIMPLE/SSA verification (--enable-checking).
+ */
+
+#include "gcc-common.h"
+
+__visible int plugin_is_GPL_compatible;
+
+static int track_frame_size = -1;
+static const char track_function[] = "track_stack";
+static const char check_function[] = "check_alloca";
+
+/*
+ * Mark these global variables (roots) for gcc garbage collector since
+ * they point to the garbage-collected memory.
+ */
+static GTY(()) tree track_function_decl;
+static GTY(()) tree check_function_decl;
+
+static struct plugin_info stackleak_plugin_info = {
+	.version = "201707101337",
+	.help = "track-min-size=nn\ttrack stack for functions with a stack frame size >= nn bytes\n"
+};
+
+static void stackleak_check_alloca(gimple_stmt_iterator *gsi)
+{
+	gimple stmt;
+	gcall *check_alloca;
+	tree alloca_size;
+	cgraph_node_ptr node;
+	int frequency;
+	basic_block bb;
+
+	/* Insert call to void check_alloca(unsigned long size) */
+	alloca_size = gimple_call_arg(gsi_stmt(*gsi), 0);
+	stmt = gimple_build_call(check_function_decl, 1, alloca_size);
+	check_alloca = as_a_gcall(stmt);
+	gsi_insert_before(gsi, check_alloca, GSI_SAME_STMT);
+
+	/* Update the cgraph */
+	bb = gimple_bb(check_alloca);
+	node = cgraph_get_create_node(check_function_decl);
+	gcc_assert(node);
+	frequency = compute_call_stmt_bb_frequency(current_function_decl, bb);
+	cgraph_create_edge(cgraph_get_node(current_function_decl), node,
+			check_alloca, bb->count, frequency, bb->loop_depth);
+}
+
+static void stackleak_add_instrumentation(gimple_stmt_iterator *gsi, bool after)
+{
+	gimple stmt;
+	gcall *track_stack;
+	cgraph_node_ptr node;
+	int frequency;
+	basic_block bb;
+
+	/* Insert call to void track_stack(void) */
+	stmt = gimple_build_call(track_function_decl, 0);
+	track_stack = as_a_gcall(stmt);
+	if (after)
+		gsi_insert_after(gsi, track_stack, GSI_CONTINUE_LINKING);
+	else
+		gsi_insert_before(gsi, track_stack, GSI_SAME_STMT);
+
+	/* Update the cgraph */
+	bb = gimple_bb(track_stack);
+	node = cgraph_get_create_node(track_function_decl);
+	gcc_assert(node);
+	frequency = compute_call_stmt_bb_frequency(current_function_decl, bb);
+	cgraph_create_edge(cgraph_get_node(current_function_decl), node,
+			track_stack, bb->count, frequency, bb->loop_depth);
+}
+
+static bool is_alloca(gimple stmt)
+{
+	if (gimple_call_builtin_p(stmt, BUILT_IN_ALLOCA))
+		return true;
+
+#if BUILDING_GCC_VERSION >= 4007
+	if (gimple_call_builtin_p(stmt, BUILT_IN_ALLOCA_WITH_ALIGN))
+		return true;
+#endif
+
+	return false;
+}
+
+/*
+ * Work with the GIMPLE representation of the code.
+ * Insert the check_alloca() call before alloca and track_stack() call after
+ * it. Also insert track_stack() call into the beginning of the function
+ * if it is not instrumented.
+ */
+static unsigned int stackleak_tree_instrument_execute(void)
+{
+	basic_block bb, entry_bb;
+	bool prologue_instrumented = false, is_leaf = true;
+	gimple_stmt_iterator gsi;
+
+	/*
+	 * ENTRY_BLOCK_PTR is a basic block which represents possible entry
+	 * point of a function. This block does not contain any code and
+	 * has a CFG edge to its successor.
+	 */
+	gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
+	entry_bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
+
+	/*
+	 * 1. Loop through the GIMPLE statements in each of cfun basic blocks.
+	 * cfun is a global variable which represents the function that is
+	 * currently processed.
+	 */
+	FOR_EACH_BB_FN(bb, cfun) {
+		for (gsi = gsi_start_bb(bb); !gsi_end_p(gsi); gsi_next(&gsi)) {
+			gimple stmt;
+
+			stmt = gsi_stmt(gsi);
+
+			/* Leaf function is a function which makes no calls */
+			if (is_gimple_call(stmt))
+				is_leaf = false;
+
+			if (!is_alloca(stmt))
+				continue;
+
+			/* 2. Insert stack overflow check before alloca call */
+			stackleak_check_alloca(&gsi);
+
+			/* 3. Insert track_stack() call after alloca call */
+			stackleak_add_instrumentation(&gsi, true);
+			if (bb == entry_bb)
+				prologue_instrumented = true;
+		}
+	}
+
+	if (prologue_instrumented)
+		return 0;
+
+	/*
+	 * Special cases to skip the instrumentation.
+	 *
+	 * Taking the address of static inline functions materializes them,
+	 * but we mustn't instrument some of them as the resulting stack
+	 * alignment required by the function call ABI will break other
+	 * assumptions regarding the expected (but not otherwise enforced)
+	 * register clobbering ABI.
+	 *
+	 * Case in point: native_save_fl on amd64 when optimized for size
+	 * clobbers rdx if it were instrumented here.
+	 *
+	 * TODO: any more special cases?
+	 */
+	if (is_leaf &&
+	    !TREE_PUBLIC(current_function_decl) &&
+	    DECL_DECLARED_INLINE_P(current_function_decl)) {
+		return 0;
+	}
+
+	if (is_leaf &&
+	    !strncmp(IDENTIFIER_POINTER(DECL_NAME(current_function_decl)),
+		     "_paravirt_", 10)) {
+		return 0;
+	}
+
+	/* 4. Insert track_stack() call at the function beginning */
+	bb = entry_bb;
+	if (!single_pred_p(bb)) {
+		/* gcc_assert(bb_loop_depth(bb) ||
+				(bb->flags & BB_IRREDUCIBLE_LOOP)); */
+		split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
+		gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
+		bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
+	}
+	gsi = gsi_after_labels(bb);
+	stackleak_add_instrumentation(&gsi, false);
+
+	return 0;
+}
+
+/*
+ * Work with the RTL representation of the code.
+ * Remove the unneeded track_stack() calls from the functions which don't
+ * call alloca and have the stack frame size less than track_frame_size.
+ */
+static unsigned int stackleak_final_execute(void)
+{
+	rtx_insn *insn, *next;
+
+	if (cfun->calls_alloca)
+		return 0;
+
+	if (get_frame_size() >= track_frame_size)
+		return 0;
+
+	/*
+	 * 1. Find track_stack() calls. Loop through the chain of insns,
+	 * which is an RTL representation of the code for a function.
+	 *
+	 * The example of a matching insn:
+	 *    (call_insn 8 4 10 2 (call (mem (symbol_ref ("track_stack")
+	 *    [flags 0x41] <function_decl 0x7f7cd3302a80 track_stack>)
+	 *    [0 track_stack S1 A8]) (0)) 675 {*call} (expr_list
+	 *    (symbol_ref ("track_stack") [flags 0x41] <function_decl
+	 *    0x7f7cd3302a80 track_stack>) (expr_list (0) (nil))) (nil))
+	 */
+	for (insn = get_insns(); insn; insn = next) {
+		rtx body;
+
+		next = NEXT_INSN(insn);
+
+		/* Check the expression code of the insn */
+		if (!CALL_P(insn))
+			continue;
+
+		/*
+		 * Check the expression code of the insn body, which is an RTL
+		 * Expression (RTX) describing the side effect performed by
+		 * that insn.
+		 */
+		body = PATTERN(insn);
+		if (GET_CODE(body) != CALL)
+			continue;
+
+		/*
+		 * Check the first operand of the call expression. It should
+		 * be a mem RTX describing the needed subroutine with a
+		 * symbol_ref RTX.
+		 */
+		body = XEXP(body, 0);
+		if (GET_CODE(body) != MEM)
+			continue;
+
+		body = XEXP(body, 0);
+		if (GET_CODE(body) != SYMBOL_REF)
+			continue;
+
+		if (SYMBOL_REF_DECL(body) != track_function_decl)
+			continue;
+
+		/* 2. Delete the track_stack() call */
+		delete_insn_and_edges(insn);
+#if BUILDING_GCC_VERSION >= 4007
+		if (GET_CODE(next) == NOTE &&
+		    NOTE_KIND(next) == NOTE_INSN_CALL_ARG_LOCATION) {
+			insn = next;
+			next = NEXT_INSN(insn);
+			delete_insn_and_edges(insn);
+		}
+#endif
+	}
+
+	/*
+	 * Uncomment the following to see the code which was cleaned at this
+	 * pass. It should not contain check_alloca() and track_stack() calls.
+	 * The stack frame size should be less than track_frame_size.
+	 *
+	 * warning(0, "Instrumentation is removed, stack frame size: %ld",
+	 * 						get_frame_size());
+	 * print_simple_rtl(stderr, get_insns());
+	 */
+
+	return 0;
+}
+
+static bool stackleak_track_stack_gate(void)
+{
+	tree section;
+
+	section = lookup_attribute("section",
+				   DECL_ATTRIBUTES(current_function_decl));
+	if (section && TREE_VALUE(section)) {
+		section = TREE_VALUE(TREE_VALUE(section));
+
+		if (!strncmp(TREE_STRING_POINTER(section), ".init.text", 10))
+			return false;
+		if (!strncmp(TREE_STRING_POINTER(section), ".devinit.text", 13))
+			return false;
+		if (!strncmp(TREE_STRING_POINTER(section), ".cpuinit.text", 13))
+			return false;
+		if (!strncmp(TREE_STRING_POINTER(section), ".meminit.text", 13))
+			return false;
+	}
+
+	return track_frame_size >= 0;
+}
+
+/* Build function declarations for track_stack() and check_alloca() */
+static void stackleak_start_unit(void *gcc_data __unused,
+				 void *user_data __unused)
+{
+	tree fntype;
+
+	/* void track_stack(void) */
+	fntype = build_function_type_list(void_type_node, NULL_TREE);
+	track_function_decl = build_fn_decl(track_function, fntype);
+	DECL_ASSEMBLER_NAME(track_function_decl); /* for LTO */
+	TREE_PUBLIC(track_function_decl) = 1;
+	TREE_USED(track_function_decl) = 1;
+	DECL_EXTERNAL(track_function_decl) = 1;
+	DECL_ARTIFICIAL(track_function_decl) = 1;
+	DECL_PRESERVE_P(track_function_decl) = 1;
+
+	/* void check_alloca(unsigned long) */
+	fntype = build_function_type_list(void_type_node,
+				long_unsigned_type_node, NULL_TREE);
+	check_function_decl = build_fn_decl(check_function, fntype);
+	DECL_ASSEMBLER_NAME(check_function_decl); /* for LTO */
+	TREE_PUBLIC(check_function_decl) = 1;
+	TREE_USED(check_function_decl) = 1;
+	DECL_EXTERNAL(check_function_decl) = 1;
+	DECL_ARTIFICIAL(check_function_decl) = 1;
+	DECL_PRESERVE_P(check_function_decl) = 1;
+}
+
+/*
+ * Pass gate function is a predicate function that gets executed before the
+ * corresponding pass. If the return value is 'true' the pass gets executed,
+ * otherwise, it is skipped.
+ */
+static bool stackleak_tree_instrument_gate(void)
+{
+	return stackleak_track_stack_gate();
+}
+
+#define PASS_NAME stackleak_tree_instrument
+#define PROPERTIES_REQUIRED PROP_gimple_leh | PROP_cfg
+#define TODO_FLAGS_START TODO_verify_ssa | TODO_verify_flow | TODO_verify_stmts
+#define TODO_FLAGS_FINISH TODO_verify_ssa | TODO_verify_stmts | TODO_dump_func \
+			| TODO_update_ssa | TODO_rebuild_cgraph_edges
+#include "gcc-generate-gimple-pass.h"
+
+static bool stackleak_final_gate(void)
+{
+	return stackleak_track_stack_gate();
+}
+
+#define PASS_NAME stackleak_final
+#define TODO_FLAGS_FINISH TODO_dump_func
+#include "gcc-generate-rtl-pass.h"
+
+/*
+ * Every gcc plugin exports a plugin_init() function that is called right
+ * after the plugin is loaded. This function is responsible for registering
+ * the plugin callbacks and doing other required initialization.
+ */
+__visible int plugin_init(struct plugin_name_args *plugin_info,
+			  struct plugin_gcc_version *version)
+{
+	const char * const plugin_name = plugin_info->base_name;
+	const int argc = plugin_info->argc;
+	const struct plugin_argument * const argv = plugin_info->argv;
+	int i;
+
+	/* Extra GGC root tables describing our GTY-ed data */
+	static const struct ggc_root_tab gt_ggc_r_gt_stackleak[] = {
+		{
+			.base = &track_function_decl,
+			.nelt = 1,
+			.stride = sizeof(track_function_decl),
+			.cb = &gt_ggc_mx_tree_node,
+			.pchw = &gt_pch_nx_tree_node
+		},
+		{
+			.base = &check_function_decl,
+			.nelt = 1,
+			.stride = sizeof(check_function_decl),
+			.cb = &gt_ggc_mx_tree_node,
+			.pchw = &gt_pch_nx_tree_node
+		},
+		LAST_GGC_ROOT_TAB
+	};
+
+	/*
+	 * The stackleak_tree_instrument pass should be executed before the
+	 * "optimized" pass, which is the control flow graph cleanup that is
+	 * performed just before expanding gcc trees to the RTL. In former
+	 * versions of the plugin this new pass was inserted before the
+	 * "tree_profile" pass, which is currently called "profile".
+	 */
+	PASS_INFO(stackleak_tree_instrument, "optimized", 1,
+						PASS_POS_INSERT_BEFORE);
+
+	/*
+	 * The stackleak_final pass should be executed before the "final" pass,
+	 * which turns the RTL (Register Transfer Language) into assembly.
+	 */
+	PASS_INFO(stackleak_final, "final", 1, PASS_POS_INSERT_BEFORE);
+
+	if (!plugin_default_version_check(version, &gcc_version)) {
+		error(G_("incompatible gcc/plugin versions"));
+		return 1;
+	}
+
+	/* Parse the plugin arguments */
+	if (argc != 1) {
+		error(G_("bad number of the plugin arguments: %d"), argc);
+		return 1;
+	}
+
+	if (strcmp(argv[i].key, "track-min-size")) {
+		error(G_("unknown option '-fplugin-arg-%s-%s'"),
+				plugin_name, argv[i].key);
+		return 1;
+	}
+
+	if (!argv[i].value) {
+		error(G_("no value supplied for option '-fplugin-arg-%s-%s'"),
+				plugin_name, argv[i].key);
+		return 1;
+	}
+
+	track_frame_size = atoi(argv[i].value);
+	if (track_frame_size < 0) {
+		error(G_("invalid option argument '-fplugin-arg-%s-%s=%s'"),
+				plugin_name, argv[i].key, argv[i].value);
+		return 1;
+	}
+
+	/* Give the information about the plugin */
+	register_callback(plugin_name, PLUGIN_INFO, NULL,
+						&stackleak_plugin_info);
+
+	/* Register to be called before processing a translation unit */
+	register_callback(plugin_name, PLUGIN_START_UNIT,
+					&stackleak_start_unit, NULL);
+
+	/* Register an extra GCC garbage collector (GGC) root table */
+	register_callback(plugin_name, PLUGIN_REGISTER_GGC_ROOTS, NULL,
+					(void *)&gt_ggc_r_gt_stackleak);
+
+	/*
+	 * Hook into the Pass Manager to register new gcc passes.
+	 *
+	 * The stack frame size info is available only at the last RTL pass,
+	 * when it's too late to insert complex code like a function call.
+	 * So we register two gcc passes to instrument every function at first
+	 * and remove the unneeded instrumentation later.
+	 */
+	register_callback(plugin_name, PLUGIN_PASS_MANAGER_SETUP, NULL,
+					&stackleak_tree_instrument_pass_info);
+	register_callback(plugin_name, PLUGIN_PASS_MANAGER_SETUP, NULL,
+					&stackleak_final_pass_info);
+
+	return 0;
+}
-- 
2.7.4

[kernel-hardening] [PATCH RFC v6 3/6] x86/entry: Erase kernel stack in syscall_trace_enter()

[Alexander Popov]

Make STACKLEAK erase kernel stack after ptrace/seccomp/auditing
not to leave any sensitive information on the stack for the syscall code.

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@linux.com>
---
 arch/x86/entry/common.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index d7d3cc2..d45b7cf 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -45,6 +45,12 @@ __visible inline void enter_from_user_mode(void)
 static inline void enter_from_user_mode(void) {}
 #endif
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+asmlinkage void erase_kstack(void);
+#else
+static void erase_kstack(void) {}
+#endif
+
 static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
 {
 #ifdef CONFIG_X86_64
@@ -81,11 +87,15 @@ static long syscall_trace_enter(struct pt_regs *regs)
 		emulated = true;
 
 	if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
-	    tracehook_report_syscall_entry(regs))
+	    tracehook_report_syscall_entry(regs)) {
+		erase_kstack();
 		return -1L;
+	}
 
-	if (emulated)
+	if (emulated) {
+		erase_kstack();
 		return -1L;
+	}
 
 #ifdef CONFIG_SECCOMP
 	/*
@@ -117,8 +127,10 @@ static long syscall_trace_enter(struct pt_regs *regs)
 		}
 
 		ret = __secure_computing(&sd);
-		if (ret == -1)
+		if (ret == -1) {
+			erase_kstack();
 			return ret;
+		}
 	}
 #endif
 
@@ -127,6 +139,7 @@ static long syscall_trace_enter(struct pt_regs *regs)
 
 	do_audit_syscall_entry(regs, arch);
 
+	erase_kstack();
 	return ret ?: regs->orig_ax;
 }
 
-- 
2.7.4

[kernel-hardening] [PATCH RFC v6 4/6] lkdtm: Add a test for STACKLEAK

[Alexander Popov]

Introduce two lkdtm tests for the STACKLEAK feature: STACKLEAK_CHECK_ALLOCA
and STACKLEAK_TRACK_STACK. Both of them check that the current task stack
is properly erased (filled with STACKLEAK_POISON).

In addition, STACKLEAK_CHECK_ALLOCA tests that:
 - check_alloca() allows alloca calls which don't exhaust the kernel stack;
 - alloca calls which exhaust/overflow the kernel stack hit BUG() in
    check_alloca().

And STACKLEAK_TRACK_STACK checks that exhausting the thread stack with a
recursion is detected:
 - it hits the BUG() in track_stack() if CONFIG_VMAP_STACK is not enabled,
 - or hits the guard page otherwise.

Signed-off-by: Tycho Andersen <tycho@docker.com>
Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 drivers/misc/Makefile          |   3 +
 drivers/misc/lkdtm.h           |   4 ++
 drivers/misc/lkdtm_core.c      |   2 +
 drivers/misc/lkdtm_stackleak.c | 139 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 148 insertions(+)
 create mode 100644 drivers/misc/lkdtm_stackleak.c

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 5ca5f64..66c3605 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -63,6 +63,9 @@ lkdtm-$(CONFIG_LKDTM)		+= lkdtm_perms.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_refcount.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_rodata_objcopy.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_usercopy.o
+lkdtm-$(CONFIG_LKDTM)		+= lkdtm_stackleak.o
+
+KASAN_SANITIZE_lkdtm_stackleak.o := n
 
 KCOV_INSTRUMENT_lkdtm_rodata.o	:= n
 
diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index 687a0db..2a0cd0d 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -83,4 +83,8 @@ void lkdtm_USERCOPY_STACK_FRAME_FROM(void);
 void lkdtm_USERCOPY_STACK_BEYOND(void);
 void lkdtm_USERCOPY_KERNEL(void);
 
+/* lkdtm_stackleak.c */
+void lkdtm_STACKLEAK_CHECK_ALLOCA(void);
+void lkdtm_STACKLEAK_TRACK_STACK(void);
+
 #endif
diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
index ba92291..3a22ffa 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -183,6 +183,8 @@ static const struct crashtype crashtypes[] = {
 	CRASHTYPE(USERCOPY_STACK_FRAME_FROM),
 	CRASHTYPE(USERCOPY_STACK_BEYOND),
 	CRASHTYPE(USERCOPY_KERNEL),
+	CRASHTYPE(STACKLEAK_CHECK_ALLOCA),
+	CRASHTYPE(STACKLEAK_TRACK_STACK),
 };
 
 
diff --git a/drivers/misc/lkdtm_stackleak.c b/drivers/misc/lkdtm_stackleak.c
new file mode 100644
index 0000000..1f95ee3
--- /dev/null
+++ b/drivers/misc/lkdtm_stackleak.c
@@ -0,0 +1,139 @@
+/*
+ * This file tests a few aspects of the STACKLEAK feature:
+ *   - the current task stack is properly erased (filled with STACKLEAK_POISON);
+ *   - check_alloca() allows alloca calls which don't exhaust the kernel stack;
+ *   - alloca calls which exhaust/overflow the kernel stack hit BUG() in
+ *    check_alloca();
+ *   - exhausting the thread stack with a recursion is detected: it hits the
+ *    BUG() in track_stack() if CONFIG_VMAP_STACK is not enabled, or hits the
+ *    guard page otherwise.
+ *
+ * Copyright (C) Docker, Inc. 2017
+ *
+ * Authors:
+ *   Tycho Andersen <tycho@docker.com>
+ *   Alexander Popov <alex.popov@linux.com>
+ */
+
+#include "lkdtm.h"
+#include <linux/sched.h>
+#include <linux/compiler.h>
+
+#ifndef CONFIG_GCC_PLUGIN_STACKLEAK
+# define STACKLEAK_POISON -0xBEEF
+# define CONFIG_STACKLEAK_TRACK_MIN_SIZE 100
+#endif
+
+static noinline bool stack_is_erased(void)
+{
+	unsigned long *sp, left, found, i;
+
+	/*
+	 * For the details about the alignment of the poison values, see
+	 * the comment in track_stack().
+	 */
+	sp = PTR_ALIGN(&i, sizeof(unsigned long));
+
+	left = ((unsigned long)sp & (THREAD_SIZE - 1)) / sizeof(unsigned long);
+	sp--;
+
+	/*
+	 * Two unsigned long ints at the bottom of the thread stack are
+	 * reserved and not poisoned.
+	 */
+	if (left <= 2)
+		return false;
+
+	left -= 2;
+	pr_info("checking unused part of the thread stack (%lu bytes)...\n",
+					left * sizeof(unsigned long));
+
+	/* Search for 17 poison values in a row (like erase_kstack() does) */
+	for (i = 0, found = 0; i < left && found < 17; i++) {
+		if (*(sp - i) == STACKLEAK_POISON)
+			found++;
+		else
+			found = 0;
+	}
+
+	if (found < 17) {
+		pr_err("FAIL: thread stack is not erased (checked %lu bytes)\n",
+						i * sizeof(unsigned long));
+		return false;
+	}
+
+	pr_info("first %lu bytes are unpoisoned\n",
+				(i - found) * sizeof(unsigned long));
+
+	/* The rest of thread stack should be erased */
+	for (; i < left; i++) {
+		if (*(sp - i) != STACKLEAK_POISON) {
+			pr_err("FAIL: thread stack is NOT properly erased\n");
+			return false;
+		}
+	}
+
+	pr_info("the rest of the thread stack is properly erased\n");
+	return true;
+}
+
+static noinline void do_alloca(unsigned long size)
+{
+	char buf[size];
+
+	/* So this doesn't get inlined or optimized out */
+	snprintf(buf, size, "hello world\n");
+}
+
+void lkdtm_STACKLEAK_CHECK_ALLOCA(void)
+{
+	unsigned long left = (unsigned long)&left & (THREAD_SIZE - 1);
+
+	if (!stack_is_erased())
+		return;
+
+	/* Try a small alloca to see if it works */
+	pr_info("try a small alloca of 16 bytes...\n");
+	do_alloca(16);
+	pr_info("small alloca is successful\n");
+
+	/* Try to hit the BUG() in check_alloca() */
+	pr_info("try a large alloca of %lu bytes (stack overflow)...\n", left);
+	do_alloca(left);
+	pr_err("FAIL: large alloca overstepped the thread stack boundary\n");
+}
+
+/*
+ * The stack frame size of recursion() is bigger than the
+ * CONFIG_STACKLEAK_TRACK_MIN_SIZE, hence that function is instrumented
+ * by the STACKLEAK gcc plugin and it calls track_stack() at the beginning.
+ */
+static noinline unsigned long recursion(unsigned long prev_sp)
+{
+	char buf[CONFIG_STACKLEAK_TRACK_MIN_SIZE + 42];
+	unsigned long sp = (unsigned long)&sp;
+
+	snprintf(buf, sizeof(buf), "hello world\n");
+
+	if (prev_sp < sp + THREAD_SIZE)
+		sp = recursion(prev_sp);
+
+	return sp;
+}
+
+void lkdtm_STACKLEAK_TRACK_STACK(void)
+{
+	unsigned long sp = (unsigned long)&sp;
+
+	if (!stack_is_erased())
+		return;
+
+	/*
+	 * Exhaust the thread stack with a recursion. It should hit the
+	 * BUG() in track_stack() if CONFIG_VMAP_STACK is not enabled, or
+	 * access the guard page otherwise.
+	 */
+	pr_info("try to exhaust the thread stack with the recursion...\n");
+	pr_err("FAIL: thread stack exhaustion (%lu bytes) is not detected\n",
+							sp - recursion(sp));
+}
-- 
2.7.4

[kernel-hardening] [PATCH RFC v6 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system

[Alexander Popov]

Introduce CONFIG_STACKLEAK_METRICS providing STACKLEAK information about
tasks via the /proc file system. In particular, /proc/<pid>/lowest_stack
shows the current lowest_stack value and its final value from the previous
syscall. That information can be useful for estimating the STACKLEAK
performance impact for different workloads.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 arch/Kconfig                     | 11 +++++++++++
 arch/x86/entry/entry_32.S        |  4 ++++
 arch/x86/entry/entry_64.S        |  4 ++++
 arch/x86/include/asm/processor.h |  3 +++
 arch/x86/kernel/asm-offsets.c    |  3 +++
 arch/x86/kernel/process_32.c     |  3 +++
 arch/x86/kernel/process_64.c     |  3 +++
 fs/proc/base.c                   | 14 ++++++++++++++
 8 files changed, 45 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index ba8e67b..3d8405c 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -559,6 +559,17 @@ config STACKLEAK_TRACK_MIN_SIZE
 	  this setting, don't break the poison search in erase_kstack.
 	  If unsure, leave the default value 100.
 
+config STACKLEAK_METRICS
+	bool "Show STACKLEAK metrics in the /proc file system"
+	depends on GCC_PLUGIN_STACKLEAK
+	depends on PROC_FS
+	help
+	  If this is set, STACKLEAK metrics for every task are available in
+	  the /proc file system. In particular, /proc/<pid>/lowest_stack
+	  shows the current lowest_stack value and its final value from the
+	  previous syscall. That information can be useful for estimating
+	  the STACKLEAK performance impact for your workloads.
+
 config HAVE_CC_STACKPROTECTOR
 	bool
 	help
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 8e4f815..2b76020 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -116,6 +116,10 @@ ENTRY(erase_kstack)
 	mov	%esp, %ecx
 	sub	%edi, %ecx
 
+#ifdef CONFIG_STACKLEAK_METRICS
+	mov	%edi, TASK_prev_lowest_stack(%ebp)
+#endif
+
 	cmp	$THREAD_SIZE_asm, %ecx
 	jb	3f
 	ud2
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 94f659d..32ee040 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -121,6 +121,10 @@ ENTRY(erase_kstack)
 	mov	%esp, %ecx
 	sub	%edi, %ecx
 
+#ifdef CONFIG_STACKLEAK_METRICS
+	mov	%rdi, TASK_prev_lowest_stack(%r11)
+#endif
+
 	/* Check that the counter value is sane. */
 	cmp	$THREAD_SIZE_asm, %rcx
 	jb	3f
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 520508d..c94fc2f 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -483,6 +483,9 @@ struct thread_struct {
 
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
 	unsigned long		lowest_stack;
+# ifdef CONFIG_STACKLEAK_METRICS
+	unsigned long		prev_lowest_stack;
+# endif
 #endif
 
 	unsigned int		sig_on_uaccess_err:1;
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 692c10e..84c5a29 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -43,6 +43,9 @@ void common(void) {
 # ifdef CONFIG_X86_32
 	OFFSET(TASK_thread_sp0, task_struct, thread.sp0);
 # endif
+# ifdef CONFIG_STACKLEAK_METRICS
+	OFFSET(TASK_prev_lowest_stack, task_struct, thread.prev_lowest_stack);
+# endif
 #endif
 
 	BLANK();
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 2bea3bf..5615b3d 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -139,6 +139,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
 	p->thread.lowest_stack = (unsigned long)task_stack_page(p) +
 						2 * sizeof(unsigned long);
+# ifdef CONFIG_STACKLEAK_METRICS
+	p->thread.prev_lowest_stack = p->thread.lowest_stack;
+# endif
 #endif
 
 	if (unlikely(p->flags & PF_KTHREAD)) {
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 1641463..d4a50ef 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -285,6 +285,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
 	p->thread.lowest_stack = (unsigned long)task_stack_page(p) +
 						2 * sizeof(unsigned long);
+# ifdef CONFIG_STACKLEAK_METRICS
+	p->thread.prev_lowest_stack = p->thread.lowest_stack;
+# endif
 #endif
 
 	savesegment(gs, p->thread.gsindex);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 28fa852..3569446 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m, struct pid_namespace *ns,
 }
 #endif /* CONFIG_LIVEPATCH */
 
+#ifdef CONFIG_STACKLEAK_METRICS
+static int proc_lowest_stack(struct seq_file *m, struct pid_namespace *ns,
+				struct pid *pid, struct task_struct *task)
+{
+	seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n",
+		   (void *)task->thread.prev_lowest_stack,
+		   (void *)task->thread.lowest_stack);
+	return 0;
+}
+#endif /* CONFIG_STACKLEAK_METRICS */
+
 /*
  * Thread groups
  */
@@ -2988,6 +2999,9 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_LIVEPATCH
 	ONE("patch_state",  S_IRUSR, proc_pid_patch_state),
 #endif
+#ifdef CONFIG_STACKLEAK_METRICS
+	ONE("lowest_stack", S_IRUGO, proc_lowest_stack),
+#endif
 };
 
 static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
-- 
2.7.4

[kernel-hardening] [PATCH RFC v6 6/6] doc: self-protection: Add information about STACKLEAK feature

[Alexander Popov]

Add information about STACKLEAK feature to "Stack depth overflow" and
"Memory poisoning" sections of self-protection.rst.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 Documentation/security/self-protection.rst | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/Documentation/security/self-protection.rst b/Documentation/security/self-protection.rst
index 60c8bd8..9693a90 100644
--- a/Documentation/security/self-protection.rst
+++ b/Documentation/security/self-protection.rst
@@ -165,10 +165,15 @@ Stack depth overflow
 A less well understood attack is using a bug that triggers the
 kernel to consume stack memory with deep function calls or large stack
 allocations. With this attack it is possible to write beyond the end of
-the kernel's preallocated stack space and into sensitive structures. Two
-important changes need to be made for better protections: moving the
-sensitive thread_info structure elsewhere, and adding a faulting memory
-hole at the bottom of the stack to catch these overflows.
+the kernel's preallocated stack space and into sensitive structures.
+The combination of the following measures gives better protection:
+
+* moving the sensitive thread_info structure off the stack
+  (``CONFIG_THREAD_INFO_IN_TASK``);
+* adding a faulting memory hole at the bottom of the stack to catch
+  these overflows (``CONFIG_VMAP_STACK``);
+* runtime checking that alloca() calls don't overstep the stack boundary
+  (``CONFIG_GCC_PLUGIN_STACKLEAK``).
 
 Heap memory integrity
 ---------------------
@@ -287,11 +292,11 @@ sure structure holes are cleared.
 Memory poisoning
 ----------------
 
-When releasing memory, it is best to poison the contents (clear stack on
-syscall return, wipe heap memory on a free), to avoid reuse attacks that
-rely on the old contents of memory. This frustrates many uninitialized
-variable attacks, stack content exposures, heap content exposures, and
-use-after-free attacks.
+When releasing memory, it is best to poison the contents, to avoid reuse
+attacks that rely on the old contents of memory. E.g., clear stack on a
+syscall return (``CONFIG_GCC_PLUGIN_STACKLEAK``), wipe heap memory on a
+free. This frustrates many uninitialized variable attacks, stack content
+exposures, heap content exposures, and use-after-free attacks.
 
 Destination tracking
 --------------------
-- 
2.7.4

[kernel-hardening] Re: [PATCH RFC v6 2/6] gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack

[Laura Abbott]

On 12/05/2017 03:33 PM, Alexander Popov wrote:
> +__visible int plugin_init(struct plugin_name_args *plugin_info,
> +			  struct plugin_gcc_version *version)
> +{
> +	const char * const plugin_name = plugin_info->base_name;
> +	const int argc = plugin_info->argc;
> +	const struct plugin_argument * const argv = plugin_info->argv;
> +	int i;
> +
> +	/* Extra GGC root tables describing our GTY-ed data */
> +	static const struct ggc_root_tab gt_ggc_r_gt_stackleak[] = {
> +		{
> +			.base = &track_function_decl,
> +			.nelt = 1,
> +			.stride = sizeof(track_function_decl),
> +			.cb = &gt_ggc_mx_tree_node,
> +			.pchw = &gt_pch_nx_tree_node
> +		},
> +		{
> +			.base = &check_function_decl,
> +			.nelt = 1,
> +			.stride = sizeof(check_function_decl),
> +			.cb = &gt_ggc_mx_tree_node,
> +			.pchw = &gt_pch_nx_tree_node
> +		},
> +		LAST_GGC_ROOT_TAB
> +	};
> +
> +	/*
> +	 * The stackleak_tree_instrument pass should be executed before the
> +	 * "optimized" pass, which is the control flow graph cleanup that is
> +	 * performed just before expanding gcc trees to the RTL. In former
> +	 * versions of the plugin this new pass was inserted before the
> +	 * "tree_profile" pass, which is currently called "profile".
> +	 */
> +	PASS_INFO(stackleak_tree_instrument, "optimized", 1,
> +						PASS_POS_INSERT_BEFORE);
> +
> +	/*
> +	 * The stackleak_final pass should be executed before the "final" pass,
> +	 * which turns the RTL (Register Transfer Language) into assembly.
> +	 */
> +	PASS_INFO(stackleak_final, "final", 1, PASS_POS_INSERT_BEFORE);
> +
> +	if (!plugin_default_version_check(version, &gcc_version)) {
> +		error(G_("incompatible gcc/plugin versions"));
> +		return 1;
> +	}
> +
> +	/* Parse the plugin arguments */
> +	if (argc != 1) {
> +		error(G_("bad number of the plugin arguments: %d"), argc);
> +		return 1;
> +	}
> +
> +	if (strcmp(argv[i].key, "track-min-size")) {
> +		error(G_("unknown option '-fplugin-arg-%s-%s'"),
> +				plugin_name, argv[i].key);
> +		return 1;
> +	}
> +
> +	if (!argv[i].value) {
> +		error(G_("no value supplied for option '-fplugin-arg-%s-%s'"),
> +				plugin_name, argv[i].key);
> +		return 1;
> +	}
> +
> +	track_frame_size = atoi(argv[i].value);
> +	if (track_frame_size < 0) {
> +		error(G_("invalid option argument '-fplugin-arg-%s-%s=%s'"),
> +				plugin_name, argv[i].key, argv[i].value);
> +		return 1;
> +	}

I don't see i getting updated anywhere, which seems to be an artifact of
removing the for loop. I'd prefer if you just kept the loop since the
arm64 version requires a --disable option like structleak.

Thanks,
Laura

[kernel-hardening] Re: [PATCH RFC v6 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system

[Laura Abbott]

On 12/05/2017 03:33 PM, Alexander Popov wrote:
> Introduce CONFIG_STACKLEAK_METRICS providing STACKLEAK information about
> tasks via the /proc file system. In particular, /proc/<pid>/lowest_stack
> shows the current lowest_stack value and its final value from the previous
> syscall. That information can be useful for estimating the STACKLEAK
> performance impact for different workloads.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> ---
>   arch/Kconfig                     | 11 +++++++++++
>   arch/x86/entry/entry_32.S        |  4 ++++
>   arch/x86/entry/entry_64.S        |  4 ++++
>   arch/x86/include/asm/processor.h |  3 +++
>   arch/x86/kernel/asm-offsets.c    |  3 +++
>   arch/x86/kernel/process_32.c     |  3 +++
>   arch/x86/kernel/process_64.c     |  3 +++
>   fs/proc/base.c                   | 14 ++++++++++++++
>   8 files changed, 45 insertions(+)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index ba8e67b..3d8405c 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -559,6 +559,17 @@ config STACKLEAK_TRACK_MIN_SIZE
>   	  this setting, don't break the poison search in erase_kstack.
>   	  If unsure, leave the default value 100.
>   
> +config STACKLEAK_METRICS
> +	bool "Show STACKLEAK metrics in the /proc file system"
> +	depends on GCC_PLUGIN_STACKLEAK
> +	depends on PROC_FS
> +	help
> +	  If this is set, STACKLEAK metrics for every task are available in
> +	  the /proc file system. In particular, /proc/<pid>/lowest_stack
> +	  shows the current lowest_stack value and its final value from the
> +	  previous syscall. That information can be useful for estimating
> +	  the STACKLEAK performance impact for your workloads.
> +
>   config HAVE_CC_STACKPROTECTOR
>   	bool
>   	help
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 8e4f815..2b76020 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -116,6 +116,10 @@ ENTRY(erase_kstack)
>   	mov	%esp, %ecx
>   	sub	%edi, %ecx
>   
> +#ifdef CONFIG_STACKLEAK_METRICS
> +	mov	%edi, TASK_prev_lowest_stack(%ebp)
> +#endif
> +
>   	cmp	$THREAD_SIZE_asm, %ecx
>   	jb	3f
>   	ud2
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 94f659d..32ee040 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -121,6 +121,10 @@ ENTRY(erase_kstack)
>   	mov	%esp, %ecx
>   	sub	%edi, %ecx
>   
> +#ifdef CONFIG_STACKLEAK_METRICS
> +	mov	%rdi, TASK_prev_lowest_stack(%r11)
> +#endif
> +
>   	/* Check that the counter value is sane. */
>   	cmp	$THREAD_SIZE_asm, %rcx
>   	jb	3f
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 520508d..c94fc2f 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -483,6 +483,9 @@ struct thread_struct {
>   
>   #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>   	unsigned long		lowest_stack;
> +# ifdef CONFIG_STACKLEAK_METRICS
> +	unsigned long		prev_lowest_stack;
> +# endif
>   #endif
>   
>   	unsigned int		sig_on_uaccess_err:1;
> diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
> index 692c10e..84c5a29 100644
> --- a/arch/x86/kernel/asm-offsets.c
> +++ b/arch/x86/kernel/asm-offsets.c
> @@ -43,6 +43,9 @@ void common(void) {
>   # ifdef CONFIG_X86_32
>   	OFFSET(TASK_thread_sp0, task_struct, thread.sp0);
>   # endif
> +# ifdef CONFIG_STACKLEAK_METRICS
> +	OFFSET(TASK_prev_lowest_stack, task_struct, thread.prev_lowest_stack);
> +# endif
>   #endif
>   
>   	BLANK();
> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
> index 2bea3bf..5615b3d 100644
> --- a/arch/x86/kernel/process_32.c
> +++ b/arch/x86/kernel/process_32.c
> @@ -139,6 +139,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
>   #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>   	p->thread.lowest_stack = (unsigned long)task_stack_page(p) +
>   						2 * sizeof(unsigned long);
> +# ifdef CONFIG_STACKLEAK_METRICS
> +	p->thread.prev_lowest_stack = p->thread.lowest_stack;
> +# endif
>   #endif
>   
>   	if (unlikely(p->flags & PF_KTHREAD)) {
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 1641463..d4a50ef 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -285,6 +285,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
>   #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>   	p->thread.lowest_stack = (unsigned long)task_stack_page(p) +
>   						2 * sizeof(unsigned long);
> +# ifdef CONFIG_STACKLEAK_METRICS
> +	p->thread.prev_lowest_stack = p->thread.lowest_stack;
> +# endif
>   #endif
>   
>   	savesegment(gs, p->thread.gsindex);
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 28fa852..3569446 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m, struct pid_namespace *ns,
>   }
>   #endif /* CONFIG_LIVEPATCH */
>   
> +#ifdef CONFIG_STACKLEAK_METRICS
> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace *ns,
> +				struct pid *pid, struct task_struct *task)
> +{
> +	seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n",
> +		   (void *)task->thread.prev_lowest_stack,
> +		   (void *)task->thread.lowest_stack);
> +	return 0;
> +}
> +#endif /* CONFIG_STACKLEAK_METRICS */
> +

This just prints the hashed value with the new pointer leak work.
I don't think we want to print the fully exposed value via %px so
it's not clear how valuable this proc file is now.

Thanks,
Laura

>   /*
>    * Thread groups
>    */
> @@ -2988,6 +2999,9 @@ static const struct pid_entry tgid_base_stuff[] = {
>   #ifdef CONFIG_LIVEPATCH
>   	ONE("patch_state",  S_IRUSR, proc_pid_patch_state),
>   #endif
> +#ifdef CONFIG_STACKLEAK_METRICS
> +	ONE("lowest_stack", S_IRUGO, proc_lowest_stack),
> +#endif
>   };
>   
>   static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
> 

[kernel-hardening] Re: [PATCH RFC v6 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system

[Kees Cook]

On Wed, Dec 6, 2017 at 11:22 AM, Laura Abbott <labbott@redhat.com> wrote:
> On 12/05/2017 03:33 PM, Alexander Popov wrote:
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 28fa852..3569446 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m,
>> struct pid_namespace *ns,
>>   }
>>   #endif /* CONFIG_LIVEPATCH */
>>   +#ifdef CONFIG_STACKLEAK_METRICS
>> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace
>> *ns,
>> +                               struct pid *pid, struct task_struct *task)
>> +{
>> +       seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n",
>> +                  (void *)task->thread.prev_lowest_stack,
>> +                  (void *)task->thread.lowest_stack);
>> +       return 0;
>> +}
>> +#endif /* CONFIG_STACKLEAK_METRICS */
>> +
>
>
> This just prints the hashed value with the new pointer leak work.
> I don't think we want to print the fully exposed value via %px so
> it's not clear how valuable this proc file is now.

Maybe print the size, not the location?

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] [PATCH RFC v6 3/6] x86/entry: Erase kernel stack in syscall_trace_enter()

[Dmitry V. Levin]

Hi,

On Wed, Dec 06, 2017 at 02:33:44AM +0300, Alexander Popov wrote:
> Make STACKLEAK erase kernel stack after ptrace/seccomp/auditing
> not to leave any sensitive information on the stack for the syscall code.
> 
> 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@linux.com>
> ---
>  arch/x86/entry/common.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index d7d3cc2..d45b7cf 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -45,6 +45,12 @@ __visible inline void enter_from_user_mode(void)
>  static inline void enter_from_user_mode(void) {}
>  #endif
>  
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +asmlinkage void erase_kstack(void);
> +#else
> +static void erase_kstack(void) {}
> +#endif
> +
>  static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
>  {
>  #ifdef CONFIG_X86_64
> @@ -81,11 +87,15 @@ static long syscall_trace_enter(struct pt_regs *regs)
>  		emulated = true;
>  
>  	if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
> -	    tracehook_report_syscall_entry(regs))
> +	    tracehook_report_syscall_entry(regs)) {
> +		erase_kstack();
>  		return -1L;
> +	}
>  
> -	if (emulated)
> +	if (emulated) {
> +		erase_kstack();
>  		return -1L;
> +	}
>  
>  #ifdef CONFIG_SECCOMP
>  	/*
> @@ -117,8 +127,10 @@ static long syscall_trace_enter(struct pt_regs *regs)
>  		}
>  
>  		ret = __secure_computing(&sd);
> -		if (ret == -1)
> +		if (ret == -1) {
> +			erase_kstack();
>  			return ret;
> +		}
>  	}
>  #endif
>  
> @@ -127,6 +139,7 @@ static long syscall_trace_enter(struct pt_regs *regs)
>  
>  	do_audit_syscall_entry(regs, arch);
>  
> +	erase_kstack();
>  	return ret ?: regs->orig_ax;
>  }

wrt adding erase_kstack() calls to syscall_trace_enter(), I think the only
case where this would be appropriate is that still has a chance of
executing syscall code.  In all cases where syscall_trace_enter() returns
-1 no syscall code is going to be executed and the stack will be erased on
exiting syscall anyway.

In other words, only the last hunk of this patch seems to be useful,
all others look redundant.

P.S.  I've trimmed the Cc list to those who took part in earlier rounds
of this discussion.


-- 
ldv

[kernel-hardening] Re: [PATCH RFC v6 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system

[Laura Abbott]

On 12/06/2017 12:40 PM, Kees Cook wrote:
> On Wed, Dec 6, 2017 at 11:22 AM, Laura Abbott <labbott@redhat.com> wrote:
>> On 12/05/2017 03:33 PM, Alexander Popov wrote:
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index 28fa852..3569446 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m,
>>> struct pid_namespace *ns,
>>>    }
>>>    #endif /* CONFIG_LIVEPATCH */
>>>    +#ifdef CONFIG_STACKLEAK_METRICS
>>> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace
>>> *ns,
>>> +                               struct pid *pid, struct task_struct *task)
>>> +{
>>> +       seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n",
>>> +                  (void *)task->thread.prev_lowest_stack,
>>> +                  (void *)task->thread.lowest_stack);
>>> +       return 0;
>>> +}
>>> +#endif /* CONFIG_STACKLEAK_METRICS */
>>> +
>>
>>
>> This just prints the hashed value with the new pointer leak work.
>> I don't think we want to print the fully exposed value via %px so
>> it's not clear how valuable this proc file is now.
> 
> Maybe print the size, not the location?
> 
> -Kees
> 
Hmmmmm, that starts to overlap with CONFIG_DEBUG_STACK_USAGE.
That's not a bad thing but it would be good to clarify what
this is tracking vs. CONFIG_DEBUG_STACK_USAGE.

Thanks,
Laura

[kernel-hardening] Re: [PATCH RFC v6 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system

[Alexander Popov]

Hello Laura and Kees,

[adding Tobin C. Harding]

On 06.12.2017 22:22, Laura Abbott wrote:
> On 12/05/2017 03:33 PM, Alexander Popov wrote:
>> Introduce CONFIG_STACKLEAK_METRICS providing STACKLEAK information about
>> tasks via the /proc file system. In particular, /proc/<pid>/lowest_stack
>> shows the current lowest_stack value and its final value from the previous
>> syscall. That information can be useful for estimating the STACKLEAK
>> performance impact for different workloads.
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
>> ---
>>   arch/Kconfig                     | 11 +++++++++++
>>   arch/x86/entry/entry_32.S        |  4 ++++
>>   arch/x86/entry/entry_64.S        |  4 ++++
>>   arch/x86/include/asm/processor.h |  3 +++
>>   arch/x86/kernel/asm-offsets.c    |  3 +++
>>   arch/x86/kernel/process_32.c     |  3 +++
>>   arch/x86/kernel/process_64.c     |  3 +++
>>   fs/proc/base.c                   | 14 ++++++++++++++
>>   8 files changed, 45 insertions(+)

[...]

>> +#ifdef CONFIG_STACKLEAK_METRICS
>> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace *ns,
>> +				struct pid *pid, struct task_struct *task)
>> +{
>> +	seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n",
>> +		   (void *)task->thread.prev_lowest_stack,
>> +		   (void *)task->thread.lowest_stack);
>> +	return 0;
>> +}
>> +#endif /* CONFIG_STACKLEAK_METRICS */
>> +
> 
> This just prints the hashed value with the new pointer leak work.
> I don't think we want to print the fully exposed value via %px so
> it's not clear how valuable this proc file is now.

Yes, I tested that before sending the patch. I was confused when I saw the
hashed values. But setting kptr_restrict to 1 fixed that for me:

root@hobbit:~# cat /proc/2627/lowest_stack
prev_lowest_stack: 00000000ed8ca991
lowest_stack: 0000000040579d76
root@hobbit:~# echo 1 > /proc/sys/kernel/kptr_restrict
root@hobbit:~# cat /proc/2627/lowest_stack
prev_lowest_stack: ffffc9000094fdb8
lowest_stack: ffffc9000094f9e0

However, Documentation/printk-formats.txt and Documentation/sysctl/kernel.txt
don't specify that behaviour.

Best regards,
Alexander

[kernel-hardening] Re: [PATCH RFC v6 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system

[Tobin C. Harding]

On Thu, Dec 07, 2017 at 10:09:27AM +0300, Alexander Popov wrote:
> Hello Laura and Kees,
> 
> [adding Tobin C. Harding]
> 
> On 06.12.2017 22:22, Laura Abbott wrote:
> > On 12/05/2017 03:33 PM, Alexander Popov wrote:
> >> Introduce CONFIG_STACKLEAK_METRICS providing STACKLEAK information about
> >> tasks via the /proc file system. In particular, /proc/<pid>/lowest_stack
> >> shows the current lowest_stack value and its final value from the previous
> >> syscall. That information can be useful for estimating the STACKLEAK
> >> performance impact for different workloads.
> >>
> >> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> >> ---
> >>   arch/Kconfig                     | 11 +++++++++++
> >>   arch/x86/entry/entry_32.S        |  4 ++++
> >>   arch/x86/entry/entry_64.S        |  4 ++++
> >>   arch/x86/include/asm/processor.h |  3 +++
> >>   arch/x86/kernel/asm-offsets.c    |  3 +++
> >>   arch/x86/kernel/process_32.c     |  3 +++
> >>   arch/x86/kernel/process_64.c     |  3 +++
> >>   fs/proc/base.c                   | 14 ++++++++++++++
> >>   8 files changed, 45 insertions(+)
> 
> [...]
> 
> >> +#ifdef CONFIG_STACKLEAK_METRICS
> >> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace *ns,
> >> +				struct pid *pid, struct task_struct *task)
> >> +{
> >> +	seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n",
> >> +		   (void *)task->thread.prev_lowest_stack,
> >> +		   (void *)task->thread.lowest_stack);
> >> +	return 0;
> >> +}
> >> +#endif /* CONFIG_STACKLEAK_METRICS */
> >> +
> > 
> > This just prints the hashed value with the new pointer leak work.
> > I don't think we want to print the fully exposed value via %px so
> > it's not clear how valuable this proc file is now.
> 
> Yes, I tested that before sending the patch. I was confused when I saw the
> hashed values. But setting kptr_restrict to 1 fixed that for me:
> 
> root@hobbit:~# cat /proc/2627/lowest_stack
> prev_lowest_stack: 00000000ed8ca991
> lowest_stack: 0000000040579d76
> root@hobbit:~# echo 1 > /proc/sys/kernel/kptr_restrict
> root@hobbit:~# cat /proc/2627/lowest_stack
> prev_lowest_stack: ffffc9000094fdb8
> lowest_stack: ffffc9000094f9e0
> 
> However, Documentation/printk-formats.txt and Documentation/sysctl/kernel.txt
> don't specify that behaviour.

thanks for pointing this out. Yes, %pK hashes the address now when
kptr_restrict==0

And yes, I forgot to update the docs :( will amend.

thanks,
Tobin.

[kernel-hardening] Re: [PATCH RFC v6 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system

[Alexander Popov]

On 07.12.2017 02:06, Laura Abbott wrote:
> On 12/06/2017 12:40 PM, Kees Cook wrote:
>> On Wed, Dec 6, 2017 at 11:22 AM, Laura Abbott <labbott@redhat.com> wrote:
>>> On 12/05/2017 03:33 PM, Alexander Popov wrote:
>>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>>> index 28fa852..3569446 100644
>>>> --- a/fs/proc/base.c
>>>> +++ b/fs/proc/base.c
>>>> @@ -2884,6 +2884,17 @@ static int proc_pid_patch_state(struct seq_file *m,
>>>> struct pid_namespace *ns,
>>>>    }
>>>>    #endif /* CONFIG_LIVEPATCH */
>>>>    +#ifdef CONFIG_STACKLEAK_METRICS
>>>> +static int proc_lowest_stack(struct seq_file *m, struct pid_namespace
>>>> *ns,
>>>> +                               struct pid *pid, struct task_struct *task)
>>>> +{
>>>> +       seq_printf(m, "prev_lowest_stack: %pK\nlowest_stack: %pK\n",
>>>> +                  (void *)task->thread.prev_lowest_stack,
>>>> +                  (void *)task->thread.lowest_stack);
>>>> +       return 0;
>>>> +}
>>>> +#endif /* CONFIG_STACKLEAK_METRICS */
>>>> +
>>>
>>> This just prints the hashed value with the new pointer leak work.
>>> I don't think we want to print the fprev_lowest_stackully exposed value via %px so
>>> it's not clear how valuable this proc file is now.
>>
>> Maybe print the size, not the location?

Yes, I think I can print: THREAD_SIZE - (addr & (THREAD_SIZE - 1)).
I can call it "stack_depth", do you like it?

N.B. this value is not a really precise stack depth, because:
 - we don't instrument all kernel functions, so a lot of them don't update the
lowest_stack value;
 - prev_lowest_stack is a final point of the poison search in erase_kstack(), it
is not an actual stack depth.

Or should I dwell on the current version and rely on non-zero kptr_restrict?

> Hmmmmm, that starts to overlap with CONFIG_DEBUG_STACK_USAGE.
> That's not a bad thing but it would be good to clarify what
> this is tracking vs. CONFIG_DEBUG_STACK_USAGE.

Thanks, Laura, I didn't know about CONFIG_DEBUG_STACK_USAGE. After testing it I
think that it should remain independent, because:
 - it works on sysrq;
 - it dumps information about all tasks in the system at once;
 - it provides precise information (in contrast to my metrics).

In addition, I guess, modifying sysrq output format might break the workflow of
users, who parse it (but I'm not sure).

Best regards,
Alexander

[kernel-hardening] Re: [PATCH RFC v6 2/6] gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack

[Alexander Popov]

On 06.12.2017 21:57, Laura Abbott wrote:
> On 12/05/2017 03:33 PM, Alexander Popov wrote:
>> +	/* Parse the plugin arguments */
>> +	if (argc != 1) {
>> +		error(G_("bad number of the plugin arguments: %d"), argc);
>> +		return 1;
>> +	}
>> +
>> +	if (strcmp(argv[i].key, "track-min-size")) {
>> +		error(G_("unknown option '-fplugin-arg-%s-%s'"),
>> +				plugin_name, argv[i].key);
>> +		return 1;
>> +	}
>> +
>> +	if (!argv[i].value) {
>> +		error(G_("no value supplied for option '-fplugin-arg-%s-%s'"),
>> +				plugin_name, argv[i].key);
>> +		return 1;
>> +	}
>> +
>> +	track_frame_size = atoi(argv[i].value);
>> +	if (track_frame_size < 0) {
>> +		error(G_("invalid option argument '-fplugin-arg-%s-%s=%s'"),
>> +				plugin_name, argv[i].key, argv[i].value);
>> +		return 1;
>> +	}
> 
> I don't see i getting updated anywhere, which seems to be an artifact of
> removing the for loop. I'd prefer if you just kept the loop since the
> arm64 version requires a --disable option like structleak.

Ok, no problem, I'll return the loop and add --disable option in v7.

Best regards,
Alexander

[kernel-hardening] Re: [PATCH RFC v6 1/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls

[Peter Zijlstra]

On Wed, Dec 06, 2017 at 02:33:42AM +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 architecture-specific code filling the used
> part of the kernel stack with a poison value before returning to the
> userspace. Full STACKLEAK feature also contains the gcc plugin which
> comes in a separate commit.

Have you looked at the entry rework in this series:

  https://lkml.kernel.org/r/20171204140706.296109558@linutronix.de

[kernel-hardening] Re: [PATCH RFC v6 1/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls

[Alexander Popov]

Hello Peter,

On 08.12.2017 14:44, Peter Zijlstra wrote:
> On Wed, Dec 06, 2017 at 02:33:42AM +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 architecture-specific code filling the used
>> part of the kernel stack with a poison value before returning to the
>> userspace. Full STACKLEAK feature also contains the gcc plugin which
>> comes in a separate commit.
> 
> Have you looked at the entry rework in this series:
> 
>   https://lkml.kernel.org/r/20171204140706.296109558@linutronix.de

Thanks for the link. I briefly looked through WIP.x86/pti branch. Should I
rebase STACKLEAK patch series onto it?

Although I don't fully understand some of the commits, I can suppose that
STACKLEAK stack erasing must be modified because of this trampoline stack
introduction:
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=WIP.x86/pti&id=813b4125a835f2eb9aa6fb08dac0bc8eeadd69a1

Am I right? Are there other changes which I should consider?

May I also ask for your feedback on this version of the STACKLEAK patch series?
Thanks!

Best regards,
Alexander

[kernel-hardening] Re: [PATCH RFC v6 1/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls

[Peter Zijlstra]

On Sat, Dec 09, 2017 at 12:54:21AM +0300, Alexander Popov wrote:
> Hello Peter,
> 
> On 08.12.2017 14:44, Peter Zijlstra wrote:
> > On Wed, Dec 06, 2017 at 02:33:42AM +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 architecture-specific code filling the used
> >> part of the kernel stack with a poison value before returning to the
> >> userspace. Full STACKLEAK feature also contains the gcc plugin which
> >> comes in a separate commit.
> > 
> > Have you looked at the entry rework in this series:
> > 
> >   https://lkml.kernel.org/r/20171204140706.296109558@linutronix.de
> 
> Thanks for the link. I briefly looked through WIP.x86/pti branch. Should I
> rebase STACKLEAK patch series onto it?

Probably a good idea; the tail end of that series is still somewhat in
flux but the entry rework is fairly stable at this point.

> Although I don't fully understand some of the commits, I can suppose that
> STACKLEAK stack erasing must be modified because of this trampoline stack
> introduction:
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=WIP.x86/pti&id=813b4125a835f2eb9aa6fb08dac0bc8eeadd69a1
> 
> Am I right? Are there other changes which I should consider?

You're right; the trampoline stack is what I was thinking of. You can
run the erase thing when we're on the trampoline back out.

> May I also ask for your feedback on this version of the STACKLEAK patch series?

I meant to have a look, but have not yet found the time for it, its on
the todo list.

Thanks!

Re: [kernel-hardening] [PATCH RFC v6 3/6] x86/entry: Erase kernel stack in syscall_trace_enter()

[Alexander Popov]

On 07.12.2017 00:12, Dmitry V. Levin wrote:
> On Wed, Dec 06, 2017 at 02:33:44AM +0300, Alexander Popov wrote:
>> Make STACKLEAK erase kernel stack after ptrace/seccomp/auditing
>> not to leave any sensitive information on the stack for the syscall code.
>>
>> 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@linux.com>
>> ---
>>  arch/x86/entry/common.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>> index d7d3cc2..d45b7cf 100644
>> --- a/arch/x86/entry/common.c
>> +++ b/arch/x86/entry/common.c
>> @@ -45,6 +45,12 @@ __visible inline void enter_from_user_mode(void)
>>  static inline void enter_from_user_mode(void) {}
>>  #endif
>>  
>> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>> +asmlinkage void erase_kstack(void);
>> +#else
>> +static void erase_kstack(void) {}
>> +#endif
>> +
>>  static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
>>  {
>>  #ifdef CONFIG_X86_64
>> @@ -81,11 +87,15 @@ static long syscall_trace_enter(struct pt_regs *regs)
>>  		emulated = true;
>>  
>>  	if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
>> -	    tracehook_report_syscall_entry(regs))
>> +	    tracehook_report_syscall_entry(regs)) {
>> +		erase_kstack();
>>  		return -1L;
>> +	}
>>  
>> -	if (emulated)
>> +	if (emulated) {
>> +		erase_kstack();
>>  		return -1L;
>> +	}
>>  
>>  #ifdef CONFIG_SECCOMP
>>  	/*
>> @@ -117,8 +127,10 @@ static long syscall_trace_enter(struct pt_regs *regs)
>>  		}
>>  
>>  		ret = __secure_computing(&sd);
>> -		if (ret == -1)
>> +		if (ret == -1) {
>> +			erase_kstack();
>>  			return ret;
>> +		}
>>  	}
>>  #endif
>>  
>> @@ -127,6 +139,7 @@ static long syscall_trace_enter(struct pt_regs *regs)
>>  
>>  	do_audit_syscall_entry(regs, arch);
>>  
>> +	erase_kstack();
>>  	return ret ?: regs->orig_ax;
>>  }

Hello Dmitry,

Thanks for the review.

> wrt adding erase_kstack() calls to syscall_trace_enter(), I think the only
> case where this would be appropriate is that still has a chance of
> executing syscall code.  In all cases where syscall_trace_enter() returns
> -1 no syscall code is going to be executed and the stack will be erased on
> exiting syscall anyway.
> 
> In other words, only the last hunk of this patch seems to be useful,
> all others look redundant.

I agree with your point. I'll fix it in v7.

> P.S.  I've trimmed the Cc list to those who took part in earlier rounds
> of this discussion.

Excuse me, I've returned everybody back to CC list again :)

Best regards,
Alexander

Re: [kernel-hardening] [PATCH RFC v6 2/6] gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack

[Dmitry V. Levin]

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@linux.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.

>  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.


-- 
ldv

Re: [kernel-hardening] [PATCH RFC v6 2/6] gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack

[Alexander Popov]

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@linux.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