| Message ID | 20230218211433.26859-28-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
On Sat, Feb 18, 2023 at 01:14:19PM -0800, Rick Edgecombe wrote: > When user shadow stack is use, Write=0,Dirty=1 is treated by the CPU as > shadow stack memory. So for shadow stack memory this bit combination is > valid, but when Dirty=1,Write=1 (conventionally writable) memory is being > write protected, the kernel has been taught to transition the Dirty=1 > bit to SavedDirty=1, to avoid inadvertently creating shadow stack > memory. It does this inside pte_wrprotect() because it knows the PTE is > not intended to be a writable shadow stack entry, it is supposed to be > write protected. > > However, when a PTE is created by a raw prot using mk_pte(), mk_pte() > can't know whether to adjust Dirty=1 to SavedDirty=1. It can't > distinguish between the caller intending to create a shadow stack PTE or > needing the SavedDirty shift. > > The kernel has been updated to not do this, and so Write=0,Dirty=1 > memory should only be created by the pte_mkfoo() helpers. Add a warning > to make sure no new mk_pte() start doing this. > > Tested-by: Pengfei Xu <pengfei.xu@intel.com> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > > --- > v6: > - New patch (Note, this has already been a useful warning, it caught the > newly added set_memory_rox() doing this) > --- > arch/x86/include/asm/pgtable.h | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h > index f3dc16fc4389..db8fe5511c74 100644 > --- a/arch/x86/include/asm/pgtable.h > +++ b/arch/x86/include/asm/pgtable.h > @@ -1032,7 +1032,15 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd) > * (Currently stuck as a macro because of indirect forward reference > * to linux/mm.h:page_to_nid()) > */ > -#define mk_pte(page, pgprot) pfn_pte(page_to_pfn(page), (pgprot)) > +#define mk_pte(page, pgprot) \ > +({ \ > + pgprot_t __pgprot = pgprot; \ > + \ > + WARN_ON_ONCE(cpu_feature_enabled(X86_FEATURE_USER_SHSTK) && \ > + (pgprot_val(__pgprot) & (_PAGE_DIRTY | _PAGE_RW)) == \ > + _PAGE_DIRTY); \ > + pfn_pte(page_to_pfn(page), __pgprot); \ > +}) This only warns? Should it also enforce the state?
On Sun, 2023-02-19 at 12:45 -0800, Kees Cook wrote: > > diff --git a/arch/x86/include/asm/pgtable.h > > b/arch/x86/include/asm/pgtable.h > > index f3dc16fc4389..db8fe5511c74 100644 > > --- a/arch/x86/include/asm/pgtable.h > > +++ b/arch/x86/include/asm/pgtable.h > > @@ -1032,7 +1032,15 @@ static inline unsigned long > > pmd_page_vaddr(pmd_t pmd) > > * (Currently stuck as a macro because of indirect forward > > reference > > * to linux/mm.h:page_to_nid()) > > */ > > -#define mk_pte(page, pgprot) pfn_pte(page_to_pfn(page), > > (pgprot)) > > +#define mk_pte(page, > > pgprot) \ > > +({ > > \ > > + pgprot_t __pgprot = > > pgprot; \ > > + > > \ > > + WARN_ON_ONCE(cpu_feature_enabled(X86_FEATURE_USER_SHSTK) > > && \ > > + (pgprot_val(__pgprot) & (_PAGE_DIRTY | _PAGE_RW)) > > == \ > > + > > _PAGE_DIRTY); \ > > + pfn_pte(page_to_pfn(page), > > __pgprot); \ > > +}) > > This only warns? Should it also enforce the state? Hmm, you mean something like forcing Dirty=0 if Write=0? The thing we are worried about here is some new x86 code that creates Write=0,Dirty=1 PTEs directly because the developer is unaware or forgot about shadow stack. The issue the warning actually caught was kernel memory being marked Write=0,Dirty=1, which today is more about consistency than any functional issue. But if some future hypothetical code was creating a userspace PTE like this, and depending on the memory being read-only, then the enforcement would be useful and potentially save the day. The downside is that it adds tricky logic into a low level helper that shouldn't be required unless strange and wrong new code is added in the future. And then it is still only useful if the warning doesn't catch the issue in testing. And then there would be some slight risk that the Dirty bit was expected to be there in some PTE without shadow stack exposure, and a functional bug would be introduced. I'm waffling here. I could be convinced either way. Hopefully that helps characterize the dilemma at least.
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h index f3dc16fc4389..db8fe5511c74 100644 --- a/arch/x86/include/asm/pgtable.h +++ b/arch/x86/include/asm/pgtable.h @@ -1032,7 +1032,15 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd) * (Currently stuck as a macro because of indirect forward reference * to linux/mm.h:page_to_nid()) */ -#define mk_pte(page, pgprot) pfn_pte(page_to_pfn(page), (pgprot)) +#define mk_pte(page, pgprot) \ +({ \ + pgprot_t __pgprot = pgprot; \ + \ + WARN_ON_ONCE(cpu_feature_enabled(X86_FEATURE_USER_SHSTK) && \ + (pgprot_val(__pgprot) & (_PAGE_DIRTY | _PAGE_RW)) == \ + _PAGE_DIRTY); \ + pfn_pte(page_to_pfn(page), __pgprot); \ +}) static inline int pmd_bad(pmd_t pmd) {