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

[Dave Hansen <dave.hansen@intel.com>]

On 1/30/22 13:18, Rick Edgecombe wrote:
> INCSSP(Q/D) increments shadow stack pointer and 'pops and discards' the
> first and the last elements in the range, effectively touches those memory
> areas.

This is a pretty close copy of the instruction reference text for
INCSSP.  I'm feeling rather dense today, but that's just not making any
sense.

The pseudocode is more sensible in the SDM.  I think this needs a better
explanation:

	The INCSSP instruction increments 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".  You can think of it 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 moving distance by INCSSPQ is 255 * 8 = 2040 bytes and
> 255 * 4 = 1020 bytes by INCSSPD.  Both ranges are far from PAGE_SIZE.

... That maximum distance, combined with an a guard pages at the end of
a shadow stack ensures that INCSSP will fault before it is able to move
across an entire guard page.

> Thus, putting a gap page on both ends of a shadow stack prevents INCSSP,
> CALL, and RET from going beyond.

> 
> diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
> index a506a411474d..e1533fdc08b4 100644
> --- a/arch/x86/include/asm/page_types.h
> +++ b/arch/x86/include/asm/page_types.h
> @@ -73,6 +73,13 @@ bool pfn_range_is_mapped(unsigned long start_pfn, unsigned long end_pfn);
>  
>  extern void initmem_init(void);
>  
> +#define vm_start_gap vm_start_gap
> +struct vm_area_struct;
> +extern unsigned long vm_start_gap(struct vm_area_struct *vma);
> +
> +#define vm_end_gap vm_end_gap
> +extern unsigned long vm_end_gap(struct vm_area_struct *vma);
> +
>  #endif	/* !__ASSEMBLY__ */
>  
>  #endif	/* _ASM_X86_PAGE_DEFS_H */
> diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
> index f3f52c5e2fd6..81f9325084d3 100644
> --- a/arch/x86/mm/mmap.c
> +++ b/arch/x86/mm/mmap.c
> @@ -250,3 +250,49 @@ bool pfn_modify_allowed(unsigned long pfn, pgprot_t prot)
>  		return false;
>  	return true;
>  }
> +
> +/*
> + * 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.
> + */
> +#define SHADOW_STACK_GUARD_GAP PAGE_SIZE
> +
> +unsigned long vm_start_gap(struct vm_area_struct *vma)
> +{
> +	unsigned long vm_start = vma->vm_start;
> +	unsigned long gap = 0;
> +
> +	if (vma->vm_flags & VM_GROWSDOWN)
> +		gap = stack_guard_gap;
> +	else if (vma->vm_flags & VM_SHADOW_STACK)
> +		gap = SHADOW_STACK_GUARD_GAP;
> +
> +	if (gap != 0) {
> +		vm_start -= gap;
> +		if (vm_start > vma->vm_start)
> +			vm_start = 0;
> +	}
> +	return vm_start;
> +}
> +
> +unsigned long vm_end_gap(struct vm_area_struct *vma)
> +{
> +	unsigned long vm_end = vma->vm_end;
> +	unsigned long gap = 0;
> +
> +	if (vma->vm_flags & VM_GROWSUP)
> +		gap = stack_guard_gap;
> +	else if (vma->vm_flags & VM_SHADOW_STACK)
> +		gap = SHADOW_STACK_GUARD_GAP;
> +
> +	if (gap != 0) {
> +		vm_end += gap;
> +		if (vm_end < vma->vm_end)
> +			vm_end = -PAGE_SIZE;
> +	}
> +	return vm_end;
> +}

First of all, __weak would be a lot better than these #ifdefs.

Second, I have the same basic objection to this as the maybe_mkwrite()
mess.  This is a forked copy of the code.  Instead of refactoring, it's
just copied-pasted-and-#ifdef'd.  Not so nice.

Isn't this just a matter of overriding 'stack_guard_gap' for
VM_SHADOW_STACK?  Why don't we just do this:

unsigned long stack_guard_gap(struct vm_area_struct *vma)
{
	if (vma->vm_flags & VM_SHADOW_STACK)
		return SHADOW_STACK_GUARD_GAP;

	return __stack_guard_gap;
}

Or, worst-case if people don't want 2 easily compiled-out lines added to
generic code, define:

unsigned long __weak stack_guard_gap(struct vm_area_struct *vma)
{
	return __stack_guard_gap;
}

in generic code, and put the top definition in arch/x86.