lkml.org 
[lkml]   [2018]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] x86/pti: Fix !PCID and sanitize defines
From
Date
On 01/13/2018 03:23 PM, Thomas Gleixner wrote:
> The switch to the user space page tables in the low level ASM code sets
> unconditionally bit 12 and bit 11 of CR3. Bit 12 is switching the base
> address of the page directory to the user part, bit 11 is switching the
> PCID to the PCID associated with the user page tables.
>
> This fails on a machine which lacks PCID support because bit 11 is set in
> CR3. Bit 11 is reserved when PCID is inactive.
>
> While the Intel SDM claims that the reserved bits are ignored when PCID is
> disabled, the AMD APM states that they should be cleared.
>
> This went unnoticed as the AMD APM was not checked when the code was
> developed and reviewed and test systems with Intel CPUs never failed to
> boot. The report is against a Centos 6 host where the guest fails to boot,
> so it's not yet clear whether this is a virt issue or can happen on real
> hardware too, but thats irrelevant as the AMD APM clearly ask for clearing
> the reserved bits.
>
> Make sure that on non PCID machines bit 11 is not set by the page table
> switching code.
>
> Andy suggested to rename the related bits and masks so they are clearly
> describing what they should be used for, which is done as well for clarity.
>
> That split could have been done with alternatives but the macro hell is
> horrible and ugly. This can be done on top if someone cares to remove the
> extra orq. For now it's a straight forward fix.
>

Original reporter confirmed it fixes the problem. Thanks for the
prompt response.

Laura

> Fixes: 6fd166aae78c ("x86/mm: Use/Fix PCID to optimize user/kernel switches")
> Reported-by: Laura Abbott <labbott@redhat.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Willy Tarreau <w@1wt.eu>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: stable@vger.kernel.org
>
> ---
> arch/x86/entry/calling.h | 36 +++++++++++++++++----------------
> arch/x86/include/asm/processor-flags.h | 2 -
> arch/x86/include/asm/tlbflush.h | 6 ++---
> 3 files changed, 23 insertions(+), 21 deletions(-)
>
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -198,8 +198,11 @@ For 32-bit we have the following convent
> * PAGE_TABLE_ISOLATION PGDs are 8k. Flip bit 12 to switch between the two
> * halves:
> */
> -#define PTI_SWITCH_PGTABLES_MASK (1<<PAGE_SHIFT)
> -#define PTI_SWITCH_MASK (PTI_SWITCH_PGTABLES_MASK|(1<<X86_CR3_PTI_SWITCH_BIT))
> +#define PTI_USER_PGTABLE_BIT PAGE_SHIFT
> +#define PTI_USER_PGTABLE_MASK (1 << PTI_USER_PGTABLE_BIT)
> +#define PTI_USER_PCID_BIT X86_CR3_PTI_PCID_USER_BIT
> +#define PTI_USER_PCID_MASK (1 << PTI_USER_PCID_BIT)
> +#define PTI_USER_PGTABLE_AND_PCID_MASK (PTI_USER_PCID_MASK | PTI_USER_PGTABLE_MASK)
>
> .macro SET_NOFLUSH_BIT reg:req
> bts $X86_CR3_PCID_NOFLUSH_BIT, \reg
> @@ -208,7 +211,7 @@ For 32-bit we have the following convent
> .macro ADJUST_KERNEL_CR3 reg:req
> ALTERNATIVE "", "SET_NOFLUSH_BIT \reg", X86_FEATURE_PCID
> /* Clear PCID and "PAGE_TABLE_ISOLATION bit", point CR3 at kernel pagetables: */
> - andq $(~PTI_SWITCH_MASK), \reg
> + andq $(~PTI_USER_PGTABLE_AND_PCID_MASK), \reg
> .endm
>
> .macro SWITCH_TO_KERNEL_CR3 scratch_reg:req
> @@ -239,15 +242,19 @@ For 32-bit we have the following convent
> /* Flush needed, clear the bit */
> btr \scratch_reg, THIS_CPU_user_pcid_flush_mask
> movq \scratch_reg2, \scratch_reg
> - jmp .Lwrcr3_\@
> + jmp .Lwrcr3_pcid_\@
>
> .Lnoflush_\@:
> movq \scratch_reg2, \scratch_reg
> SET_NOFLUSH_BIT \scratch_reg
>
> +.Lwrcr3_pcid_\@:
> + /* Flip the ASID to the user version */
> + orq $(PTI_USER_PCID_MASK), \scratch_reg
> +
> .Lwrcr3_\@:
> - /* Flip the PGD and ASID to the user version */
> - orq $(PTI_SWITCH_MASK), \scratch_reg
> + /* Flip the PGD to the user version */
> + orq $(PTI_USER_PGTABLE_MASK), \scratch_reg
> mov \scratch_reg, %cr3
> .Lend_\@:
> .endm
> @@ -263,17 +270,12 @@ For 32-bit we have the following convent
> movq %cr3, \scratch_reg
> movq \scratch_reg, \save_reg
> /*
> - * Is the "switch mask" all zero? That means that both of
> - * these are zero:
> - *
> - * 1. The user/kernel PCID bit, and
> - * 2. The user/kernel "bit" that points CR3 to the
> - * bottom half of the 8k PGD
> - *
> - * That indicates a kernel CR3 value, not a user CR3.
> + * Test the user pagetable bit. If set, then the user page tables
> + * are active. If clear CR3 already has the kernel page table
> + * active.
> */
> - testq $(PTI_SWITCH_MASK), \scratch_reg
> - jz .Ldone_\@
> + bt $PTI_USER_PGTABLE_BIT, \scratch_reg
> + jnc .Ldone_\@
>
> ADJUST_KERNEL_CR3 \scratch_reg
> movq \scratch_reg, %cr3
> @@ -290,7 +292,7 @@ For 32-bit we have the following convent
> * KERNEL pages can always resume with NOFLUSH as we do
> * explicit flushes.
> */
> - bt $X86_CR3_PTI_SWITCH_BIT, \save_reg
> + bt $PTI_USER_PGTABLE_BIT, \save_reg
> jnc .Lnoflush_\@
>
> /*
> --- a/arch/x86/include/asm/processor-flags.h
> +++ b/arch/x86/include/asm/processor-flags.h
> @@ -40,7 +40,7 @@
> #define CR3_NOFLUSH BIT_ULL(63)
>
> #ifdef CONFIG_PAGE_TABLE_ISOLATION
> -# define X86_CR3_PTI_SWITCH_BIT 11
> +# define X86_CR3_PTI_PCID_USER_BIT 11
> #endif
>
> #else
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -81,13 +81,13 @@ static inline u16 kern_pcid(u16 asid)
> * Make sure that the dynamic ASID space does not confict with the
> * bit we are using to switch between user and kernel ASIDs.
> */
> - BUILD_BUG_ON(TLB_NR_DYN_ASIDS >= (1 << X86_CR3_PTI_SWITCH_BIT));
> + BUILD_BUG_ON(TLB_NR_DYN_ASIDS >= (1 << X86_CR3_PTI_PCID_USER_BIT));
>
> /*
> * The ASID being passed in here should have respected the
> * MAX_ASID_AVAILABLE and thus never have the switch bit set.
> */
> - VM_WARN_ON_ONCE(asid & (1 << X86_CR3_PTI_SWITCH_BIT));
> + VM_WARN_ON_ONCE(asid & (1 << X86_CR3_PTI_PCID_USER_BIT));
> #endif
> /*
> * The dynamically-assigned ASIDs that get passed in are small
> @@ -112,7 +112,7 @@ static inline u16 user_pcid(u16 asid)
> {
> u16 ret = kern_pcid(asid);
> #ifdef CONFIG_PAGE_TABLE_ISOLATION
> - ret |= 1 << X86_CR3_PTI_SWITCH_BIT;
> + ret |= 1 << X86_CR3_PTI_PCID_USER_BIT;
> #endif
> return ret;
> }
>

\
 
 \ /
  Last update: 2018-01-15 16:15    [W:0.073 / U:0.088 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site