| Message ID | 20180607143705.3531-8-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > If a page fault is triggered by a shadow stack access (e.g. > call/ret) or shadow stack management instructions (e.g. > wrussq), then bit[6] of the page fault error code is set. > > In access_error(), we check if a shadow stack page fault > is within a shadow stack memory area. > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > index 73bd8c95ac71..2b3b9170109c 100644 > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -1166,6 +1166,17 @@ access_error(unsigned long error_code, struct vm_area_struct *vma) > (error_code & X86_PF_INSTR), foreign)) > return 1; > > + /* > + * Verify X86_PF_SHSTK is within a shadow stack VMA. > + * It is always an error if there is a shadow stack > + * fault outside a shadow stack VMA. > + */ > + if (error_code & X86_PF_SHSTK) { > + if (!(vma->vm_flags & VM_SHSTK)) > + return 1; > + return 0; > + } > + What, if anything, would go wrong without this change? It seems like it might be purely an optimization. If so, can you mention that in the comment?
On Thu, 2018-06-07 at 09:26 -0700, Andy Lutomirski wrote: > On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: > > > > If a page fault is triggered by a shadow stack access (e.g. > > call/ret) or shadow stack management instructions (e.g. > > wrussq), then bit[6] of the page fault error code is set. > > > > In access_error(), we check if a shadow stack page fault > > is within a shadow stack memory area. > > > > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> > > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > > index 73bd8c95ac71..2b3b9170109c 100644 > > --- a/arch/x86/mm/fault.c > > +++ b/arch/x86/mm/fault.c > > @@ -1166,6 +1166,17 @@ access_error(unsigned long error_code, struct vm_area_struct *vma) > > (error_code & X86_PF_INSTR), foreign)) > > return 1; > > > > + /* > > + * Verify X86_PF_SHSTK is within a shadow stack VMA. > > + * It is always an error if there is a shadow stack > > + * fault outside a shadow stack VMA. > > + */ > > + if (error_code & X86_PF_SHSTK) { > > + if (!(vma->vm_flags & VM_SHSTK)) > > + return 1; > > + return 0; > > + } > > + > > What, if anything, would go wrong without this change? It seems like > it might be purely an optimization. If so, can you mention that in > the comment? Without this check, the page fault code could overlook the fact that the application is trying to use non shadow stack area for shadow stack. I will add this to the comments.
On 06/07/2018 09:26 AM, Andy Lutomirski wrote: >> >> + /* >> + * Verify X86_PF_SHSTK is within a shadow stack VMA. >> + * It is always an error if there is a shadow stack >> + * fault outside a shadow stack VMA. >> + */ >> + if (error_code & X86_PF_SHSTK) { >> + if (!(vma->vm_flags & VM_SHSTK)) >> + return 1; >> + return 0; >> + } >> + > What, if anything, would go wrong without this change? It seems like > it might be purely an optimization. If so, can you mention that in > the comment? This is a fine exercise. I'm curious what it does, too. But, I really like it being explicit in the end. If we depend on implicit behavior, I really worry that someone breaks it accidentally.
diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h index 5196050ff3d5..58ea2f5722e9 100644 --- a/arch/x86/include/asm/traps.h +++ b/arch/x86/include/asm/traps.h @@ -157,6 +157,7 @@ enum { * 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 */ enum x86_pf_error_code { X86_PF_PROT = 1 << 0, @@ -165,5 +166,6 @@ 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, }; #endif /* _ASM_X86_TRAPS_H */ diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index 73bd8c95ac71..2b3b9170109c 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -1166,6 +1166,17 @@ access_error(unsigned long error_code, struct vm_area_struct *vma) (error_code & X86_PF_INSTR), foreign)) return 1; + /* + * Verify X86_PF_SHSTK is within a shadow stack VMA. + * It is always an error if there is a shadow stack + * fault outside a shadow stack VMA. + */ + if (error_code & X86_PF_SHSTK) { + if (!(vma->vm_flags & VM_SHSTK)) + return 1; + return 0; + } + if (error_code & X86_PF_WRITE) { /* write, present and write, not present: */ if (unlikely(!(vma->vm_flags & VM_WRITE)))
If a page fault is triggered by a shadow stack access (e.g. call/ret) or shadow stack management instructions (e.g. wrussq), then bit[6] of the page fault error code is set. In access_error(), we check if a shadow stack page fault is within a shadow stack memory area. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- arch/x86/include/asm/traps.h | 2 ++ arch/x86/mm/fault.c | 11 +++++++++++ 2 files changed, 13 insertions(+)