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.
