diff mbox series

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

Message ID 20221104223604.29615-19-rick.p.edgecombe@intel.com (mailing list archive)
State New
Headers show
Series Shadow stacks for userspace | expand

Commit Message

Edgecombe, Rick P Nov. 4, 2022, 10:35 p.m. UTC 
From: Yu-cheng Yu <yu-cheng.yu@intel.com>

The x86 Control-flow Enforcement Technology (CET) feature includes a new
type of memory called shadow stack. This shadow stack memory has some
unusual properties, which requires some core mm changes to function
properly.

The architecture of shadow stack constrains the ability of userspace to
move the shadow stack pointer (SSP) in order to  prevent corrupting or
switching to other shadow stacks. The RSTORSSP can move the spp to
different shadow stacks, but it requires a specially placed token in order
to do this. However, the architecture does not prevent incrementing the
stack pointer to wander onto an adjacent shadow stack. To prevent this in
software, enforce guard pages at the beginning of shadow stack vmas, such
that there will always be a gap between adjacent shadow stacks.

Make the gap big enough so that no userspace SSP changing operations
(besides RSTORSSP), can move the SSP from one stack to the next. The
SSP can increment or decrement by CALL, RET  and INCSSP. CALL and RET
can move the SSP by a maximum of 8 bytes, at which point the shadow
stack would be accessed.

The INCSSP instruction can also increment the shadow stack pointer. It
is the shadow stack analog of an instruction like:

	addq    $0x80, %rsp

However, there is one important difference between an ADD on %rsp and
INCSSP. In addition to modifying SSP, INCSSP also reads from the memory
of the first and last elements that were "popped". It can be thought of
as acting like this:

READ_ONCE(ssp);       // read+discard top element on stack
ssp += nr_to_pop * 8; // move the shadow stack
READ_ONCE(ssp-8);     // read+discard last popped stack element

The maximum distance INCSSP can move the SSP is 2040 bytes, before it
would read the memory. Therefore a single page gap will be enough to
prevent any operation from shifting the SSP to an adjacent stack, since
it would have to land in the gap at least once, causing a fault.

This could be accomplished by using VM_GROWSDOWN, but this has a
downside. The behavior would allow shadow stack's to grow, which is
unneeded and adds a strange difference to how most regular stacks work.

Tested-by: Pengfei Xu <pengfei.xu@intel.com>
Tested-by: John Allen <john.allen@amd.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Kees Cook <keescook@chromium.org>

---

v2:
 - Use __weak instead of #ifdef (Dave Hansen)
 - Only have start gap on shadow stack (Andy Luto)
 - Create stack_guard_start_gap() to not duplicate code
   in an arch version of vm_start_gap() (Dave Hansen)
 - Improve commit log partly with verbiage from (Dave Hansen)

Yu-cheng v25:
 - Move SHADOW_STACK_GUARD_GAP to arch/x86/mm/mmap.c.

Yu-cheng v24:
 - Instead changing vm_*_gap(), create x86-specific versions.

 arch/x86/mm/mmap.c | 23 +++++++++++++++++++++++
 include/linux/mm.h | 11 ++++++-----
 mm/mmap.c          |  7 +++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

Comments

Peter Zijlstra Nov. 15, 2022, 12:04 p.m. UTC | #1 
On Fri, Nov 04, 2022 at 03:35:45PM -0700, Rick Edgecombe wrote:

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

> +	 * 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?
Edgecombe, Rick P Nov. 15, 2022, 8:40 p.m. UTC | #2 
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:
> 
> > 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.

> 
> > +      * 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.
Peter Zijlstra Nov. 15, 2022, 8:56 p.m. UTC | #3 
On Tue, Nov 15, 2022 at 08:40:19PM +0000, Edgecombe, Rick P wrote:
> > > +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.

That was kinda the point, __weak sucks and this is very small in any
case.
Edgecombe, Rick P Nov. 15, 2022, 9:49 p.m. UTC | #4 
On Tue, 2022-11-15 at 21:56 +0100, Peter Zijlstra wrote:
> On Tue, Nov 15, 2022 at 08:40:19PM +0000, Edgecombe, Rick P wrote:
> > > > +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.
> 
> That was kinda the point, __weak sucks and this is very small in any
> case.

__weak was suggested here:

https://lore.kernel.org/lkml/f92c5110-7d97-b68d-d387-7e6a16a29e49@intel.com/

Let me try to put in cross arch code again like the other suggestion
was. I can't remember the reason why I didn't do it.
diff mbox series

Patch

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).
+	 * 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;
+}
+
 #if defined(CONFIG_DEBUG_VM_MAPLE_TREE)
 extern void mt_validate(struct maple_tree *mt);
 extern void mt_dump(const struct maple_tree *mt);