Messages in this thread | | | Subject | Re: [PATCH v23 14/28] x86/mm: Shadow Stack page fault error checking | From | "Yu, Yu-cheng" <> | Date | Mon, 22 Mar 2021 10:27:53 -0700 |
| |
On 3/22/2021 3:38 AM, Kirill A. Shutemov wrote: > On Tue, Mar 16, 2021 at 08:10:40AM -0700, Yu-cheng Yu wrote: [...] >> >> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c >> index a73347e2cdfc..4316732a18c6 100644 >> --- a/arch/x86/mm/fault.c >> +++ b/arch/x86/mm/fault.c >> @@ -1100,6 +1100,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. >> + */ >> + if (error_code & X86_PF_SHSTK) { >> + if (!(vma->vm_flags & VM_SHSTK)) >> + return 1; >> + return 0; > > Any reason to return 0 here? I would rather keep the single return 0 in > the function, after all checks are done. >
For shadow stack fault, X86_PF_SHSTK and X86_PF_WRITE both can be set. So for shadow stack fault, return from here and don't go into the normal write fault case.
Thanks, Yu-cheng
>> + } >> + >> if (error_code & X86_PF_WRITE) { >> /* write, present and write, not present: */ >> if (unlikely(!(vma->vm_flags & VM_WRITE))) >> @@ -1293,6 +1304,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. >> + */ >> + if (error_code & X86_PF_SHSTK) >> + flags |= FAULT_FLAG_WRITE; >> if (error_code & X86_PF_WRITE) >> flags |= FAULT_FLAG_WRITE; >> if (error_code & X86_PF_INSTR) >> -- >> 2.21.0 >> >
| |