| Message ID | 89201c66b0261b2f5ee83e7672830317fde21dfa.1673123823.git.demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make PAT handling less brittle | expand |
On 07.01.2023 23:07, Demi Marie Obenour wrote: > @@ -6412,6 +6414,100 @@ static void __init __maybe_unused build_assertions(void) > * using different PATs will not work. > */ > BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL); > + > + /* > + * _PAGE_WB must be zero. Linux PV guests assume that _PAGE_WB will be > + * zero, and indeed Linux has a BUILD_BUG_ON validating that their version > + * of _PAGE_WB *is* zero. Furthermore, since _PAGE_WB is zero, it is quite > + * likely to be omitted from various parts of Xen, and indeed L1 PTE > + * validation code checks that ((l1f & PAGE_CACHE_ATTRs) == 0), not > + * ((l1f & PAGE_CACHE_ATTRs) == _PAGE_WB). > + */ > + BUILD_BUG_ON(_PAGE_WB); > + > + /* _PAGE_RSVD_1 must be less than _PAGE_RSVD_2 */ > + BUILD_BUG_ON(_PAGE_RSVD_1 >= _PAGE_RSVD_2); > + > +#define PAT_ENTRY(v) \ > + (BUILD_BUG_ON_ZERO(((v) < 0) || ((v) > 7)) + \ > + (0xFF & (XEN_MSR_PAT >> (8 * (v))))) > + > + /* Validate at compile-time that v is a valid value for a PAT entry */ > +#define CHECK_PAT_ENTRY_VALUE(v) \ > + BUILD_BUG_ON((v) > X86_NUM_MT || (v) == X86_MT_RSVD_2 || \ > + (v) == X86_MT_RSVD_3) > + > + /* Validate at compile-time that PAT entry v is valid */ > +#define CHECK_PAT_ENTRY(v) CHECK_PAT_ENTRY_VALUE(PAT_ENTRY(v)) > + > + /* > + * If one of these trips, the corresponding entry in XEN_MSR_PAT is invalid. > + * This would cause Xen to crash (with #GP) at startup. > + */ > + CHECK_PAT_ENTRY(0); > + CHECK_PAT_ENTRY(1); > + CHECK_PAT_ENTRY(2); > + CHECK_PAT_ENTRY(3); > + CHECK_PAT_ENTRY(4); > + CHECK_PAT_ENTRY(5); > + CHECK_PAT_ENTRY(6); > + CHECK_PAT_ENTRY(7); > + > + /* Macro version of pte_flags_to_cacheattr(), for use in BUILD_BUG_ON()s */ > +#define PTE_FLAGS_TO_CACHEATTR(pte_value) \ > + /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */ \ > + (BUILD_BUG_ON_ZERO(((pte_value) & PAGE_CACHE_ATTRS) != (pte_value)) | \ Slightly cheaper as BUILD_BUG_ON_ZERO((pte_value) & ~PAGE_CACHE_ATTRS)? > + (((pte_value) & _PAGE_PAT) >> 5) | \ > + (((pte_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3)) > + > + CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_RSVD_1)); > + CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_RSVD_2)); What do these two check that the 8 instances above don't already check? > +#define PAT_ENTRY_FROM_FLAGS(x) PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(x)) > + > + /* Validate at compile time that X does not duplicate a smaller PAT entry */ > +#define CHECK_DUPLICATE_ENTRY(x, y) \ > + BUILD_BUG_ON((x) >= (y) && \ > + (PAT_ENTRY_FROM_FLAGS(x) == PAT_ENTRY_FROM_FLAGS(y))) Imo nothing says that the reserved entries come last. I'm therefore not convinced of the usefulness of the two uses of this macro. > + /* Check that a PAT-related _PAGE_* macro is correct */ > +#define CHECK_PAGE_VALUE(page_value) do { \ > + /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */ \ > + BUILD_BUG_ON(((_PAGE_ ## page_value) & PAGE_CACHE_ATTRS) != \ > + (_PAGE_ ## page_value)); \ > + /* Check that the _PAGE_* are consistent with XEN_MSR_PAT */ \ > + BUILD_BUG_ON(PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_ ## page_value)) != \ > + (X86_MT_ ## page_value)); \ > + case _PAGE_ ## page_value:; /* ensure no duplicate values */ \ Wouldn't this better come first in the macro? The semicolon looks unnecessary in any event. > + /* \ > + * Check that the _PAGE_* entries do not duplicate a smaller reserved \ > + * entry. \ > + */ \ > + CHECK_DUPLICATE_ENTRY(_PAGE_ ## page_value, _PAGE_RSVD_1); \ > + CHECK_DUPLICATE_ENTRY(_PAGE_ ## page_value, _PAGE_RSVD_2); \ > + CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_ ## page_value)); \ > +} while ( false ) > + > + /* > + * If one of these trips, the corresponding _PAGE_* macro is inconsistent > + * with XEN_MSR_PAT. This would cause Xen to use incorrect cacheability > + * flags, with results that are unknown and possibly harmful. > + */ > + switch (0) { Nit: Style. > + CHECK_PAGE_VALUE(WT); > + CHECK_PAGE_VALUE(WB); > + CHECK_PAGE_VALUE(WC); > + CHECK_PAGE_VALUE(UC); > + CHECK_PAGE_VALUE(UCM); > + CHECK_PAGE_VALUE(WP); All of these are lacking "break" and hence are liable to trigger static checker warnings. > + case _PAGE_RSVD_1: > + case _PAGE_RSVD_2: > + break; > + } > +#undef CHECK_PAT_ENTRY > +#undef CHECK_PAT_ENTRY_VALUE > +#undef CHECK_PAGE_VALUE > +#undef PAGE_FLAGS_TO_CACHEATTR PTE_FLAGS_TO_CACHEATTR? > +#undef PAT_ENTRY You also #define more than these 5 macros now (but as per above e.g. CHECK_DUPLICATE_ENTRY() may go away again). Jan
diff --git a/xen/arch/x86/include/asm/page.h b/xen/arch/x86/include/asm/page.h index b585235d064a567082582c8e92a4e8283fd949ca..c7d77ab2901aa5bdb03a719af810c6f8d8ba9d4e 100644 --- a/xen/arch/x86/include/asm/page.h +++ b/xen/arch/x86/include/asm/page.h @@ -338,6 +338,8 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t); #define _PAGE_UC ( _PAGE_PCD | _PAGE_PWT) #define _PAGE_WC (_PAGE_PAT ) #define _PAGE_WP (_PAGE_PAT | _PAGE_PWT) +#define _PAGE_RSVD_1 (_PAGE_PAT | _PAGE_PCD ) +#define _PAGE_RSVD_2 (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT) /* * Debug option: Ensure that granted mappings are not implicitly unmapped. diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index a8f137925cba1846b97aee9321df6427f4dd1a94..d69e9bea6c30bc782ab4c331f42502f6e61a028a 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -890,6 +890,8 @@ get_page_from_l1e( case _PAGE_WT: case _PAGE_WP: break; + case _PAGE_RSVD_1: + case _PAGE_RSVD_2: default: /* * If we get here, a PV guest tried to use one of the @@ -6412,6 +6414,100 @@ static void __init __maybe_unused build_assertions(void) * using different PATs will not work. */ BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL); + + /* + * _PAGE_WB must be zero. Linux PV guests assume that _PAGE_WB will be + * zero, and indeed Linux has a BUILD_BUG_ON validating that their version + * of _PAGE_WB *is* zero. Furthermore, since _PAGE_WB is zero, it is quite + * likely to be omitted from various parts of Xen, and indeed L1 PTE + * validation code checks that ((l1f & PAGE_CACHE_ATTRs) == 0), not + * ((l1f & PAGE_CACHE_ATTRs) == _PAGE_WB). + */ + BUILD_BUG_ON(_PAGE_WB); + + /* _PAGE_RSVD_1 must be less than _PAGE_RSVD_2 */ + BUILD_BUG_ON(_PAGE_RSVD_1 >= _PAGE_RSVD_2); + +#define PAT_ENTRY(v) \ + (BUILD_BUG_ON_ZERO(((v) < 0) || ((v) > 7)) + \ + (0xFF & (XEN_MSR_PAT >> (8 * (v))))) + + /* Validate at compile-time that v is a valid value for a PAT entry */ +#define CHECK_PAT_ENTRY_VALUE(v) \ + BUILD_BUG_ON((v) > X86_NUM_MT || (v) == X86_MT_RSVD_2 || \ + (v) == X86_MT_RSVD_3) + + /* Validate at compile-time that PAT entry v is valid */ +#define CHECK_PAT_ENTRY(v) CHECK_PAT_ENTRY_VALUE(PAT_ENTRY(v)) + + /* + * If one of these trips, the corresponding entry in XEN_MSR_PAT is invalid. + * This would cause Xen to crash (with #GP) at startup. + */ + CHECK_PAT_ENTRY(0); + CHECK_PAT_ENTRY(1); + CHECK_PAT_ENTRY(2); + CHECK_PAT_ENTRY(3); + CHECK_PAT_ENTRY(4); + CHECK_PAT_ENTRY(5); + CHECK_PAT_ENTRY(6); + CHECK_PAT_ENTRY(7); + + /* Macro version of pte_flags_to_cacheattr(), for use in BUILD_BUG_ON()s */ +#define PTE_FLAGS_TO_CACHEATTR(pte_value) \ + /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */ \ + (BUILD_BUG_ON_ZERO(((pte_value) & PAGE_CACHE_ATTRS) != (pte_value)) | \ + (((pte_value) & _PAGE_PAT) >> 5) | \ + (((pte_value) & (_PAGE_PCD | _PAGE_PWT)) >> 3)) + + CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_RSVD_1)); + CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_RSVD_2)); +#define PAT_ENTRY_FROM_FLAGS(x) PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(x)) + + /* Validate at compile time that X does not duplicate a smaller PAT entry */ +#define CHECK_DUPLICATE_ENTRY(x, y) \ + BUILD_BUG_ON((x) >= (y) && \ + (PAT_ENTRY_FROM_FLAGS(x) == PAT_ENTRY_FROM_FLAGS(y))) + + /* Check that a PAT-related _PAGE_* macro is correct */ +#define CHECK_PAGE_VALUE(page_value) do { \ + /* Check that the _PAGE_* macros only use bits from PAGE_CACHE_ATTRS */ \ + BUILD_BUG_ON(((_PAGE_ ## page_value) & PAGE_CACHE_ATTRS) != \ + (_PAGE_ ## page_value)); \ + /* Check that the _PAGE_* are consistent with XEN_MSR_PAT */ \ + BUILD_BUG_ON(PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_ ## page_value)) != \ + (X86_MT_ ## page_value)); \ + case _PAGE_ ## page_value:; /* ensure no duplicate values */ \ + /* \ + * Check that the _PAGE_* entries do not duplicate a smaller reserved \ + * entry. \ + */ \ + CHECK_DUPLICATE_ENTRY(_PAGE_ ## page_value, _PAGE_RSVD_1); \ + CHECK_DUPLICATE_ENTRY(_PAGE_ ## page_value, _PAGE_RSVD_2); \ + CHECK_PAT_ENTRY(PTE_FLAGS_TO_CACHEATTR(_PAGE_ ## page_value)); \ +} while ( false ) + + /* + * If one of these trips, the corresponding _PAGE_* macro is inconsistent + * with XEN_MSR_PAT. This would cause Xen to use incorrect cacheability + * flags, with results that are unknown and possibly harmful. + */ + switch (0) { + CHECK_PAGE_VALUE(WT); + CHECK_PAGE_VALUE(WB); + CHECK_PAGE_VALUE(WC); + CHECK_PAGE_VALUE(UC); + CHECK_PAGE_VALUE(UCM); + CHECK_PAGE_VALUE(WP); + case _PAGE_RSVD_1: + case _PAGE_RSVD_2: + break; + } +#undef CHECK_PAT_ENTRY +#undef CHECK_PAT_ENTRY_VALUE +#undef CHECK_PAGE_VALUE +#undef PAGE_FLAGS_TO_CACHEATTR +#undef PAT_ENTRY } /*