| Message ID | 20200205181935.3712-11-yu-cheng.yu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Control-flow Enforcement: Shadow Stack | expand |
The subject really needs work. Could you think of a way to summarize the changes here in english as opposed to just listing the symbols you modified? I think we could probably just auto-generate subjects for patches if the existing one were sufficient. On 2/5/20 10:19 AM, Yu-cheng Yu wrote: > After the introduction of _PAGE_DIRTY_SW, pte_modify and pmd_modify need to > set the Dirty bit accordingly: if Shadow Stack is enabled and _PAGE_RW is > cleared, use _PAGE_DIRTY_SW; otherwise _PAGE_DIRTY_HW. You've basically gone and written the code's if() statement in english here. That doesn't really help me understand the patch. > Since the Dirty bit is modify by pte_modify(), remove _PAGE_DIRTY_HW from > PAGE_CHG_MASK. ^ modified This is a great example of a changelog that adds very little value. It's following the comments and doing what they say, but it's pretty obvious that the analysis stopped there. What *kinds* of bits are in _PAGE_CHG_MASK or not? What changed about _PAGE_DIRTY_HW. By this definition, shouldn't _PAGE_DIRTY_SW have technically been in this mask before this patch? > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h > index 62aeb118bc36..2733e7ec16b3 100644 > --- a/arch/x86/include/asm/pgtable.h > +++ b/arch/x86/include/asm/pgtable.h > @@ -702,6 +702,14 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot) > val &= _PAGE_CHG_MASK; > val |= check_pgprot(newprot) & ~_PAGE_CHG_MASK; > val = flip_protnone_guard(oldval, val, PTE_PFN_MASK); > + > + if (pte_dirty(pte)) { > + if (static_cpu_has(X86_FEATURE_SHSTK) && !(val & _PAGE_RW)) > + val |= _PAGE_DIRTY_SW; > + else > + val |= _PAGE_DIRTY_HW; > + } > + > return __pte(val); > } OK, so this is a path we use for changing bunches of PTEs to 'newprot'. It doesn't use the pte_*() helpers that the previous patch fixed up, so we need a new site. Right? Maybe that would make good changelog text. Also, couldn't we just have a pte_fixup() function or something that did this logic and could be shared? > @@ -712,6 +720,14 @@ static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot) > val &= _HPAGE_CHG_MASK; > val |= check_pgprot(newprot) & ~_HPAGE_CHG_MASK; > val = flip_protnone_guard(oldval, val, PHYSICAL_PMD_PAGE_MASK); > + > + if (pmd_dirty(pmd)) { > + if (static_cpu_has(X86_FEATURE_SHSTK) && !(val & _PAGE_RW)) > + val |= _PAGE_DIRTY_SW; > + else > + val |= _PAGE_DIRTY_HW; > + } > + > return __pmd(val); > } > > diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h > index 826823df917f..e7e28bf7e919 100644 > --- a/arch/x86/include/asm/pgtable_types.h > +++ b/arch/x86/include/asm/pgtable_types.h > @@ -150,8 +150,8 @@ > * instance, and is *not* included in this mask since > * pte_modify() does modify it. > */ > -#define _PAGE_CHG_MASK (PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT | \ > - _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY_HW | \ > +#define _PAGE_CHG_MASK (PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT | \ > + _PAGE_SPECIAL | _PAGE_ACCESSED | \ > _PAGE_SOFT_DIRTY | _PAGE_DEVMAP) > #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h index 62aeb118bc36..2733e7ec16b3 100644 --- a/arch/x86/include/asm/pgtable.h +++ b/arch/x86/include/asm/pgtable.h @@ -702,6 +702,14 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot) val &= _PAGE_CHG_MASK; val |= check_pgprot(newprot) & ~_PAGE_CHG_MASK; val = flip_protnone_guard(oldval, val, PTE_PFN_MASK); + + if (pte_dirty(pte)) { + if (static_cpu_has(X86_FEATURE_SHSTK) && !(val & _PAGE_RW)) + val |= _PAGE_DIRTY_SW; + else + val |= _PAGE_DIRTY_HW; + } + return __pte(val); } @@ -712,6 +720,14 @@ static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot) val &= _HPAGE_CHG_MASK; val |= check_pgprot(newprot) & ~_HPAGE_CHG_MASK; val = flip_protnone_guard(oldval, val, PHYSICAL_PMD_PAGE_MASK); + + if (pmd_dirty(pmd)) { + if (static_cpu_has(X86_FEATURE_SHSTK) && !(val & _PAGE_RW)) + val |= _PAGE_DIRTY_SW; + else + val |= _PAGE_DIRTY_HW; + } + return __pmd(val); } diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h index 826823df917f..e7e28bf7e919 100644 --- a/arch/x86/include/asm/pgtable_types.h +++ b/arch/x86/include/asm/pgtable_types.h @@ -150,8 +150,8 @@ * instance, and is *not* included in this mask since * pte_modify() does modify it. */ -#define _PAGE_CHG_MASK (PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT | \ - _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY_HW | \ +#define _PAGE_CHG_MASK (PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT | \ + _PAGE_SPECIAL | _PAGE_ACCESSED | \ _PAGE_SOFT_DIRTY | _PAGE_DEVMAP) #define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
After the introduction of _PAGE_DIRTY_SW, pte_modify and pmd_modify need to set the Dirty bit accordingly: if Shadow Stack is enabled and _PAGE_RW is cleared, use _PAGE_DIRTY_SW; otherwise _PAGE_DIRTY_HW. Since the Dirty bit is modify by pte_modify(), remove _PAGE_DIRTY_HW from PAGE_CHG_MASK. Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> --- arch/x86/include/asm/pgtable.h | 16 ++++++++++++++++ arch/x86/include/asm/pgtable_types.h | 4 ++-- 2 files changed, 18 insertions(+), 2 deletions(-)