virtually mapped stacks and thread_info cleanup

SMP does ECB crypto on stack buffers. This is complicated and fragile, and it will not work if the stack is virtually allocated.

Switch to the crypto_cipher interface, which is simpler and safer.

Cc Marcel Holtmann <[email protected]>

Cc Gustavo Padovan <[email protected]>

Cc Johan Hedberg <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc [email protected]

Acked-by Herbert Xu <[email protected]>

Acked-and-tested-by Johan Hedberg <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
net/bluetooth/smp.c | 67 ++++++++++++++++++++++-------------------------------

1 file changed, 28 insertions(+), 39 deletions(-)

Hi Andy,

Quoted 14 lines

SMP does ECB crypto on stack buffers. This is complicated and fragile, and it will not work if the stack is virtually allocated.

Switch to the crypto_cipher interface, which is simpler and safer.

Cc Marcel Holtmann <[email protected]>

Cc Gustavo Padovan <[email protected]>

Cc Johan Hedberg <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc [email protected]

Acked-by Herbert Xu <[email protected]>

Acked-and-tested-by Johan Hedberg <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Quoted 3 lines

---
net/bluetooth/smp.c | 67 ++++++++++++++++++++++-------------------------------

1 file changed, 28 insertions(+), 39 deletions(-)

patch has been applied to bluetooth-next tree.

Quoted 18 lines

Quoted 17 lines

SMP does ECB crypto on stack buffers. This is complicated and fragile, and it will not work if the stack is virtually allocated.

Switch to the crypto_cipher interface, which is simpler and safer.

Cc Marcel Holtmann <[email protected]>

Cc Gustavo Padovan <[email protected]>

Cc Johan Hedberg <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc [email protected]

Acked-by Herbert Xu <[email protected]>

Acked-and-tested-by Johan Hedberg <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
net/bluetooth/smp.c | 67 ++++++++++++++++++++++-------------------------------

1 file changed, 28 insertions(+), 39 deletions(-)

Quoted 1 line

patch has been applied to bluetooth-next tree.

Sadly carrying this separately will delay the virtual kernel stacks feature by a kernel cycle, because it's a must-have prerequisite.

Hi Ingo,

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

SMP does ECB crypto on stack buffers. This is complicated and fragile, and it will not work if the stack is virtually allocated.

Switch to the crypto_cipher interface, which is simpler and safer.

Cc Marcel Holtmann <[email protected]>

Cc Gustavo Padovan <[email protected]>

Cc Johan Hedberg <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc [email protected]

Acked-by Herbert Xu <[email protected]>

Acked-and-tested-by Johan Hedberg <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
net/bluetooth/smp.c | 67 ++++++++++++++++++++++-------------------------------

1 file changed, 28 insertions(+), 39 deletions(-)

Quoted 4 lines

Quoted 1 line

patch has been applied to bluetooth-next tree.

Sadly carrying this separately will delay the virtual kernel stacks feature by a kernel cycle, because it's a must-have prerequisite.

I can take it back out, but then I have the fear the the ECDH change to use KPP for SMP might be the one that has to wait a kernel cycle. Either way is fine with me, but I want to avoid nasty merge conflicts in the Bluetooth SMP code.

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

SMP does ECB crypto on stack buffers. This is complicated and fragile, and it will not work if the stack is virtually allocated.

Switch to the crypto_cipher interface, which is simpler and safer.

Cc Marcel Holtmann <[email protected]>

Cc Gustavo Padovan <[email protected]>

Cc Johan Hedberg <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc [email protected]

Acked-by Herbert Xu <[email protected]>

Acked-and-tested-by Johan Hedberg <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
net/bluetooth/smp.c | 67 ++++++++++++++++++++++-------------------------------

1 file changed, 28 insertions(+), 39 deletions(-)

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

patch has been applied to bluetooth-next tree.

Sadly carrying this separately will delay the virtual kernel stacks feature by a kernel cycle, because it's a must-have prerequisite.

I can take it back out, but then I have the fear the the ECDH change to use KPP for SMP might be the one that has to wait a kernel cycle. Either way is fine with me, but I want to avoid nasty merge conflicts in the Bluetooth SMP code.

Nothing goes wrong if an identical patch is queued in both places, right? Or, if you prefer not to duplicate it, could one of you commit it and the other one pull it? Ingo, given that this is patch 1 in the series and unlikely to change, if you want to make this whole thing have a separate branch in -tip, this could live there for starters. (But, if you do so, please make sure you base off a very new copy of Linus' tree -- the series is heavily dependent on the thread_info change he applied a few days ago.)

Hi Andy,

Quoted 25 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

SMP does ECB crypto on stack buffers. This is complicated and fragile, and it will not work if the stack is virtually allocated.

Switch to the crypto_cipher interface, which is simpler and safer.

Cc Marcel Holtmann <[email protected]>

Cc Gustavo Padovan <[email protected]>

Cc Johan Hedberg <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Cc [email protected]

Acked-by Herbert Xu <[email protected]>

Acked-and-tested-by Johan Hedberg <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
net/bluetooth/smp.c | 67 ++++++++++++++++++++++-------------------------------

1 file changed, 28 insertions(+), 39 deletions(-)

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

patch has been applied to bluetooth-next tree.

Sadly carrying this separately will delay the virtual kernel stacks feature by a kernel cycle, because it's a must-have prerequisite.

I can take it back out, but then I have the fear the the ECDH change to use KPP for SMP might be the one that has to wait a kernel cycle. Either way is fine with me, but I want to avoid nasty merge conflicts in the Bluetooth SMP code.

Quoted 8 lines

Nothing goes wrong if an identical patch is queued in both places, right? Or, if you prefer not to duplicate it, could one of you commit it and the other one pull it? Ingo, given that this is patch 1 in the series and unlikely to change, if you want to make this whole thing have a separate branch in -tip, this could live there for starters. (But, if you do so, please make sure you base off a very new copy of Linus' tree -- the series is heavily dependent on the thread_info change he applied a few days ago.)

so what are doing now? I take this back out or we keep it in and let git deal with it when merging the trees?

net/rxrpc/ar-internal.h struct rxrpc_connection { · line 283

Quoted 8 lines · Andy Lutomirski

net/rxrpc/ar-internal.h +1 -0

@@ -277,6 +277,7 @@ struct rxrpc_connection {
 	struct key		*key;		/* security for this connection (client) */
 	struct key		*server_key;	/* security for this service */
 	struct crypto_skcipher	*cipher;	/* encryption handle */
+	struct rxrpc_crypt	csum_iv_head;	/* leading block for csum_iv */
 	struct rxrpc_crypt	csum_iv;	/* packet checksum base */
 	unsigned long		events;
 #define RXRPC_CONN_CHALLENGE	0		/* send challenge packet */

NAK. This won't work. csum_iv_head is per packet being processed, but you've put it in rxrpc_connection which is shared amongst several creators/digestors of packets. Putting it in rxrpc_call won't work either since it's also needed for connection level packets.

Huh? If you can't write to csum_iv_head without clobbering others then by the same reasoning you can't write to csum_iv either. So unless you're saying the existing code is already broken then there is nothing wrong with the patch.

Since it's (more or less) a one off piece of memory, why not kmalloc it temporarily rather than expanding the connection struct? Also, the bit where you put a second rxrpc_crypt in just so that it happens to give you a 16-byte slot by adjacency is pretty icky. It would be much better to use a union instead:

	union {
		struct rxrpc_crypt	csum_iv; /* packet checksum base */
		__be32 tmpbuf[4];
	};

Note also that the above doesn't guarantee that the struct will be inside of a single page. It would need an alignment of 16 for that - but you only have one sg. Could that be a problem?

/*
- * load a scatterlist with a potentially split-page buffer
+ * load a scatterlist
*/

net/rxrpc/rxkad.c +2 -6

-static void rxkad_sg_set_buf2(struct scatterlist sg[2],
+static void rxkad_sg_set_buf2(struct scatterlist sg[1],
 			      void *buf, size_t buflen)
 {
-	int nsg = 1;
-
-	sg_init_table(sg, 2);
-
+	sg_init_table(sg, 1);
 	sg_set_buf(&sg[0], buf, buflen);
-	if (sg[0].offset + buflen > PAGE_SIZE) {

Review

Quoted 2 lines · Andy Lutomirski

(quoted diff) +1 -1

-	skcipher_request_set_crypt(req, &sg[1], &sg[0], sizeof(tmpbuf), iv.x);
+	skcipher_request_set_crypt(req, &sg, &sg, sizeof(tmpbuf), iv.x);

Don't the sg's have to be different? Aren't they both altered by the process of reading/writing from them?

net/rxrpc/rxkad.c static int rxkad_secure_packet(const struct rxrpc_call *call, · line 288

Quoted 5 lines

	struct rxrpc_skb_priv *sp;

...

net/rxrpc/rxkad.c +3 -0

+	swap(tmpbuf.xl, *(__be64 *)sp);
+
+	sg_init_one(&sg, sp, sizeof(tmpbuf));

???? I assume you're assuming that the rxrpc_skb_priv struct contents can arbitrarily replaced temporarily...

And using an XCHG-equivalent instruction? This won't work on a 32-bit arch (apart from one that sports CMPXCHG8 or similar).

Quoted 2 lines

???? I assume you're assuming that the rxrpc_skb_priv struct contents can arbitrarily replaced temporarily...

Of course you can, it's per-skb state.

I'm using the per-skb state for my own purposes and might be looking at it elsewhere at the same time.

Quoted 2 lines

I'm using the per-skb state for my own purposes and might be looking at it elsewhere at the same time.

AFAICS this cannot happen for secure_packet/verify_packet. In both cases we have exclusive ownership of the skb.

But it's your code so feel free to send your own patch.

This avoids pointless races in which another CPU or task might see a partially populated global pgd entry. These races should normally be harmless, but, if another CPU propagates the entry via vmalloc_fault and then populate_pgd fails (due to memory allocation failure, for example), this prevents a use-after-free of the pgd entry.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/mm/pageattr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/x86/mm/pageattr.c +0 -0

index 7a1f7bbf4105..6a8026918bf6 100644

Quoted 18 lines · Andy Lutomirski

This avoids pointless races in which another CPU or task might see a partially populated global pgd entry. These races should normally be harmless, but, if another CPU propagates the entry via vmalloc_fault and then populate_pgd fails (due to memory allocation failure, for example), this prevents a use-after-free of the pgd entry.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/mm/pageattr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/x86/mm/pageattr.c +0 -0

index 7a1f7bbf4105..6a8026918bf6 100644
@@ -1113,7 +1113,9 @@ static int populate_pgd(struct cpa_data *cpa, unsigned long addr)
 

arch/x86/mm/pageattr.c static int populate_pgd(struct cpa_data *cpa, unsigned long addr)

Quoted 10 lines

arch/x86/mm/pageattr.c +3 -2

      ret = populate_pud(cpa, addr, pgd_entry, pgprot);
      if (ret < 0) {
-             unmap_pgd_range(cpa->pgd, addr,
+             if (pud)
+                     free_page((unsigned long)pud);
+             unmap_pud_range(pgd_entry, addr,
                              addr + (cpa->numpages << PAGE_SHIFT));
              return ret;
      }
--

So something's amiss here. Subject says:

"x86/cpa: In populate_pgd, don't set the pgd entry until it's populated"

but you haven't moved

        set_pgd(pgd_entry, __pgd(__pa(pud) | _KERNPG_TABLE));

after populate_pud() succeeds... Which is a good catch but your patch should do it too. :-)

kernel_unmap_pages_in_pgd() is dangerous: if a pgd entry in init_mm.pgd were to be cleared, callers would need to ensure that the pgd entry hadn't been propagated to any other pgd.

Its only caller was efi_cleanup_page_tables(), and that, in turn, was unused, so just delete both functions. This leaves a couple of other helpers unused, so delete them, too.

Cc Matt Fleming <[email protected]>

Cc [email protected]

Reviewed-by Matt Fleming <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

arch/x86/include/asm/efi.h           |  1 -
arch/x86/include/asm/pgtable_types.h |  2 --
arch/x86/mm/pageattr.c               | 28 ----------------------------
arch/x86/platform/efi/efi.c          |  2 --
arch/x86/platform/efi/efi_32.c       |  3 ---
arch/x86/platform/efi/efi_64.c       |  5 -----
6 files changed, 41 deletions(-)

If we're using CONFIG_VMAP_STACK and we manage to point an sg entry at the stack, then either the sg page will be in highmem or sg_virt will return the direct-map alias. In neither case will the existing check_for_stack() implementation realize that it's a stack page.

Fix it by explicitly checking for stack pages.

This has no effect by itself. It's broken out for ease of review.

Cc Andrew Morton <[email protected]>

Cc Arnd Bergmann <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 lib/dma-debug.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

lib/dma-debug.c +1 -0

index 51a76af25c66..5b2e63cba90e 100644
@@ -22,6 +22,7 @@
 #include <linux/stacktrace.h>
 #include <linux/dma-debug.h>
 #include <linux/spinlock.h>
+#include <linux/vmalloc.h>
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
 #include <linux/export.h>

@@ -1162,11 +1163,35 @@ static void check_unmap(struct dma_debug_entry *ref)
 	put_hash_bucket(bucket, &flags);
 }

Quoted 31 lines · Andy Lutomirski

If we're using CONFIG_VMAP_STACK and we manage to point an sg entry at the stack, then either the sg page will be in highmem or sg_virt will return the direct-map alias. In neither case will the existing check_for_stack() implementation realize that it's a stack page.

Fix it by explicitly checking for stack pages.

This has no effect by itself. It's broken out for ease of review.

Cc Andrew Morton <[email protected]>

Cc Arnd Bergmann <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 lib/dma-debug.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

lib/dma-debug.c +1 -0

index 51a76af25c66..5b2e63cba90e 100644
@@ -22,6 +22,7 @@
 #include <linux/stacktrace.h>
 #include <linux/dma-debug.h>
 #include <linux/spinlock.h>
+#include <linux/vmalloc.h>
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
 #include <linux/export.h>

@@ -1162,11 +1163,35 @@ static void check_unmap(struct dma_debug_entry *ref)
      put_hash_bucket(bucket, &flags);
 }

lib/dma-debug.c static void check_unmap(struct dma_debug_entry *ref) · line 1170

Quoted 10 lines

lib/dma-debug.c +4 -4

 
-static void check_for_stack(struct device *dev, void *addr)
+static void check_for_stack(struct device *dev,
+                         struct page *page, size_t offset)
 {
-     if (object_is_on_stack(addr))
-             err_printk(dev, NULL, "DMA-API: device driver maps memory from "
-                             "stack [addr=%p]\n", addr);
+     void *addr;
+     struct vm_struct *stack_vm_area = task_stack_vm_area(current);

lib/dma-debug.c: In function ‘check_for_stack’:
lib/dma-debug.c:1170:36: error: implicit declaration of function ‘task_stack_vm_area’ [-Werror=implicit-function-declaration]

  struct vm_struct *stack_vm_area = task_stack_vm_area(current);
                                    ^

lib/dma-debug.c:1170:36: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
cc1: some warnings being treated as errors
make[1]: *** [lib/dma-debug.o] Error 1
make: *** [lib] Error 2
make: *** Waiting for unfinished jobs....

Probably reorder pieces from patch 9 to earlier ones...

If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with vmalloc_node.

grsecurity has had a similar feature (called
GRKERNSEC_KSTACKOVERFLOW) for a long time.

Cc Oleg Nesterov <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/Kconfig                        | 29 +++++++++++++
 arch/ia64/include/asm/thread_info.h |  2 +-
 include/linux/sched.h               | 15 +++++++
 kernel/fork.c                       | 87 +++++++++++++++++++++++++++++--------
 4 files changed, 113 insertions(+), 20 deletions(-)

arch/Kconfig +9 -0

index 15996290fed4..18a2c3a7b460 100644
@@ -661,4 +661,33 @@ config ARCH_NO_COHERENT_DMA_MMAP
 config CPU_NO_EFFICIENT_FFS
 	def_bool n
 
+config HAVE_ARCH_VMAP_STACK
+	def_bool n
+	help
+	  An arch should select this symbol if it can support kernel stacks
+	  in vmalloc space.  This means:
+
+	  - vmalloc space must be large enough to hold many kernel stacks.
+	    This may rule out many 32-bit architectures.
+

kernel/fork.c +0 -1

index 146c9840c079..06761de69360 100644
@@ -158,19 +158,37 @@ void __weak arch_release_thread_stack(unsigned long *stack)
  * Allocate pages if THREAD_SIZE is >= PAGE_SIZE, otherwise use a
  * kmemcache based allocator.
  */
-# if THREAD_SIZE >= PAGE_SIZE

kernel/fork.c +23 -12

+	 * the vm_struct.
+	 */
+	if (stack)
+		tsk->stack_vm_area = find_vm_area(stack);
+	return stack;
+#else
 	struct page *page = alloc_kmem_pages_node(node, THREADINFO_GFP,
 						  THREAD_SIZE_ORDER);
 
 	return page ? page_address(page) : NULL;
+#endif
 }
 
-static inline void free_thread_stack(unsigned long *stack)
+static inline void free_thread_stack(struct task_struct *tsk)
 {
-	free_kmem_pages((unsigned long)stack, THREAD_SIZE_ORDER);
+	if (task_stack_vm_area(tsk))
+		vfree(tsk->stack);
+	else
+		free_kmem_pages((unsigned long)tsk->stack, THREAD_SIZE_ORDER);
 }
 # else
 static struct kmem_cache *thread_stack_cache;

@@ -181,9 +199,9 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk,
 	return kmem_cache_alloc_node(thread_stack_cache, THREADINFO_GFP, node);
 }
 
-static void free_thread_stack(unsigned long *stack)
+static void free_thread_stack(struct task_struct *tsk)
 {
-	kmem_cache_free(thread_stack_cache, stack);
+	kmem_cache_free(thread_stack_cache, tsk->stack);
 }
 
 void thread_stack_cache_init(void)

@@ -213,24 +231,49 @@ struct kmem_cache *vm_area_cachep;
 /* SLAB cache for mm_struct structures (tsk->mm) */
 static struct kmem_cache *mm_cachep;
 
-static void account_kernel_stack(unsigned long *stack, int account)
+static void account_kernel_stack(struct task_struct *tsk, int account)
 {
-	/* All stack pages are in the same zone and belong to the same memcg. */
-	struct page *first_page = virt_to_page(stack);
+	void *stack = task_stack_page(tsk);
+	struct vm_struct *vm = task_stack_vm_area(tsk);
+
+	BUILD_BUG_ON(IS_ENABLED(CONFIG_VMAP_STACK) && PAGE_SIZE % 1024 != 0);
+
+	if (vm) {
+		int i;
 
-	mod_zone_page_state(page_zone(first_page), NR_KERNEL_STACK_KB,
-			    THREAD_SIZE / 1024 * account);
+		BUG_ON(vm->nr_pages != THREAD_SIZE / PAGE_SIZE);
 
-	memcg_kmem_update_page_stat(
-		first_page, MEMCG_KERNEL_STACK_KB,
-		account * (THREAD_SIZE / 1024));

kernel/fork.c +3 -2

+	}
 }
 
 void free_task(struct task_struct *tsk)
 {
-	account_kernel_stack(tsk->stack, -1);
+	account_kernel_stack(tsk, -1);
 	arch_release_thread_stack(tsk->stack);
-	free_thread_stack(tsk->stack);
+	free_thread_stack(tsk);
 	rt_mutex_debug_task_free(tsk);
 	ftrace_graph_exit_task(tsk);
 	put_seccomp_filter(tsk);

Quoted 3 lines · Andy Lutomirski

If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with vmalloc_node.

It's not going to work, because the scheduler will explode if we try to schedule when running on an IST stack or similar.

This will matter when we let kernel stack overflows (which are #DF) call die().

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/dumpstack.c | 3 +++
 1 file changed, 3 insertions(+)

arch/x86/kernel/dumpstack.c +0 -0

index ef8017ca5ba9..352f022cfd5b 100644

Quoted 16 lines · Andy Lutomirski

It's not going to work, because the scheduler will explode if we try to schedule when running on an IST stack or similar.

This will matter when we let kernel stack overflows (which are #DF) call die().

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/dumpstack.c | 3 +++
 1 file changed, 3 insertions(+)

arch/x86/kernel/dumpstack.c +0 -0

index ef8017ca5ba9..352f022cfd5b 100644

arch/x86/kernel/dumpstack.c void oops_end(unsigned long flags, struct pt_regs *regs, int signr) · line 249

Quoted 5 lines

arch/x86/kernel/dumpstack.c +2 -0

@@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 		return;
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
+	if (((current_stack_pointer() ^ (current_top_of_stack() - 1))
+	     & ~(THREAD_SIZE - 1)) != 0)

Ugh, that's hard to parse. You could remove the "!= 0" at least to shorten it a bit and have one less braces level.

Or maybe even do something like that to make it a bit more readable:

        if ((current_stack_pointer() ^ (current_top_of_stack() - 1))
                        &
             ~(THREAD_SIZE - 1))
                panic("Fatal exception on non-default stack");

Meh.

The existing 'object_is_on_stack()' can probably be used:

	if (!object_is_on_stack(current_top_of_stack()))
		panic("...");

Though that function isn't quite accurately named. It should really have 'task_stack' in its name, like 'object_is_on_task_stack()'. Or even better, something more concise like 'on_task_stack()'.

Quoted 26 lines · Borislav Petkov

Quoted 16 lines · Andy Lutomirski

It's not going to work, because the scheduler will explode if we try to schedule when running on an IST stack or similar.

This will matter when we let kernel stack overflows (which are #DF) call die().

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/dumpstack.c | 3 +++
 1 file changed, 3 insertions(+)

arch/x86/kernel/dumpstack.c +0 -0

index ef8017ca5ba9..352f022cfd5b 100644

arch/x86/kernel/dumpstack.c void oops_end(unsigned long flags, struct pt_regs *regs, int signr) · line 249

Quoted 5 lines

arch/x86/kernel/dumpstack.c +2 -0

@@ -245,6 +245,9 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
            return;
    if (in_interrupt())
            panic("Fatal exception in interrupt");
+   if (((current_stack_pointer() ^ (current_top_of_stack() - 1))
+        & ~(THREAD_SIZE - 1)) != 0)

Ugh, that's hard to parse. You could remove the "!= 0" at least to shorten it a bit and have one less braces level.

Quoted 8 lines

Or maybe even do something like that to make it a bit more readable:

        if ((current_stack_pointer() ^ (current_top_of_stack() - 1))
                        &
             ~(THREAD_SIZE - 1))
                panic("Fatal exception on non-default stack");

Meh.

A helper function would be even better.

The existing 'object_is_on_stack()' can probably be used:

        if (!object_is_on_stack(current_top_of_stack()))
                panic("...");

Though that function isn't quite accurately named. It should really have 'task_stack' in its name, like 'object_is_on_task_stack()'. Or even better, something more concise like 'on_task_stack()'.

Given that the very next patch deletes this code, I vote for leaving it alone. Or I could fold the patches together.

If we call do_exit with a clean stack, we greatly reduce the risk of

recursive oopses due to stack overflow in do_exit, and we allow

do_exit to work even if we OOPS from an IST stack. The latter gives

us a much better chance of surviving long enough after we detect a stack overflow to write out our logs.

I intentionally separated this from the preceding patch that disables do_exit-on-OOPS on IST stacks. This way, if we need to revert this patch, we still end up in an acceptable state wrt stack overflow handling.

Reviewed-by Josh Poimboeuf <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

arch/x86/kernel/dumpstack.c +1 -0

+
 void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 {
 	if (regs && kexec_should_crash(current))

This allows x86_64 kernels to enable vmapped stacks. There are a couple of interesting bits.

First, x86 lazily faults in top-level paging entries for the vmalloc area. This won't work if we get a page fault while trying to access the stack: the CPU will promote it to a double-fault and we'll die. To avoid this problem, probe the new stack when switching stacks and forcibly populate the pgd entry for the stack when switching mms.

Second, once we have guard pages around the stack, we'll want to detect and handle stack overflow.

I didn't enable it on x86_32. We'd need to rework the double-fault code a bit and I'm concerned about running out of vmalloc virtual addresses under some workloads.

This patch, by itself, will behave somewhat erratically when the stack overflows while RSP is still more than a few tens of bytes above the bottom of the stack. Specifically, we'll get #PF and make it to no_context and an oops without triggering a double-fault, and no_context doesn't know about stack overflows. The next patch will improve that case.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                 |  1 +
 arch/x86/include/asm/switch_to.h | 28 +++++++++++++++++++++++++++-
 arch/x86/kernel/traps.c          | 32 ++++++++++++++++++++++++++++++++
 arch/x86/mm/tlb.c                | 15 +++++++++++++++
 4 files changed, 75 insertions(+), 1 deletion(-)

arch/x86/Kconfig +1 -0

index d9a94da0c29f..afdcf96ef109 100644
@@ -92,6 +92,7 @@ config X86
        select HAVE_ARCH_TRACEHOOK
        select HAVE_ARCH_TRANSPARENT_HUGEPAGE
        select HAVE_EBPF_JIT                    if X86_64
+       select HAVE_ARCH_VMAP_STACK             if X86_64
        select HAVE_CC_STACKPROTECTOR
        select HAVE_CMPXCHG_DOUBLE
        select HAVE_CMPXCHG_LOCAL

arch/x86/include/asm/switch_to.h +27 -1

index 8f321a1b03a1..14e4b20f0aaf 100644
@@ -8,6 +8,28 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
                      struct tss_struct *tss);
 
+/* This runs runs on the previous thread's stack. */
+static inline void prepare_switch_to(struct task_struct *prev,
+                                    struct task_struct *next)
+{
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we switch to a stack that has a top-level paging entry
+        * that is not present in the current mm, the resulting #PF will
+        * will be promoted to a double-fault and we'll panic.  Probe
+        * the new stack now so that vmalloc_fault can fix up the page
+        * tables if needed.  This can only happen if we use a stack
+        * in vmap space.
+        *
+        * We assume that the stack is aligned so that it never spans
+        * more than one top-level paging entry.
+        *
+        * To minimize cache pollution, just follow the stack pointer.
+        */
+       READ_ONCE(*(unsigned char *)next->thread.sp);
+#endif
+}
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

@@ -39,6 +61,8 @@ do {                                                                  \
         */                                                             \
        unsigned long ebx, ecx, edx, esi, edi;                          \
                                                                        \
+       prepare_switch_to(prev, next);                                  \
+                                                                       \
        asm volatile("pushl %%ebp\n\t"          /* save    EBP   */     \
                     "movl %%esp,%[prev_sp]\n\t"        /* save    ESP   */ \
                     "movl %[next_sp],%%esp\n\t"        /* restore ESP   */ \

@@ -103,7 +127,9 @@ do {                                                                        \
  * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
  * has no effect.
  */
-#define switch_to(prev, next, last) \
+#define switch_to(prev, next, last)                                      \
+       prepare_switch_to(prev, next);                                    \
+                                                                         \
        asm volatile(SAVE_CONTEXT                                         \
             "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */       \
             "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */    \

arch/x86/kernel/traps.c +21 -0

index 00f03d82e69a..9cb7ea781176 100644
@@ -292,12 +292,30 @@ DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present", segment_not_present)
 DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",            stack_segment)
 DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",          alignment_check)
 
+#ifdef CONFIG_VMAP_STACK
+static void __noreturn handle_stack_overflow(const char *message,
+                                            struct pt_regs *regs,
+                                            unsigned long fault_address)
+{
+       printk(KERN_EMERG "BUG: stack guard page was hit at %p (stack is %p..%p)\n",
+                (void *)fault_address, current->stack,
+                (char *)current->stack + THREAD_SIZE - 1);
+       die(message, regs, 0);
+
+       /* Be absolutely certain we don't return. */
+       panic(message);
+}
+#endif
+
 #ifdef CONFIG_X86_64
 /* Runs on IST stack */
 dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 {
        static const char str[] = "double fault";
        struct task_struct *tsk = current;
+#ifdef CONFIG_VMAP_STACK
+       unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
        extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        tsk->thread.error_code = error_code;
        tsk->thread.trap_nr = X86_TRAP_DF;
 
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we overflow the stack into a guard page, the CPU will fail

Quoted 135 lines · Andy Lutomirski

This allows x86_64 kernels to enable vmapped stacks. There are a couple of interesting bits.

First, x86 lazily faults in top-level paging entries for the vmalloc area. This won't work if we get a page fault while trying to access the stack: the CPU will promote it to a double-fault and we'll die. To avoid this problem, probe the new stack when switching stacks and forcibly populate the pgd entry for the stack when switching mms.

Second, once we have guard pages around the stack, we'll want to detect and handle stack overflow.

I didn't enable it on x86_32. We'd need to rework the double-fault code a bit and I'm concerned about running out of vmalloc virtual addresses under some workloads.

This patch, by itself, will behave somewhat erratically when the stack overflows while RSP is still more than a few tens of bytes above the bottom of the stack. Specifically, we'll get #PF and make it to no_context and an oops without triggering a double-fault, and no_context doesn't know about stack overflows. The next patch will improve that case.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                 |  1 +
 arch/x86/include/asm/switch_to.h | 28 +++++++++++++++++++++++++++-
 arch/x86/kernel/traps.c          | 32 ++++++++++++++++++++++++++++++++
 arch/x86/mm/tlb.c                | 15 +++++++++++++++
 4 files changed, 75 insertions(+), 1 deletion(-)

arch/x86/Kconfig +1 -0

index d9a94da0c29f..afdcf96ef109 100644
@@ -92,6 +92,7 @@ config X86
        select HAVE_ARCH_TRACEHOOK
        select HAVE_ARCH_TRANSPARENT_HUGEPAGE
        select HAVE_EBPF_JIT                    if X86_64
+       select HAVE_ARCH_VMAP_STACK             if X86_64
        select HAVE_CC_STACKPROTECTOR
        select HAVE_CMPXCHG_DOUBLE
        select HAVE_CMPXCHG_LOCAL

arch/x86/include/asm/switch_to.h +27 -1

index 8f321a1b03a1..14e4b20f0aaf 100644
@@ -8,6 +8,28 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
                      struct tss_struct *tss);
 
+/* This runs runs on the previous thread's stack. */
+static inline void prepare_switch_to(struct task_struct *prev,
+                                    struct task_struct *next)
+{
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we switch to a stack that has a top-level paging entry
+        * that is not present in the current mm, the resulting #PF will
+        * will be promoted to a double-fault and we'll panic.  Probe
+        * the new stack now so that vmalloc_fault can fix up the page
+        * tables if needed.  This can only happen if we use a stack
+        * in vmap space.
+        *
+        * We assume that the stack is aligned so that it never spans
+        * more than one top-level paging entry.
+        *
+        * To minimize cache pollution, just follow the stack pointer.
+        */
+       READ_ONCE(*(unsigned char *)next->thread.sp);
+#endif
+}
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

@@ -39,6 +61,8 @@ do {                                                                  \
         */                                                             \
        unsigned long ebx, ecx, edx, esi, edi;                          \
                                                                        \
+       prepare_switch_to(prev, next);                                  \
+                                                                       \
        asm volatile("pushl %%ebp\n\t"          /* save    EBP   */     \
                     "movl %%esp,%[prev_sp]\n\t"        /* save    ESP   */ \
                     "movl %[next_sp],%%esp\n\t"        /* restore ESP   */ \

@@ -103,7 +127,9 @@ do {                                                                        \
  * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
  * has no effect.
  */
-#define switch_to(prev, next, last) \
+#define switch_to(prev, next, last)                                      \
+       prepare_switch_to(prev, next);                                    \
+                                                                         \
        asm volatile(SAVE_CONTEXT                                         \
             "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */       \
             "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */    \

arch/x86/kernel/traps.c +18 -0

index 00f03d82e69a..9cb7ea781176 100644
@@ -292,12 +292,30 @@ DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present", segment_not_present)
 DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",            stack_segment)
 DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",          alignment_check)
 
+#ifdef CONFIG_VMAP_STACK
+static void __noreturn handle_stack_overflow(const char *message,
+                                            struct pt_regs *regs,
+                                            unsigned long fault_address)
+{
+       printk(KERN_EMERG "BUG: stack guard page was hit at %p (stack is %p..%p)\n",
+                (void *)fault_address, current->stack,
+                (char *)current->stack + THREAD_SIZE - 1);
+       die(message, regs, 0);
+
+       /* Be absolutely certain we don't return. */
+       panic(message);
+}
+#endif
+
 #ifdef CONFIG_X86_64
 /* Runs on IST stack */
 dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 {
        static const char str[] = "double fault";
        struct task_struct *tsk = current;
+#ifdef CONFIG_VMAP_STACK
+       unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
        extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        tsk->thread.error_code = error_code;
        tsk->thread.trap_nr = X86_TRAP_DF;
 

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code) · line 365

Quoted 13 lines

arch/x86/kernel/traps.c +13 -0

+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we overflow the stack into a guard page, the CPU will fail
+        * to deliver #PF and will send #DF instead.  CR2 will contain
+        * the linear address of the second fault, which will be in the
+        * guard page below the bottom of the stack.
+        */
+       cr2 = read_cr2();
+       if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+               handle_stack_overflow(
+                       "kernel stack overflow (double-fault)",
+                       regs, cr2);
+#endif

Is there any other way to tell if this was from a page fault? If it wasn't a page fault then CR2 is undefined.

Quoted 136 lines · Brian Gerst

Quoted 135 lines · Andy Lutomirski

This allows x86_64 kernels to enable vmapped stacks. There are a couple of interesting bits.

First, x86 lazily faults in top-level paging entries for the vmalloc area. This won't work if we get a page fault while trying to access the stack: the CPU will promote it to a double-fault and we'll die. To avoid this problem, probe the new stack when switching stacks and forcibly populate the pgd entry for the stack when switching mms.

Second, once we have guard pages around the stack, we'll want to detect and handle stack overflow.

I didn't enable it on x86_32. We'd need to rework the double-fault code a bit and I'm concerned about running out of vmalloc virtual addresses under some workloads.

This patch, by itself, will behave somewhat erratically when the stack overflows while RSP is still more than a few tens of bytes above the bottom of the stack. Specifically, we'll get #PF and make it to no_context and an oops without triggering a double-fault, and no_context doesn't know about stack overflows. The next patch will improve that case.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/Kconfig                 |  1 +
 arch/x86/include/asm/switch_to.h | 28 +++++++++++++++++++++++++++-
 arch/x86/kernel/traps.c          | 32 ++++++++++++++++++++++++++++++++
 arch/x86/mm/tlb.c                | 15 +++++++++++++++
 4 files changed, 75 insertions(+), 1 deletion(-)

arch/x86/Kconfig +1 -0

index d9a94da0c29f..afdcf96ef109 100644
@@ -92,6 +92,7 @@ config X86
        select HAVE_ARCH_TRACEHOOK
        select HAVE_ARCH_TRANSPARENT_HUGEPAGE
        select HAVE_EBPF_JIT                    if X86_64
+       select HAVE_ARCH_VMAP_STACK             if X86_64
        select HAVE_CC_STACKPROTECTOR
        select HAVE_CMPXCHG_DOUBLE
        select HAVE_CMPXCHG_LOCAL

arch/x86/include/asm/switch_to.h +27 -1

index 8f321a1b03a1..14e4b20f0aaf 100644
@@ -8,6 +8,28 @@ struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
                      struct tss_struct *tss);
 
+/* This runs runs on the previous thread's stack. */
+static inline void prepare_switch_to(struct task_struct *prev,
+                                    struct task_struct *next)
+{
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we switch to a stack that has a top-level paging entry
+        * that is not present in the current mm, the resulting #PF will
+        * will be promoted to a double-fault and we'll panic.  Probe
+        * the new stack now so that vmalloc_fault can fix up the page
+        * tables if needed.  This can only happen if we use a stack
+        * in vmap space.
+        *
+        * We assume that the stack is aligned so that it never spans
+        * more than one top-level paging entry.
+        *
+        * To minimize cache pollution, just follow the stack pointer.
+        */
+       READ_ONCE(*(unsigned char *)next->thread.sp);
+#endif
+}
+
 #ifdef CONFIG_X86_32
 
 #ifdef CONFIG_CC_STACKPROTECTOR

@@ -39,6 +61,8 @@ do {                                                                  \
         */                                                             \
        unsigned long ebx, ecx, edx, esi, edi;                          \
                                                                        \
+       prepare_switch_to(prev, next);                                  \
+                                                                       \
        asm volatile("pushl %%ebp\n\t"          /* save    EBP   */     \
                     "movl %%esp,%[prev_sp]\n\t"        /* save    ESP   */ \
                     "movl %[next_sp],%%esp\n\t"        /* restore ESP   */ \

@@ -103,7 +127,9 @@ do {                                                                        \
  * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
  * has no effect.
  */
-#define switch_to(prev, next, last) \
+#define switch_to(prev, next, last)                                      \
+       prepare_switch_to(prev, next);                                    \
+                                                                         \
        asm volatile(SAVE_CONTEXT                                         \
             "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */       \
             "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */    \

arch/x86/kernel/traps.c +18 -0

index 00f03d82e69a..9cb7ea781176 100644
@@ -292,12 +292,30 @@ DO_ERROR(X86_TRAP_NP,     SIGBUS,  "segment not present", segment_not_present)
 DO_ERROR(X86_TRAP_SS,     SIGBUS,  "stack segment",            stack_segment)
 DO_ERROR(X86_TRAP_AC,     SIGBUS,  "alignment check",          alignment_check)
 
+#ifdef CONFIG_VMAP_STACK
+static void __noreturn handle_stack_overflow(const char *message,
+                                            struct pt_regs *regs,
+                                            unsigned long fault_address)
+{
+       printk(KERN_EMERG "BUG: stack guard page was hit at %p (stack is %p..%p)\n",
+                (void *)fault_address, current->stack,
+                (char *)current->stack + THREAD_SIZE - 1);
+       die(message, regs, 0);
+
+       /* Be absolutely certain we don't return. */
+       panic(message);
+}
+#endif
+
 #ifdef CONFIG_X86_64
 /* Runs on IST stack */
 dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 {
        static const char str[] = "double fault";
        struct task_struct *tsk = current;
+#ifdef CONFIG_VMAP_STACK
+       unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
        extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        tsk->thread.error_code = error_code;
        tsk->thread.trap_nr = X86_TRAP_DF;
 

Quoted 16 lines

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code) · line 365

Quoted 13 lines

arch/x86/kernel/traps.c +13 -0

+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we overflow the stack into a guard page, the CPU will fail
+        * to deliver #PF and will send #DF instead.  CR2 will contain
+        * the linear address of the second fault, which will be in the
+        * guard page below the bottom of the stack.
+        */
+       cr2 = read_cr2();
+       if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+               handle_stack_overflow(
+                       "kernel stack overflow (double-fault)",
+                       regs, cr2);
+#endif

Is there any other way to tell if this was from a page fault? If it wasn't a page fault then CR2 is undefined.

I guess it doesn't really matter, since the fault is fatal either way. The error message might be incorrect though.

Quoted 35 lines · Brian Gerst

Quoted 33 lines · Brian Gerst

Quoted 19 lines · Andy Lutomirski

#ifdef CONFIG_X86_64
/* Runs on IST stack */
dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)

 {
        static const char str[] = "double fault";
        struct task_struct *tsk = current;

arch/x86/kernel/traps.c +6 -0

+#ifdef CONFIG_VMAP_STACK
+       unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
        extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        tsk->thread.error_code = error_code;
        tsk->thread.trap_nr = X86_TRAP_DF;
 
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we overflow the stack into a guard page, the CPU will fail

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code) · line 365

Quoted 10 lines

arch/x86/kernel/traps.c +10 -0

+        * to deliver #PF and will send #DF instead.  CR2 will contain
+        * the linear address of the second fault, which will be in the
+        * guard page below the bottom of the stack.
+        */
+       cr2 = read_cr2();
+       if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+               handle_stack_overflow(
+                       "kernel stack overflow (double-fault)",
+                       regs, cr2);
+#endif

Is there any other way to tell if this was from a page fault? If it wasn't a page fault then CR2 is undefined.

Quoted 3 lines

I guess it doesn't really matter, since the fault is fatal either way. The error message might be incorrect though.

It's at least worth a comment, though. Maybe I should check if regs->rsp is within 40 bytes of the bottom of the stack, too, such that delivery of an inner fault would have double-faulted assuming the inner fault didn't use an IST vector.

Quoted 36 lines · Andy Lutomirski

Quoted 35 lines · Brian Gerst

Quoted 33 lines · Brian Gerst

Quoted 19 lines · Andy Lutomirski

#ifdef CONFIG_X86_64
/* Runs on IST stack */
dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)

 {
        static const char str[] = "double fault";
        struct task_struct *tsk = current;

arch/x86/kernel/traps.c +6 -0

+#ifdef CONFIG_VMAP_STACK
+       unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
        extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        tsk->thread.error_code = error_code;
        tsk->thread.trap_nr = X86_TRAP_DF;
 
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we overflow the stack into a guard page, the CPU will fail

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code) · line 365

Quoted 10 lines

arch/x86/kernel/traps.c +10 -0

+        * to deliver #PF and will send #DF instead.  CR2 will contain
+        * the linear address of the second fault, which will be in the
+        * guard page below the bottom of the stack.
+        */
+       cr2 = read_cr2();
+       if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+               handle_stack_overflow(
+                       "kernel stack overflow (double-fault)",
+                       regs, cr2);
+#endif

Is there any other way to tell if this was from a page fault? If it wasn't a page fault then CR2 is undefined.

Quoted 9 lines

Quoted 3 lines

I guess it doesn't really matter, since the fault is fatal either way. The error message might be incorrect though.

It's at least worth a comment, though. Maybe I should check if regs->rsp is within 40 bytes of the bottom of the stack, too, such that delivery of an inner fault would have double-faulted assuming the inner fault didn't use an IST vector.

How about:

/*
* If we overflow the stack into a guard page, the CPU will fail
* to deliver #PF and will send #DF instead. CR2 will contain
* the linear address of the second fault, which will be in the
* guard page below the bottom of the stack.
*
* We're limited to using heuristics here, since the CPU does
* not tell us what type of fault failed and, if the first fault
* wasn't a page fault, CR2 may contain stale garbage. To mostly
* rule out garbage, we check if the saved RSP is close enough to
* the bottom of the stack to cause exception delivery to fail.
* The worst case is 7 stack slots: one for alignment, five for
* SS..RIP, and one for the error code.
*/
tsk_stack = (unsigned long)task_stack_page(tsk);

if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
/* A double-fault due to #PF delivery failure is plausible. */ cr2 = read_cr2();
if (tsk_stack - 1 - cr2 < PAGE_SIZE)
handle_stack_overflow(
"kernel stack overflow (double-fault)",
regs, cr2);
}

Quoted 64 lines · Andy Lutomirski

Quoted 36 lines · Andy Lutomirski

Quoted 35 lines · Brian Gerst

Quoted 33 lines · Brian Gerst

Quoted 19 lines · Andy Lutomirski

#ifdef CONFIG_X86_64
/* Runs on IST stack */
dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)

 {
        static const char str[] = "double fault";
        struct task_struct *tsk = current;

arch/x86/kernel/traps.c +6 -0

+#ifdef CONFIG_VMAP_STACK
+       unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
        extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        tsk->thread.error_code = error_code;
        tsk->thread.trap_nr = X86_TRAP_DF;
 
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we overflow the stack into a guard page, the CPU will fail

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code) · line 365

Quoted 10 lines

arch/x86/kernel/traps.c +10 -0

+        * to deliver #PF and will send #DF instead.  CR2 will contain
+        * the linear address of the second fault, which will be in the
+        * guard page below the bottom of the stack.
+        */
+       cr2 = read_cr2();
+       if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+               handle_stack_overflow(
+                       "kernel stack overflow (double-fault)",
+                       regs, cr2);
+#endif

Is there any other way to tell if this was from a page fault? If it wasn't a page fault then CR2 is undefined.

Quoted 9 lines

Quoted 3 lines

I guess it doesn't really matter, since the fault is fatal either way. The error message might be incorrect though.

It's at least worth a comment, though. Maybe I should check if regs->rsp is within 40 bytes of the bottom of the stack, too, such that delivery of an inner fault would have double-faulted assuming the inner fault didn't use an IST vector.

How about:

/*
* If we overflow the stack into a guard page, the CPU will fail
* to deliver #PF and will send #DF instead. CR2 will contain
* the linear address of the second fault, which will be in the
* guard page below the bottom of the stack.
*
* We're limited to using heuristics here, since the CPU does
* not tell us what type of fault failed and, if the first fault
* wasn't a page fault, CR2 may contain stale garbage. To mostly
* rule out garbage, we check if the saved RSP is close enough to
* the bottom of the stack to cause exception delivery to fail.
* The worst case is 7 stack slots: one for alignment, five for
* SS..RIP, and one for the error code.
*/
tsk_stack = (unsigned long)task_stack_page(tsk);

Quoted 8 lines

if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
/* A double-fault due to #PF delivery failure is plausible. */ cr2 = read_cr2();
if (tsk_stack - 1 - cr2 < PAGE_SIZE)
handle_stack_overflow(
"kernel stack overflow (double-fault)",
regs, cr2);
}

I think RSP anywhere in the guard page would be best, since it could have been decremented by a function prologue into the guard page before an access that triggers the page fault.

Quoted 74 lines · Brian Gerst

Quoted 64 lines · Andy Lutomirski

Quoted 36 lines · Andy Lutomirski

Quoted 35 lines · Brian Gerst

Quoted 33 lines · Brian Gerst

Quoted 19 lines · Andy Lutomirski

#ifdef CONFIG_X86_64
/* Runs on IST stack */
dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)

 {
        static const char str[] = "double fault";
        struct task_struct *tsk = current;

(quoted diff) +6 -0

+#ifdef CONFIG_VMAP_STACK
+       unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
        extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        tsk->thread.error_code = error_code;
        tsk->thread.trap_nr = X86_TRAP_DF;
 
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we overflow the stack into a guard page, the CPU will fail

Review dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code) · line 365

Quoted 10 lines

(quoted diff) +10 -0

+        * to deliver #PF and will send #DF instead.  CR2 will contain
+        * the linear address of the second fault, which will be in the
+        * guard page below the bottom of the stack.
+        */
+       cr2 = read_cr2();
+       if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+               handle_stack_overflow(
+                       "kernel stack overflow (double-fault)",
+                       regs, cr2);
+#endif

Is there any other way to tell if this was from a page fault? If it wasn't a page fault then CR2 is undefined.

Quoted 9 lines

Quoted 3 lines

I guess it doesn't really matter, since the fault is fatal either way. The error message might be incorrect though.

It's at least worth a comment, though. Maybe I should check if regs->rsp is within 40 bytes of the bottom of the stack, too, such that delivery of an inner fault would have double-faulted assuming the inner fault didn't use an IST vector.

How about:

/*
* If we overflow the stack into a guard page, the CPU will fail
* to deliver #PF and will send #DF instead. CR2 will contain
* the linear address of the second fault, which will be in the
* guard page below the bottom of the stack.
*
* We're limited to using heuristics here, since the CPU does
* not tell us what type of fault failed and, if the first fault
* wasn't a page fault, CR2 may contain stale garbage. To mostly
* rule out garbage, we check if the saved RSP is close enough to
* the bottom of the stack to cause exception delivery to fail.
* The worst case is 7 stack slots: one for alignment, five for
* SS..RIP, and one for the error code.
*/
tsk_stack = (unsigned long)task_stack_page(tsk);

Quoted 8 lines

if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
/* A double-fault due to #PF delivery failure is plausible. */ cr2 = read_cr2();
if (tsk_stack - 1 - cr2 < PAGE_SIZE)
handle_stack_overflow(
"kernel stack overflow (double-fault)",
regs, cr2);
}

Quoted 4 lines

I think RSP anywhere in the guard page would be best, since it could have been decremented by a function prologue into the guard page before an access that triggers the page fault.

I think that can miss some stack overflows. Suppose that RSP points very close to the bottom of the stack and we take an unrelated fault. The CPU can fail to deliver that fault and we get a double fault instead. But I counted wrong, too. Do you like this version and its explanation?

/*
* If we overflow the stack into a guard page, the CPU will fail
* to deliver #PF and will send #DF instead. Similarly, if we
* take any non-IST exception while too close to the bottom of
* the stack, the processor will get a page fault while
* delivering the exception and will generate a double fault. *
* According to the SDM (footnote in 6.15 under "Interrupt 14 -
* Page-Fault Exception (#PF):
*
* Processors update CR2 whenever a page fault is detected. If a
* second page fault occurs while an earlier page fault is being
* deliv- ered, the faulting linear address of the second fault will
* overwrite the contents of CR2 (replacing the previous
* address). These updates to CR2 occur even if the page fault
* results in a double fault or occurs during the delivery of a
* double fault.
*
* However, if we got here due to a non-page-fault exception while
* delivering a non-page-fault exception, CR2 may contain a
* stale value.
*
* As a heuristic: we consider this double fault to be a stack
* overflow if CR2 points to the guard page and RSP is either
* in the guard page or close enough to the bottom of the stack *
* We're limited to using heuristics here, since the CPU does
* not tell us what type of fault failed and, if the first fault
* wasn't a page fault, CR2 may contain stale garbage. To
* mostly rule out garbage, we check if the saved RSP is close
* enough to the bottom of the stack to cause exception delivery
* to fail. If RSP == tsk_stack + 48 and we take an exception,
* the stack is already aligned and there will be enough room
* SS, RSP, RFLAGS, CS, RIP, and a possible error code. With
* any less space left, exception delivery could fail. */
tsk_stack = (unsigned long)task_stack_page(tsk);

if (regs->rsp < tsk_stack + 48 && regs->rsp > tsk_stack - PAGE_SIZE) {
/* A double-fault due to #PF delivery failure is plausible. */ cr2 = read_cr2();
if (tsk_stack - 1 - cr2 < PAGE_SIZE)
handle_stack_overflow(
"kernel stack overflow (double-fault)",
regs, cr2);
}

Quoted 123 lines · Andy Lutomirski

Quoted 74 lines · Brian Gerst

Quoted 64 lines · Andy Lutomirski

Quoted 36 lines · Andy Lutomirski

Quoted 35 lines · Brian Gerst

Quoted 33 lines · Brian Gerst

Quoted 19 lines · Andy Lutomirski

#ifdef CONFIG_X86_64
/* Runs on IST stack */
dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)

 {
        static const char str[] = "double fault";
        struct task_struct *tsk = current;

(quoted diff) +6 -0

+#ifdef CONFIG_VMAP_STACK
+       unsigned long cr2;
+#endif
 
 #ifdef CONFIG_X86_ESPFIX64
        extern unsigned char native_irq_return_iret[];

@@ -332,6 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        tsk->thread.error_code = error_code;
        tsk->thread.trap_nr = X86_TRAP_DF;
 
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If we overflow the stack into a guard page, the CPU will fail

Review dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code) · line 365

Quoted 10 lines

(quoted diff) +10 -0

+        * to deliver #PF and will send #DF instead.  CR2 will contain
+        * the linear address of the second fault, which will be in the
+        * guard page below the bottom of the stack.
+        */
+       cr2 = read_cr2();
+       if ((unsigned long)tsk->stack - 1 - cr2 < PAGE_SIZE)
+               handle_stack_overflow(
+                       "kernel stack overflow (double-fault)",
+                       regs, cr2);
+#endif

Is there any other way to tell if this was from a page fault? If it wasn't a page fault then CR2 is undefined.

Quoted 9 lines

Quoted 3 lines

I guess it doesn't really matter, since the fault is fatal either way. The error message might be incorrect though.

It's at least worth a comment, though. Maybe I should check if regs->rsp is within 40 bytes of the bottom of the stack, too, such that delivery of an inner fault would have double-faulted assuming the inner fault didn't use an IST vector.

How about:

/*
* If we overflow the stack into a guard page, the CPU will fail
* to deliver #PF and will send #DF instead. CR2 will contain
* the linear address of the second fault, which will be in the
* guard page below the bottom of the stack.
*
* We're limited to using heuristics here, since the CPU does
* not tell us what type of fault failed and, if the first fault
* wasn't a page fault, CR2 may contain stale garbage. To mostly
* rule out garbage, we check if the saved RSP is close enough to
* the bottom of the stack to cause exception delivery to fail.
* The worst case is 7 stack slots: one for alignment, five for
* SS..RIP, and one for the error code.
*/
tsk_stack = (unsigned long)task_stack_page(tsk);

Quoted 8 lines

if (regs->rsp <= tsk_stack + 7*8 && regs->rsp > tsk_stack - PAGE_SIZE) {
/* A double-fault due to #PF delivery failure is plausible. */ cr2 = read_cr2();
if (tsk_stack - 1 - cr2 < PAGE_SIZE)
handle_stack_overflow(
"kernel stack overflow (double-fault)",
regs, cr2);
}

Quoted 4 lines

I think RSP anywhere in the guard page would be best, since it could have been decremented by a function prologue into the guard page before an access that triggers the page fault.

I think that can miss some stack overflows. Suppose that RSP points very close to the bottom of the stack and we take an unrelated fault. The CPU can fail to deliver that fault and we get a double fault instead. But I counted wrong, too. Do you like this version and its explanation?

/*
* If we overflow the stack into a guard page, the CPU will fail
* to deliver #PF and will send #DF instead. Similarly, if we
* take any non-IST exception while too close to the bottom of
* the stack, the processor will get a page fault while
* delivering the exception and will generate a double fault. *
* According to the SDM (footnote in 6.15 under "Interrupt 14 -
* Page-Fault Exception (#PF):
*
* Processors update CR2 whenever a page fault is detected. If a
* second page fault occurs while an earlier page fault is being
* deliv- ered, the faulting linear address of the second fault will
* overwrite the contents of CR2 (replacing the previous
* address). These updates to CR2 occur even if the page fault
* results in a double fault or occurs during the delivery of a
* double fault.
*
* However, if we got here due to a non-page-fault exception while
* delivering a non-page-fault exception, CR2 may contain a
* stale value.
*
* As a heuristic: we consider this double fault to be a stack
* overflow if CR2 points to the guard page and RSP is either
* in the guard page or close enough to the bottom of the stack *
* We're limited to using heuristics here, since the CPU does
* not tell us what type of fault failed and, if the first fault
* wasn't a page fault, CR2 may contain stale garbage. To
* mostly rule out garbage, we check if the saved RSP is close
* enough to the bottom of the stack to cause exception delivery
* to fail. If RSP == tsk_stack + 48 and we take an exception,
* the stack is already aligned and there will be enough room
* SS, RSP, RFLAGS, CS, RIP, and a possible error code. With
* any less space left, exception delivery could fail. */
tsk_stack = (unsigned long)task_stack_page(tsk);

Quoted 8 lines

if (regs->rsp < tsk_stack + 48 && regs->rsp > tsk_stack - PAGE_SIZE) {
/* A double-fault due to #PF delivery failure is plausible. */ cr2 = read_cr2();
if (tsk_stack - 1 - cr2 < PAGE_SIZE)
handle_stack_overflow(
"kernel stack overflow (double-fault)",
regs, cr2);
}