Re: [PATCH v1 3/3] x86: Make the GDT remapping read-only on 64 bit

[Andy Lutomirski]

On Fri, Jan 20, 2017 at 8:41 AM, Thomas Garnier <thgarnie@xxxxxxxxxx> wrote:
> This patch makes the GDT remapped pages read-only to prevent corruption.
> This change is done only on 64 bit.
>
> The native_load_tr_desc function was adapted to correctly handle a
> read-only GDT. The LTR instruction always writes to the GDT TSS entry.
> This generates a page fault if the GDT is read-only. This change checks
> if the current GDT is a remap and swap GDTs as needed. This function was
> tested by booting multiple machines and checking hibernation works
> properly.
>
> KVM SVM and VMX were adapted to use the writeable GDT. On VMX, the GDT
> description and writeble address were put in two per-cpu variables for
> convenience. For testing, VMs were started and restored on multiple
> configurations.
>
> Signed-off-by: Thomas Garnier <thgarnie@xxxxxxxxxx>
> ---
> Based on next-20170119
> ---
>  arch/x86/include/asm/desc.h      | 44 +++++++++++++++++++++++++++++++++++-----
>  arch/x86/include/asm/processor.h |  1 +
>  arch/x86/kernel/cpu/common.c     | 36 ++++++++++++++++++++++++++------
>  arch/x86/kvm/svm.c               |  5 ++---
>  arch/x86/kvm/vmx.c               | 23 +++++++++++++--------
>  5 files changed, 86 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
> index 12080d87da3b..6ed68d449c7b 100644
> --- a/arch/x86/include/asm/desc.h
> +++ b/arch/x86/include/asm/desc.h
> @@ -50,6 +50,13 @@ static inline struct desc_struct *get_cpu_gdt_table(unsigned int cpu)
>         return per_cpu(gdt_page, cpu).gdt;
>  }
>
> +static inline struct desc_struct *get_current_gdt_table(void)
> +{
> +       return get_cpu_var(gdt_page).gdt;
> +}

This function is poorly named IMO.  Which GDT address does it return?
Can we call it get_current_direct_gdt()?  Also, IIRC
this_cpu_read(gdt_page.gdt) generates better code.

> +
> +struct desc_struct *get_remapped_gdt(int cpu);

And get_current_fixmap_gdt(void) perhaps?  And why isn't it inline?
It's very simple.

> +/*
> + * The LTR instruction marks the TSS GDT entry as busy. In 64bit, the GDT is
> + * a read-only remapping. To prevent a page fault, the GDT is switched to the
> + * original writeable version when needed.
> + */
> +#ifdef CONFIG_X86_64
> +static inline void native_load_tr_desc(void)
> +{
> +       struct desc_ptr gdt;
> +       int cpu = raw_smp_processor_id();
> +       bool restore = false;
> +       struct desc_struct *remapped_gdt;
> +
> +       native_store_gdt(&gdt);
> +       remapped_gdt = get_remapped_gdt(cpu);
> +
> +       /* Swap and restore only if the current GDT is the remap. */
> +       if (gdt.address == (unsigned long)remapped_gdt) {
> +               load_percpu_gdt(cpu);

This line (load_percpu_gdt(cpu)) is hard to understand because of the
poorly named function.

>  /* Load a fixmap remapping of the per-cpu GDT */
>  void load_remapped_gdt(int cpu)
>  {
>         struct desc_ptr gdt_descr;
>         unsigned long idx = FIX_GDT_REMAP_BEGIN + cpu;
>
> -       __set_fixmap(idx, __pa(get_cpu_gdt_table(cpu)), PAGE_KERNEL);
> +       __set_fixmap(idx, __pa(get_cpu_gdt_table(cpu)), GDT_REMAP_PROT);

How about PAGE_FIXMAP_GDT instead of GDT_REMAP_PROT?

> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index d0414f054bdf..da261f231017 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -741,7 +741,6 @@ static int svm_hardware_enable(void)
>
>         struct svm_cpu_data *sd;
>         uint64_t efer;
> -       struct desc_ptr gdt_descr;
>         struct desc_struct *gdt;
>         int me = raw_smp_processor_id();
>
> @@ -763,8 +762,7 @@ static int svm_hardware_enable(void)
>         sd->max_asid = cpuid_ebx(SVM_CPUID_FUNC) - 1;
>         sd->next_asid = sd->max_asid + 1;
>
> -       native_store_gdt(&gdt_descr);
> -       gdt = (struct desc_struct *)gdt_descr.address;
> +       gdt = get_current_gdt_table();
>         sd->tss_desc = (struct kvm_ldttss_desc *)(gdt + GDT_ENTRY_TSS);
>
>         wrmsrl(MSR_EFER, efer | EFER_SVME);
> @@ -4251,6 +4249,7 @@ static void reload_tss(struct kvm_vcpu *vcpu)
>
>         struct svm_cpu_data *sd = per_cpu(svm_data, cpu);
>         sd->tss_desc->type = 9; /* available 32/64-bit TSS */
> +
>         load_TR_desc();
>  }

Paulo, are you okay with the performance hit here?  Is there any easy
way to avoid it?

>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index d2fe3a51876c..acbf78c962d0 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -935,7 +935,8 @@ static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
>   * when a CPU is brought down, and we need to VMCLEAR all VMCSs loaded on it.
>   */
>  static DEFINE_PER_CPU(struct list_head, loaded_vmcss_on_cpu);
> -static DEFINE_PER_CPU(struct desc_ptr, host_gdt);
> +static DEFINE_PER_CPU(struct desc_ptr, host_gdt_desc);
> +static DEFINE_PER_CPU(unsigned long, host_gdt);

This pair of variables is inscrutible IMO.  It should at least have a
comment, but better names would help even more.

>
>  /*
>   * We maintian a per-CPU linked-list of vCPU, so in wakeup_handler() we
> @@ -1997,10 +1998,9 @@ static void reload_tss(void)
>         /*
>          * VT restores TR but not its size.  Useless.
>          */
> -       struct desc_ptr *gdt = this_cpu_ptr(&host_gdt);
>         struct desc_struct *descs;
>
> -       descs = (void *)gdt->address;
> +       descs = (void *)get_cpu_var(host_gdt);
>         descs[GDT_ENTRY_TSS].type = 9; /* available TSS */
>         load_TR_desc();
>  }
> @@ -2061,7 +2061,6 @@ static bool update_transition_efer(struct vcpu_vmx *vmx, int efer_offset)
>
>  static unsigned long segment_base(u16 selector)
>  {
> -       struct desc_ptr *gdt = this_cpu_ptr(&host_gdt);
>         struct desc_struct *d;
>         unsigned long table_base;
>         unsigned long v;
> @@ -2069,7 +2068,7 @@ static unsigned long segment_base(u16 selector)
>         if (!(selector & ~3))
>                 return 0;
>
> -       table_base = gdt->address;
> +       table_base = get_cpu_var(host_gdt);

this_cpu_read() if possible, please.  But can't this just use the
generic accessor instead of a KVM-specific percpu variable?

>
>         if (selector & 4) {           /* from ldt */
>                 u16 ldt_selector = kvm_read_ldt();
> @@ -2185,7 +2184,7 @@ static void __vmx_load_host_state(struct vcpu_vmx *vmx)
>  #endif
>         if (vmx->host_state.msr_host_bndcfgs)
>                 wrmsrl(MSR_IA32_BNDCFGS, vmx->host_state.msr_host_bndcfgs);
> -       load_gdt(this_cpu_ptr(&host_gdt));
> +       load_gdt(this_cpu_ptr(&host_gdt_desc));
>  }

I assume the intent is to have the VM exit restore the direct GDT,
then to load the new TR, then to load the fixmap GDT.  Is that indeed
the case?.

>
>  static void vmx_load_host_state(struct vcpu_vmx *vmx)
> @@ -2287,7 +2286,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>         }
>
>         if (!already_loaded) {
> -               struct desc_ptr *gdt = this_cpu_ptr(&host_gdt);
> +               unsigned long gdt = get_cpu_var(host_gdt);
>                 unsigned long sysenter_esp;
>
>                 kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
> @@ -2297,7 +2296,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>                  * processors.
>                  */
>                 vmcs_writel(HOST_TR_BASE, kvm_read_tr_base()); /* 22.2.4 */
> -               vmcs_writel(HOST_GDTR_BASE, gdt->address);   /* 22.2.4 */
> +               vmcs_writel(HOST_GDTR_BASE, gdt);   /* 22.2.4 */
>
>                 rdmsrl(MSR_IA32_SYSENTER_ESP, sysenter_esp);
>                 vmcs_writel(HOST_IA32_SYSENTER_ESP, sysenter_esp); /* 22.2.3 */
> @@ -3523,7 +3522,13 @@ static int hardware_enable(void)
>                 ept_sync_global();
>         }
>
> -       native_store_gdt(this_cpu_ptr(&host_gdt));
> +       native_store_gdt(this_cpu_ptr(&host_gdt_desc));
> +
> +       /*
> +        * The GDT is remapped and can be read-only, use the underlying memory
> +        * that is always writeable.
> +        */
> +       per_cpu(host_gdt, cpu) = (unsigned long)get_current_gdt_table();

this_cpu_write().  Better yet, just get rid of this entirely.

--Andy