[Xen-devel] Ping: [PATCH 4/6] VMX: correct PDPTE load checks

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.07.18 at 12:49,  wrote:
> Checking the low 5 bits of CR3 is not the job of vmx_load_pdptrs().
> Instead it should #GP upon bad PDPTE values, rather than causing a VM
> entry failure.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1361,20 +1361,18 @@ static void vmx_set_interrupt_shadow(str
>      __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
>  }
>  
> -static void vmx_load_pdptrs(struct vcpu *v)
> +static bool vmx_load_pdptrs(struct vcpu *v)
>  {
>      unsigned long cr3 = v->arch.hvm_vcpu.guest_cr[3];
> -    uint64_t *guest_pdptes;
> +    uint64_t *guest_pdptes, valid_mask;
>      struct page_info *page;
>      p2m_type_t p2mt;
>      char *p;
> +    unsigned int i;
>  
>      /* EPT needs to load PDPTRS into VMCS for PAE. */
>      if ( !hvm_pae_enabled(v) || (v->arch.hvm_vcpu.guest_efer & EFER_LMA) )
> -        return;
> -
> -    if ( (cr3 & 0x1fUL) && !hvm_pcid_enabled(v) )
> -        goto crash;
> +        return true;
>  
>      page = get_page_from_gfn(v->domain, cr3 >> PAGE_SHIFT, &p2mt, 
> P2M_UNSHARE);
>      if ( !page )
> @@ -1385,34 +1383,47 @@ static void vmx_load_pdptrs(struct vcpu
>          gdprintk(XENLOG_ERR,
>                   "Bad cr3 on load pdptrs gfn %lx type %d\n",
>                   cr3 >> PAGE_SHIFT, (int) p2mt);
> -        goto crash;
> +        domain_crash(v->domain);
> +        return false;
>      }
>  
>      p = __map_domain_page(page);
>  
> -    guest_pdptes = (uint64_t *)(p + (cr3 & ~PAGE_MASK));
> +    guest_pdptes = (uint64_t *)(p + (cr3 & 0xfe0));
>  
> -    /*
> -     * We do not check the PDPTRs for validity. The CPU will do this during
> -     * vm entry, and we can handle the failure there and crash the guest.
> -     * The only thing we could do better here is #GP instead.
> -     */
> +    valid_mask = ((1ULL << v->domain->arch.cpuid->extd.maxphysaddr) - 1) &
> +                 (PAGE_MASK | _PAGE_AVAIL | _PAGE_PRESENT);
> +    for ( i = 0; i < 4; ++i )
> +        if ( (guest_pdptes[i] & _PAGE_PRESENT) &&
> +             (guest_pdptes[i] & ~valid_mask) )
> +        {
> +            if ( v == current )
> +                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +            else
> +            {
> +                printk(XENLOG_G_ERR "%pv: bad PDPTE%u: %"PRIx64"\n",
> +                       v, i, guest_pdptes[i]);
> +                domain_crash(v->domain);
> +            }
> +            break;
> +        }
>  
> -    vmx_vmcs_enter(v);
> +    if ( i == 4 )
> +    {
> +        vmx_vmcs_enter(v);
>  
> -    __vmwrite(GUEST_PDPTE(0), guest_pdptes[0]);
> -    __vmwrite(GUEST_PDPTE(1), guest_pdptes[1]);
> -    __vmwrite(GUEST_PDPTE(2), guest_pdptes[2]);
> -    __vmwrite(GUEST_PDPTE(3), guest_pdptes[3]);
> +        __vmwrite(GUEST_PDPTE(0), guest_pdptes[0]);
> +        __vmwrite(GUEST_PDPTE(1), guest_pdptes[1]);
> +        __vmwrite(GUEST_PDPTE(2), guest_pdptes[2]);
> +        __vmwrite(GUEST_PDPTE(3), guest_pdptes[3]);
>  
> -    vmx_vmcs_exit(v);
> +        vmx_vmcs_exit(v);
> +    }
>  
>      unmap_domain_page(p);
>      put_page(page);
> -    return;
>  
> - crash:
> -    domain_crash(v->domain);
> +    return i == 4;
>  }
>  
>  static void vmx_update_host_cr3(struct vcpu *v)
> @@ -1621,7 +1632,8 @@ static void vmx_update_guest_cr(struct v
>              if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
>                  v->arch.hvm_vcpu.hw_cr[3] =
>                      v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT];
> -            vmx_load_pdptrs(v);
> +            if ( !vmx_load_pdptrs(v) )
> +                break;
>          }
>  
>          __vmwrite(GUEST_CR3, v->arch.hvm_vcpu.hw_cr[3]);
> 
> 
> 
> 





_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel