Re: [PATCH 15/35] x86/mm: Check Shadow Stack page fault errors
From: Dave Hansen <hidden>
Date: 2022-02-09 19:07:20
Also in:
linux-arch, linux-doc, linux-mm, lkml
On 1/30/22 13:18, Rick Edgecombe wrote:
From: Yu-cheng Yu <redacted> Shadow stack accesses are those that are performed by the CPU where it expects to encounter a shadow stack mapping. These accesses are performed implicitly by CALL/RET at the site of the shadow stack pointer. These accesses are made explicitly by shadow stack management instructions like WRUSSQ.
The passive voice is killing me. Here's a rewrite: 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 WRUSSQ). Since we defined what a shadow stack access *is*, shouldn't we also connect it to X86_PF_SHSTK?
Shadow stacks accesses to shadow-stack mapping can see faults in normal,
^ mappings
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.
... and use faults to implement those features.
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.
Those two paragraphs tell a pretty good story. Nice.
In handling a shadow stack page fault, verify it occurs within a shadow stack mapping. It is always an error otherwise. For valid shadow stack accesses, set FAULT_FLAG_WRITE to effect copy-on-write. Because clearing _PAGE_DIRTY (vs. _PAGE_RW) is used to trigger the fault, shadow stack read fault and shadow stack write fault are not differentiated and both are handled as a write access.
This paragraph is a rehash of what the code does. It can go. *But*, with or without this paragraph, the reader is left with all background and no discussion of why this patch exists. Even just this would be fine: Handle valid and invalid shadow-stack accesses in the page fault handler.
quoted hunk ↗ jump to hunk
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 d0074c6ed31a..6769134986ec 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c@@ -1107,6 +1107,17 @@ access_error(unsigned long error_code, struct vm_area_struct *vma) (error_code & X86_PF_INSTR), foreign)) return 1; + /* + * Verify a shadow stack access is within a shadow stack VMA. + * It is always an error otherwise. Normal data access to a + * shadow stack area is checked in the case followed. + */
That comment needs some help. Maybe: Shadow stack accesses (PF_SHSTK=1) are only permitted to shadow stack VMAs. All other accesses result in an error. I don't think we need to talk about the other cases being handled below.
quoted hunk ↗ jump to hunk
+ if (error_code & X86_PF_SHSTK) { + if (!(vma->vm_flags & VM_SHADOW_STACK)) + return 1; + return 0; + } + if (error_code & X86_PF_WRITE) { /* write, present and write, not present: */ if (unlikely(!(vma->vm_flags & VM_WRITE)))@@ -1300,6 +1311,14 @@ void do_user_addr_fault(struct pt_regs *regs, perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address); + /* + * Clearing _PAGE_DIRTY is used to detect shadow stack access. + * This method cannot distinguish shadow stack read vs. write. + * For valid shadow stack accesses, set FAULT_FLAG_WRITE to effect + * copy-on-write. + */
Too much detail. This is also rather unconnected to the code I can see:
+ if (error_code & X86_PF_SHSTK) + flags |= FAULT_FLAG_WRITE;
Also, the use of "effect" here is arguably wrong. It's odd at best. I'd use some alternative wording. Let's stick to the facts: 1. Shadow stack pages architecturally can't be read-only 2. Don't bother with read faults, consider everything a write BTW, what happens if we don't do this? What breaks?