Re: [PATCH v3 18/37] mm: Add guard pages around a shadow stack.

On Tue, 2022-11-15 at 13:04 +0100, Peter Zijlstra wrote:

On Fri, Nov 04, 2022 at 03:35:45PM -0700, Rick Edgecombe wrote:

quoted

diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index c90c20904a60..66da1f3298b0 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c

@@ -248,3 +248,26 @@ bool pfn_modify_allowed(unsigned long pfn,

pgprot_t prot)
               return false;
       return true;
  }
+
+unsigned long stack_guard_start_gap(struct vm_area_struct *vma)
+{
+     if (vma->vm_flags & VM_GROWSDOWN)
+             return stack_guard_gap;
+
+     /*
+      * Shadow stack pointer is moved by CALL, RET, and
INCSSP(Q/D).

Can we perhaps write this like: INCSPP[QD] ? The () notation makes it
look like a function.

Sure.

quoted

+      * INCSSPQ moves shadow stack pointer up to 255 * 8 = ~2 KB
+      * (~1KB for INCSSPD) and touches the first and the last
element
+      * in the range, which triggers a page fault if the range is
not
+      * in a shadow stack. Because of this, creating 4-KB guard
pages
+      * around a shadow stack prevents these instructions from
going
+      * beyond.
+      *
+      * Creation of VM_SHADOW_STACK is tightly controlled, so a
vma
+      * can't be both VM_GROWSDOWN and VM_SHADOW_STACK
+      */
+     if (vma->vm_flags & VM_SHADOW_STACK)
+             return PAGE_SIZE;
+
+     return 0;
+}

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5d9536fa860a..0a3f7e2b32df 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h

@@ -2832,15 +2832,16 @@ struct vm_area_struct *vma_lookup(struct

mm_struct *mm, unsigned long addr)
       return mtree_load(&mm->mm_mt, addr);
  }
  
+unsigned long stack_guard_start_gap(struct vm_area_struct *vma);
+
  static inline unsigned long vm_start_gap(struct vm_area_struct
*vma)
  {
+     unsigned long gap = stack_guard_start_gap(vma);
       unsigned long vm_start = vma->vm_start;
  
-     if (vma->vm_flags & VM_GROWSDOWN) {
-             vm_start -= stack_guard_gap;
-             if (vm_start > vma->vm_start)
-                     vm_start = 0;
-     }
+     vm_start -= gap;
+     if (vm_start > vma->vm_start)
+             vm_start = 0;
       return vm_start;
  }
  

diff --git a/mm/mmap.c b/mm/mmap.c
index 2def55555e05..f67606fbc464 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c

@@ -281,6 +281,13 @@ SYSCALL_DEFINE1(brk, unsigned long, brk)
       return origbrk;
  }
  
+unsigned long __weak stack_guard_start_gap(struct vm_area_struct

*vma)
+{
+     if (vma->vm_flags & VM_GROWSDOWN)
+             return stack_guard_gap;
+     return 0;
+}

I'm thinking perhaps this wants to be an inline function?

I don't think it can work with weak then.