lkdtm: Test VMAP_STACK allocates leading/trailing guard pages

Two new tests STACK_GUARD_PAGE_LEADING and STACK_GUARD_PAGE_TRAILING attempt to read the byte before and after, respectively, of the current stack frame, which should fault under VMAP_STACK.

Signed-off-by Kees Cook <[email protected]>

---
Do these tests both trip with the new arm64 VMAP_STACK code?

---
 drivers/misc/lkdtm.h      |  2 ++
 drivers/misc/lkdtm_bugs.c | 30 ++++++++++++++++++++++++++++++
 drivers/misc/lkdtm_core.c |  2 ++
 3 files changed, 34 insertions(+)

drivers/misc/lkdtm.h +2 -0

index 063f5d651076..3c8627ca5f42 100644
@@ -22,6 +22,8 @@ void lkdtm_HUNG_TASK(void);
 void lkdtm_CORRUPT_LIST_ADD(void);
 void lkdtm_CORRUPT_LIST_DEL(void);
 void lkdtm_CORRUPT_USER_DS(void);
+void lkdtm_STACK_GUARD_PAGE_LEADING(void);
+void lkdtm_STACK_GUARD_PAGE_TRAILING(void);
 
 /* lkdtm_heap.c */
 void lkdtm_OVERWRITE_ALLOCATION(void);

drivers/misc/lkdtm_bugs.c +30 -0

index ef3d06f901fc..041fe6e9532a 100644
@@ -8,6 +8,7 @@
 #include <linux/list.h>
 #include <linux/sched.h>
 #include <linux/sched/signal.h>
+#include <linux/sched/task_stack.h>
 #include <linux/uaccess.h>
 
 struct lkdtm_list {

@@ -199,6 +200,7 @@ void lkdtm_CORRUPT_LIST_DEL(void)
                pr_err("list_del() corruption not detected!\n");
 }
 
+/* Test if unbalanced set_fs(KERNEL_DS)/set_fs(USER_DS) check exists. */
 void lkdtm_CORRUPT_USER_DS(void)
 {
        pr_info("setting bad task size limit\n");

@@ -207,3 +209,31 @@ void lkdtm_CORRUPT_USER_DS(void)
        /* Make sure we do not keep running with a KERNEL_DS! */
        force_sig(SIGKILL, current);
 }
+
+/* Test that VMAP_STACK is actually allocating with a leading guard page */
+void lkdtm_STACK_GUARD_PAGE_LEADING(void)
+{
+       const unsigned char *stack = task_stack_page(current);
+       const unsigned char *ptr = stack - 1;
+       volatile unsigned char byte;
+
+       pr_info("attempting bad read from page below current stack\n");
+
+       byte = *ptr;
+
+       pr_err("FAIL: accessed page before stack!\n");
+}
+
+/* Test that VMAP_STACK is actually allocating with a trailing guard page */
+void lkdtm_STACK_GUARD_PAGE_TRAILING(void)
+{
+       const unsigned char *stack = task_stack_page(current);
+       const unsigned char *ptr = stack + THREAD_SIZE;
+       volatile unsigned char byte;
+
+       pr_info("attempting bad read from page above current stack\n");
+
+       byte = *ptr;
+
+       pr_err("FAIL: accessed page after stack!\n");
+}

drivers/misc/lkdtm_core.c +2 -1

index 51decc07eeda..9e98d7ef5503 100644
@@ -201,6 +201,8 @@ struct crashtype crashtypes[] = {
        CRASHTYPE(CORRUPT_LIST_DEL),
        CRASHTYPE(CORRUPT_USER_DS),
        CRASHTYPE(CORRUPT_STACK),
+       CRASHTYPE(STACK_GUARD_PAGE_LEADING),
+       CRASHTYPE(STACK_GUARD_PAGE_TRAILING),
        CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE),
        CRASHTYPE(OVERWRITE_ALLOCATION),
        CRASHTYPE(WRITE_AFTER_FREE),
--

2.7.4

--
Kees Cook
Pixel Security

Quoted 7 lines · Kees Cook

Two new tests STACK_GUARD_PAGE_LEADING and STACK_GUARD_PAGE_TRAILING attempt to read the byte before and after, respectively, of the current stack frame, which should fault under VMAP_STACK.

Signed-off-by Kees Cook <[email protected]>

---
Do these tests both trip with the new arm64 VMAP_STACK code?

Probably not. On arm64, the registers are stacked by software at exception entry, and so we decrement the sp first by the size of the register file, and if the resulting value overflows the stack, the situation is handled as if the exception was caused by a faulting stack access while it may be caused by something else in reality. Since the act of handling the exception is guaranteed to overflow the stack anyway, this does not really make a huge difference, and it prevents the recursive fault from wiping the context that we need to produce the diagnostics.

This means an illegal access right above the stack will go undetected.

Quoted 19 lines · Ard Biesheuvel

Quoted 7 lines · Kees Cook

Two new tests STACK_GUARD_PAGE_LEADING and STACK_GUARD_PAGE_TRAILING attempt to read the byte before and after, respectively, of the current stack frame, which should fault under VMAP_STACK.

Signed-off-by Kees Cook <[email protected]>

---
Do these tests both trip with the new arm64 VMAP_STACK code?

Probably not. On arm64, the registers are stacked by software at exception entry, and so we decrement the sp first by the size of the register file, and if the resulting value overflows the stack, the situation is handled as if the exception was caused by a faulting stack access while it may be caused by something else in reality. Since the act of handling the exception is guaranteed to overflow the stack anyway, this does not really make a huge difference, and it prevents the recursive fault from wiping the context that we need to produce the diagnostics.

Quoted 1 line

This means an illegal access right above the stack will go undetected.

I thought vmap entries provided guard pages around allocations? Shouldn't that catch it?

Quoted 23 lines · Kees Cook

Quoted 20 lines · Ard Biesheuvel

Quoted 7 lines · Kees Cook

Two new tests STACK_GUARD_PAGE_LEADING and STACK_GUARD_PAGE_TRAILING attempt to read the byte before and after, respectively, of the current stack frame, which should fault under VMAP_STACK.

Signed-off-by Kees Cook <[email protected]>

---
Do these tests both trip with the new arm64 VMAP_STACK code?

Probably not. On arm64, the registers are stacked by software at exception entry, and so we decrement the sp first by the size of the register file, and if the resulting value overflows the stack, the situation is handled as if the exception was caused by a faulting stack access while it may be caused by something else in reality. Since the act of handling the exception is guaranteed to overflow the stack anyway, this does not really make a huge difference, and it prevents the recursive fault from wiping the context that we need to produce the diagnostics.

This means an illegal access right above the stack will go undetected.

Quoted 3 lines

I thought vmap entries provided guard pages around allocations? Shouldn't that catch it?

Ah yes, so we will fault. We should probably double check whether we will not misidentify the fault because of the subtraction we do first, but that should be trivial to add.

Two new tests STACK_GUARD_PAGE_LEADING and STACK_GUARD_PAGE_TRAILING attempt to read the byte before and after, respectively, of the current stack frame, which should fault under VMAP_STACK.

Signed-off-by Kees Cook <[email protected]>

---
Do these tests both trip with the new arm64 VMAP_STACK code?

drivers/misc/lkdtm_bugs.c +17 -0

+/* Test that VMAP_STACK is actually allocating with a leading guard page */
+void lkdtm_STACK_GUARD_PAGE_LEADING(void)
+{
+	const unsigned char *stack = task_stack_page(current);
+	const unsigned char *ptr = stack - 1;
+	volatile unsigned char byte;
+
+	pr_info("attempting bad read from page below current stack\n");
+
+	byte = *ptr;
+
+	pr_err("FAIL: accessed page before stack!\n");
+}
+
+/* Test that VMAP_STACK is actually allocating with a trailing guard page */
+void lkdtm_STACK_GUARD_PAGE_TRAILING(void)
+{

Hi,

Quoted 25 lines · Kees Cook

Two new tests STACK_GUARD_PAGE_LEADING and STACK_GUARD_PAGE_TRAILING attempt to read the byte before and after, respectively, of the current stack frame, which should fault under VMAP_STACK.

Signed-off-by Kees Cook <[email protected]>

---
Do these tests both trip with the new arm64 VMAP_STACK code?

drivers/misc/lkdtm_bugs.c +17 -0

+/* Test that VMAP_STACK is actually allocating with a leading guard page */
+void lkdtm_STACK_GUARD_PAGE_LEADING(void)
+{
+	const unsigned char *stack = task_stack_page(current);
+	const unsigned char *ptr = stack - 1;
+	volatile unsigned char byte;
+
+	pr_info("attempting bad read from page below current stack\n");
+
+	byte = *ptr;
+
+	pr_err("FAIL: accessed page before stack!\n");
+}
+
+/* Test that VMAP_STACK is actually allocating with a trailing guard page */
+void lkdtm_STACK_GUARD_PAGE_TRAILING(void)
+{

drivers/misc/lkdtm_bugs.c void lkdtm_CORRUPT_USER_DS(void) · line 239

Quoted 10 lines

drivers/misc/lkdtm_bugs.c +10 -0

+	const unsigned char *stack = task_stack_page(current);
+	const unsigned char *ptr = stack + THREAD_SIZE;
+	volatile unsigned char byte;
+
+	pr_info("attempting bad read from page above current stack\n");
+
+	byte = *ptr;
+
+	pr_err("FAIL: accessed page after stack!\n");
+}

I can give these a go tomorrow.

These *should* fault, and IIUC should trigger the usual "Unable to handle kernel %s at virtual address %08lx\n" splat from arm64's __do_kernel_fault(), which should end up with an Oops().

Since these don't mess with the SP, they shouldn't trigger the overflow detection, which detects whether we have sufficient stack space to store the exception context to the stack. That caught the LKDTM overflow test reliably.

Thanks,
Mark.

Quoted 45 lines · Mark Rutland

Hi,

Quoted 25 lines · Kees Cook

Two new tests STACK_GUARD_PAGE_LEADING and STACK_GUARD_PAGE_TRAILING attempt to read the byte before and after, respectively, of the current stack frame, which should fault under VMAP_STACK.

Signed-off-by Kees Cook <[email protected]>

---
Do these tests both trip with the new arm64 VMAP_STACK code?

drivers/misc/lkdtm_bugs.c +17 -0

+/* Test that VMAP_STACK is actually allocating with a leading guard page */
+void lkdtm_STACK_GUARD_PAGE_LEADING(void)
+{
+    const unsigned char *stack = task_stack_page(current);
+    const unsigned char *ptr = stack - 1;
+    volatile unsigned char byte;
+
+    pr_info("attempting bad read from page below current stack\n");
+
+    byte = *ptr;
+
+    pr_err("FAIL: accessed page before stack!\n");
+}
+
+/* Test that VMAP_STACK is actually allocating with a trailing guard page */
+void lkdtm_STACK_GUARD_PAGE_TRAILING(void)
+{

drivers/misc/lkdtm_bugs.c void lkdtm_CORRUPT_USER_DS(void) · line 239

Quoted 10 lines

drivers/misc/lkdtm_bugs.c +10 -0

+    const unsigned char *stack = task_stack_page(current);
+    const unsigned char *ptr = stack + THREAD_SIZE;
+    volatile unsigned char byte;
+
+    pr_info("attempting bad read from page above current stack\n");
+
+    byte = *ptr;
+
+    pr_err("FAIL: accessed page after stack!\n");
+}

I can give these a go tomorrow.

These *should* fault, and IIUC should trigger the usual "Unable to handle kernel %s at virtual address %08lx\n" splat from arm64's __do_kernel_fault(), which should end up with an Oops().

Quoted 7 lines

Since these don't mess with the SP, they shouldn't trigger the overflow detection, which detects whether we have sufficient stack space to store the exception context to the stack. That caught the LKDTM overflow test reliably.

Thanks,
Mark.

I gave these a quick test in QEMU and they seem to do the correct thing:

# echo STACK_GUARD_PAGE_LEADING > /sys/kernel/debug/provoke-crash/DIRECT
[   24.593306] lkdtm: Performing direct entry STACK_GUARD_PAGE_LEADING
[   24.593780] lkdtm: attempting bad read from page below current stack
[   24.594289] Unable to handle kernel paging request at virtual address ffff000009b77fff
[...]

# echo STACK_GUARD_PAGE_TRAILING > /sys/kernel/debug/provoke-crash/DIRECT
[  103.144313] lkdtm: Performing direct entry STACK_GUARD_PAGE_TRAILING
[  103.144749] lkdtm: attempting bad read from page above current stack
[  103.145100] Unable to handle kernel paging request at virtual address ffff000009c2c000
[...]

I also confirmed they failed as expected with CONFIG_VMAP_STACK=n

Quoted 7 lines · Mark Rutland

Quoted 4 lines · Kees Cook

Do these tests both trip with the new arm64 VMAP_STACK code?

drivers/misc/lkdtm_bugs.c +2 -0

+/* Test that VMAP_STACK is actually allocating with a leading guard page */
+void lkdtm_STACK_GUARD_PAGE_LEADING(void)

Quoted 8 lines

drivers/misc/lkdtm_bugs.c void lkdtm_CORRUPT_USER_DS(void) · line 228

Quoted 2 lines

drivers/misc/lkdtm_bugs.c +2 -0

+/* Test that VMAP_STACK is actually allocating with a trailing guard page */
+void lkdtm_STACK_GUARD_PAGE_TRAILING(void)

I can give these a go tomorrow.

These *should* fault, and IIUC should trigger the usual "Unable to handle kernel %s at virtual address %08lx\n" splat from arm64's __do_kernel_fault(), which should end up with an Oops().

I gave these a quick test in QEMU and they seem to do the correct thing:

# echo STACK_GUARD_PAGE_LEADING > /sys/kernel/debug/provoke-crash/DIRECT 
[   24.593306] lkdtm: Performing direct entry STACK_GUARD_PAGE_LEADING
[   24.593780] lkdtm: attempting bad read from page below current stack
[   24.594289] Unable to handle kernel paging request at virtual address ffff000009b77fff
[   24.594747] swapper pgtable: 4k pages, 48-bit VAs, pgd = ffff000009050000
[   24.595443] [ffff000009b77fff] *pgd=000000007effe003, *pud=000000007effd003, *pmd=000000007cc43003, *pte=0000000000000000
[   24.596743] Internal error: Oops: 96000007 [#1] PREEMPT SMP

# echo STACK_GUARD_PAGE_TRAILING > /sys/kernel/debug/provoke-crash/DIRECT 
[  103.144313] lkdtm: Performing direct entry STACK_GUARD_PAGE_TRAILING
[  103.144749] lkdtm: attempting bad read from page above current stack
[  103.145100] Unable to handle kernel paging request at virtual address ffff000009c2c000
[  103.145477] swapper pgtable: 4k pages, 48-bit VAs, pgd = ffff000009050000
[  103.145836] [ffff000009c2c000] *pgd=000000007effe003, *pud=000000007effd003, *pmd=000000007d014003, *pte=0000000000000000
[  103.146445] Internal error: Oops: 96000007 [#2] PREEMPT SMP