[v7,21/41] mm: Add guard pages around a shadow stack.

  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 ssp 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>
Tested-by: Kees Cook <keescook@chromium.org>
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
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>

---
v5:
 - Fix typo in commit log

v4:
 - Drop references to 32 bit instructions
 - Switch to generic code to drop __weak (Peterz)

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.
---
 include/linux/mm.h | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
  

Just typos:

On Mon, Feb 27, 2023 at 02:29:37PM -0800, Rick Edgecombe wrote:
> 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 ssp to
						^
						instruction

s/ssp/SSP/g


> 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

VMAs

> 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 be incremented or decremented"

> 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

s/stack's/stacks/

> 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>
> Tested-by: Kees Cook <keescook@chromium.org>
> Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
> 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>
> 
> ---
> v5:
>  - Fix typo in commit log
> 
> v4:
>  - Drop references to 32 bit instructions
>  - Switch to generic code to drop __weak (Peterz)
> 
> 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.
> ---
>  include/linux/mm.h | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 097544afb1aa..6a093daced88 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3107,15 +3107,36 @@ struct vm_area_struct *vma_lookup(struct mm_struct *mm, unsigned long addr)
>  	return mtree_load(&mm->mm_mt, addr);
>  }
>  
> +static inline 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 INCSSPQ.
> +	 * INCSSPQ moves shadow stack pointer up to 255 * 8 = ~2 KB
> +	 * 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.

I'd prefer the equivalant explanation above from the commit message - it
is more precise.

> +	 *
> +	 * 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;
> +}
  

On Mon, 2023-03-06 at 09:08 +0100, Borislav Petkov wrote:
> Just typos:

All seem reasonable to me. Thanks. 

For using the log verbiage for the comment, it is quite big. Does
something like this seem reasonable?

/*
 * The shadow stack pointer(SSP) is moved by CALL, RET, and INCSSPQ.
 * The INCSSP instruction can 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.
 *
 * Prevent using INCSSP to move the SSP between shadow stacks by
 * having a PAGE_SIZE gaurd gap.
 */
  

On Tue, Mar 07, 2023 at 01:29:50AM +0000, Edgecombe, Rick P wrote:
> On Mon, 2023-03-06 at 09:08 +0100, Borislav Petkov wrote:
> > Just typos:
> 
> All seem reasonable to me. Thanks. 
> 
> For using the log verbiage for the comment, it is quite big. Does
> something like this seem reasonable?

Yeah, it does. I wouldn't want to lose that explanation in a commit
message.

However, this special aspect pertains to the shstk implementation in x86
but the code is generic mm and such arch-specific comments are kinda
unfitting there.

I wonder if it would be better if you could stick that explanation
somewhere in arch/x86/ and only refer to it in a short comment above
VM_SHADOW_STACK check in stack_guard_start_gap()...

Thx.
  

On 07.03.23 11:32, Borislav Petkov wrote:
> On Tue, Mar 07, 2023 at 01:29:50AM +0000, Edgecombe, Rick P wrote:
>> On Mon, 2023-03-06 at 09:08 +0100, Borislav Petkov wrote:
>>> Just typos:
>>
>> All seem reasonable to me. Thanks.
>>
>> For using the log verbiage for the comment, it is quite big. Does
>> something like this seem reasonable?
> 
> Yeah, it does. I wouldn't want to lose that explanation in a commit
> message.
> 
> However, this special aspect pertains to the shstk implementation in x86
> but the code is generic mm and such arch-specific comments are kinda
> unfitting there.
> 
> I wonder if it would be better if you could stick that explanation
> somewhere in arch/x86/ and only refer to it in a short comment above
> VM_SHADOW_STACK check in stack_guard_start_gap()...

+1
  

On Tue, 2023-03-07 at 11:44 +0100, David Hildenbrand wrote:
> On 07.03.23 11:32, Borislav Petkov wrote:
> > On Tue, Mar 07, 2023 at 01:29:50AM +0000, Edgecombe, Rick P wrote:
> > > On Mon, 2023-03-06 at 09:08 +0100, Borislav Petkov wrote:
> > > > Just typos:
> > > 
> > > All seem reasonable to me. Thanks.
> > > 
> > > For using the log verbiage for the comment, it is quite big. Does
> > > something like this seem reasonable?
> > 
> > Yeah, it does. I wouldn't want to lose that explanation in a commit
> > message.
> > 
> > However, this special aspect pertains to the shstk implementation
> > in x86
> > but the code is generic mm and such arch-specific comments are
> > kinda
> > unfitting there.
> > 
> > I wonder if it would be better if you could stick that explanation
> > somewhere in arch/x86/ and only refer to it in a short comment
> > above
> > VM_SHADOW_STACK check in stack_guard_start_gap()...
> 
> +1

I can't find a good place for it in the arch code. Basically there is
no arch/x86 functionality that has to do with guard pages. The closest
is pte_mkwrite() because it at least references VM_SHADOW_STACK but it
doesn't really fit.

We could to add an arch version of stack_guard_start_gap() but we had
that and removed it for other style reasons. Code duplication IIRC.

So I thought to just move it elsewhere in mm.h where VM_SHADOW_STACK is
defined.
  

On Mon, Feb 27, 2023 at 2:31 PM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
>
> 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 ssp 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>
> Tested-by: Kees Cook <keescook@chromium.org>
> Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
> 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>
>
> ---
> v5:
>  - Fix typo in commit log
>
> v4:
>  - Drop references to 32 bit instructions
>  - Switch to generic code to drop __weak (Peterz)
>
> 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.
> ---
>  include/linux/mm.h | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 097544afb1aa..6a093daced88 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3107,15 +3107,36 @@ struct vm_area_struct *vma_lookup(struct mm_struct *mm, unsigned long addr)
>         return mtree_load(&mm->mm_mt, addr);
>  }
>
> +static inline 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 INCSSPQ.
> +        * INCSSPQ moves shadow stack pointer up to 255 * 8 = ~2 KB
> +        * 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;

This is an arch agnostic header file. Can we remove `VM_SHADOW_STACK`
from here? and instead
have `arch_is_shadow_stack` which consumes vma flags and returns true or false.
This allows different architectures to choose their own encoding of
vma flags to represent a shadow stack.

> +
> +       return 0;
> +}
> +
>  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;
>  }
>
> --
> 2.17.1
>