Re: [PATCH v6 19/41] x86/mm: Check shadow stack page fault errors

[David Hildenbrand <david@redhat.com>]

On 18.02.23 22:14, Rick Edgecombe wrote:
> From: Yu-cheng Yu <yu-cheng.yu@intel.com>
> 
> The CPU performs "shadow stack accesses" when it expects to encounter
> shadow stack mappings. These accesses can be implicit (via CALL/RET
> instructions) or explicit (instructions like WRSS).
> 
> Shadow stack accesses to shadow-stack mappings can result in faults in
> normal, valid operation just like regular accesses to regular mappings.
> Shadow stacks need some of the same features like delayed allocation, swap
> and copy-on-write. The kernel needs to use faults to implement those
> features.
> 
> The architecture has concepts of both shadow stack reads and shadow stack
> writes. Any shadow stack access to non-shadow stack memory will generate
> a fault with the shadow stack error code bit set.
> 
> This means that, unlike normal write protection, the fault handler needs
> to create a type of memory that can be written to (with instructions that
> generate shadow stack writes), even to fulfill a read access. So in the
> case of COW memory, the COW needs to take place even with a shadow stack
> read. Otherwise the page will be left (shadow stack) writable in
> userspace. So to trigger the appropriate behavior, set FAULT_FLAG_WRITE
> for shadow stack accesses, even if the access was a shadow stack read.
> 
> For the purpose of making this clearer, consider the following example.
> If a process has a shadow stack, and forks, the shadow stack PTEs will
> become read-only due to COW. If the CPU in one process performs a shadow
> stack read access to the shadow stack, for example executing a RET and
> causing the CPU to read the shadow stack copy of the return address, then
> in order for the fault to be resolved the PTE will need to be set with
> shadow stack permissions. But then the memory would be changeable from
> userspace (from CALL, RET, WRSS, etc). So this scenario needs to trigger
> COW, otherwise the shared page would be changeable from both processes.
> 
> Shadow stack accesses can also result in errors, such as when a shadow
> stack overflows, or if a shadow stack access occurs to a non-shadow-stack
> mapping. Also, generate the errors for invalid shadow stack accesses.
> 
> 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>
> 
> ---
> v6:
>   - Update comment due to rename of Cow bit to SavedDirty
> 
> v5:
>   - Add description of COW example (Boris)
>   - Replace "permissioned" (Boris)
>   - Remove capitalization of shadow stack (Boris)
> 
> v4:
>   - Further improve comment talking about FAULT_FLAG_WRITE (Peterz)
> 
> v3:
>   - Improve comment talking about using FAULT_FLAG_WRITE (Peterz)
> ---
>   arch/x86/include/asm/trap_pf.h |  2 ++
>   arch/x86/mm/fault.c            | 38 ++++++++++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
> 
> diff --git a/arch/x86/include/asm/trap_pf.h b/arch/x86/include/asm/trap_pf.h
> index 10b1de500ab1..afa524325e55 100644
> --- a/arch/x86/include/asm/trap_pf.h
> +++ b/arch/x86/include/asm/trap_pf.h
> @@ -11,6 +11,7 @@
>    *   bit 3 ==				1: use of reserved bit detected
>    *   bit 4 ==				1: fault was an instruction fetch
>    *   bit 5 ==				1: protection keys block access
> + *   bit 6 ==				1: shadow stack access fault
>    *   bit 15 ==				1: SGX MMU page-fault
>    */
>   enum x86_pf_error_code {
> @@ -20,6 +21,7 @@ enum x86_pf_error_code {
>   	X86_PF_RSVD	=		1 << 3,
>   	X86_PF_INSTR	=		1 << 4,
>   	X86_PF_PK	=		1 << 5,
> +	X86_PF_SHSTK	=		1 << 6,
>   	X86_PF_SGX	=		1 << 15,
>   };
>   
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 7b0d4ab894c8..42885d8e2036 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1138,8 +1138,22 @@ access_error(unsigned long error_code, struct vm_area_struct *vma)
>   				       (error_code & X86_PF_INSTR), foreign))
>   		return 1;
>   
> +	/*
> +	 * Shadow stack accesses (PF_SHSTK=1) are only permitted to
> +	 * shadow stack VMAs. All other accesses result in an error.
> +	 */
> +	if (error_code & X86_PF_SHSTK) {
> +		if (unlikely(!(vma->vm_flags & VM_SHADOW_STACK)))
> +			return 1;
> +		if (unlikely(!(vma->vm_flags & VM_WRITE)))
> +			return 1;
> +		return 0;
> +	}
> +
>   	if (error_code & X86_PF_WRITE) {
>   		/* write, present and write, not present: */
> +		if (unlikely(vma->vm_flags & VM_SHADOW_STACK))
> +			return 1;
>   		if (unlikely(!(vma->vm_flags & VM_WRITE)))
>   			return 1;
>   		return 0;
> @@ -1331,6 +1345,30 @@ void do_user_addr_fault(struct pt_regs *regs,
>   
>   	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
>   
> +	/*
> +	 * When a page becomes COW it changes from a shadow stack permission
> +	 * page (Write=0,Dirty=1) to (Write=0,Dirty=0,SavedDirty=1), which is simply
> +	 * read-only to the CPU. When shadow stack is enabled, a RET would
> +	 * normally pop the shadow stack by reading it with a "shadow stack
> +	 * read" access. However, in the COW case the shadow stack memory does
> +	 * not have shadow stack permissions, it is read-only. So it will
> +	 * generate a fault.
> +	 *
> +	 * For conventionally writable pages, a read can be serviced with a
> +	 * read only PTE, and COW would not have to happen. But for shadow
> +	 * stack, there isn't the concept of read-only shadow stack memory.
> +	 * If it is shadow stack permission, it can be modified via CALL and
> +	 * RET instructions. So COW needs to happen before any memory can be
> +	 * mapped with shadow stack permissions.
> +	 *
> +	 * Shadow stack accesses (read or write) need to be serviced with
> +	 * shadow stack permission memory, so in the case of a shadow stack
> +	 * read access, treat it as a WRITE fault so both COW will happen and
> +	 * the write fault path will tickle maybe_mkwrite() and map the memory
> +	 * shadow stack.
> +	 */

Again, I suggest dropping all details about COW from this comment and 
from the patch description. It's just one such case that can happen.


-- 
Thanks,

David / dhildenb