diff mbox series

[5/9] x86/HVM: refuse CR3 loads with reserved (upper) bits set

Message ID 30619001-ca6c-0450-a0bb-4d71687b281a@suse.com (mailing list archive)
State Superseded
Headers show
Series XSA-292 follow-up | expand

Commit Message

Jan Beulich Sept. 11, 2019, 3:24 p.m. UTC 
While bits 11 and below are, if not used for other purposes, reserved
but ignored, bits beyond physical address width are supposed to raise
exceptions (at least in the non-nested case; I'm not convinced the
current nested SVM/VMX behavior of raising #GP(0) here is correct, but
that's not the subject of this change).

Introduce currd as a local variable, and replace other v->domain
instances at the same time.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Roger Pau Monné Sept. 12, 2019, 11:45 a.m. UTC | #1 
On Wed, Sep 11, 2019 at 05:24:41PM +0200, Jan Beulich wrote:
> While bits 11 and below are, if not used for other purposes, reserved
> but ignored, bits beyond physical address width are supposed to raise
> exceptions (at least in the non-nested case; I'm not convinced the
> current nested SVM/VMX behavior of raising #GP(0) here is correct, but
> that's not the subject of this change).
> 
> Introduce currd as a local variable, and replace other v->domain
> instances at the same time.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

LGTM, just two comments which are not related to functionality, so:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1004,6 +1004,13 @@ static int hvm_load_cpu_ctxt(struct doma
>          return -EINVAL;
>      }
>  
> +    if ( ctxt.cr3 & ~((1UL << d->arch.cpuid->extd.maxphysaddr) - 1) )
> +    {
> +        printk(XENLOG_G_ERR "HVM%d restore: bad CR3 %#" PRIx64 "\n",

gprintk would be more natural here IMO.

> +               d->domain_id, ctxt.cr3);
> +        return X86EMUL_EXCEPTION;
> +    }
> +
>      if ( (ctxt.flags & ~XEN_X86_FPU_INITIALISED) != 0 )
>      {
>          gprintk(XENLOG_ERR, "bad flags value in CPU context: %#x\n",
> @@ -2290,10 +2297,19 @@ int hvm_set_cr0(unsigned long value, boo
>  int hvm_set_cr3(unsigned long value, bool noflush, bool may_defer)
>  {
>      struct vcpu *v = current;
> +    struct domain *currd = v->domain;
>      struct page_info *page;
>      unsigned long old = v->arch.hvm.guest_cr[3];
>  
> -    if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled &
> +    if ( value & ~((1UL << currd->arch.cpuid->extd.maxphysaddr) - 1) )

I would consider introducing a macro/inline helper for this, since
it's already used twice in this patch.

Thanks, Roger.
Jan Beulich Sept. 12, 2019, 12:01 p.m. UTC | #2 
On 12.09.2019 13:45, Roger Pau Monné  wrote:
> On Wed, Sep 11, 2019 at 05:24:41PM +0200, Jan Beulich wrote:
>> While bits 11 and below are, if not used for other purposes, reserved
>> but ignored, bits beyond physical address width are supposed to raise
>> exceptions (at least in the non-nested case; I'm not convinced the
>> current nested SVM/VMX behavior of raising #GP(0) here is correct, but
>> that's not the subject of this change).
>>
>> Introduce currd as a local variable, and replace other v->domain
>> instances at the same time.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> LGTM, just two comments which are not related to functionality, so:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1004,6 +1004,13 @@ static int hvm_load_cpu_ctxt(struct doma
>>          return -EINVAL;
>>      }
>>  
>> +    if ( ctxt.cr3 & ~((1UL << d->arch.cpuid->extd.maxphysaddr) - 1) )
>> +    {
>> +        printk(XENLOG_G_ERR "HVM%d restore: bad CR3 %#" PRIx64 "\n",
> 
> gprintk would be more natural here IMO.

I don't think so, no - the %pv value additionally getting logged
by gprintk() has no real use in this case.

>> @@ -2290,10 +2297,19 @@ int hvm_set_cr0(unsigned long value, boo
>>  int hvm_set_cr3(unsigned long value, bool noflush, bool may_defer)
>>  {
>>      struct vcpu *v = current;
>> +    struct domain *currd = v->domain;
>>      struct page_info *page;
>>      unsigned long old = v->arch.hvm.guest_cr[3];
>>  
>> -    if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled &
>> +    if ( value & ~((1UL << currd->arch.cpuid->extd.maxphysaddr) - 1) )
> 
> I would consider introducing a macro/inline helper for this, since
> it's already used twice in this patch.

Well, yes, I could do this in a prereq patch, taking care of the
same expression in guest_pt.h. Grep-ing the tree I see a better
way of doing this though (in paging.h), and hence I guess I'll
switch both to

    if ( <VAL> >> currd->arch.cpuid->extd.maxphysaddr )

I'll take it that this wouldn't invalidate you R-b.

Jan
diff mbox series

Patch

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1004,6 +1004,13 @@  static int hvm_load_cpu_ctxt(struct doma
         return -EINVAL;
     }
 
+    if ( ctxt.cr3 & ~((1UL << d->arch.cpuid->extd.maxphysaddr) - 1) )
+    {
+        printk(XENLOG_G_ERR "HVM%d restore: bad CR3 %#" PRIx64 "\n",
+               d->domain_id, ctxt.cr3);
+        return X86EMUL_EXCEPTION;
+    }
+
     if ( (ctxt.flags & ~XEN_X86_FPU_INITIALISED) != 0 )
     {
         gprintk(XENLOG_ERR, "bad flags value in CPU context: %#x\n",
@@ -2290,10 +2297,19 @@  int hvm_set_cr0(unsigned long value, boo
 int hvm_set_cr3(unsigned long value, bool noflush, bool may_defer)
 {
     struct vcpu *v = current;
+    struct domain *currd = v->domain;
     struct page_info *page;
     unsigned long old = v->arch.hvm.guest_cr[3];
 
-    if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled &
+    if ( value & ~((1UL << currd->arch.cpuid->extd.maxphysaddr) - 1) )
+    {
+        HVM_DBG_LOG(DBG_LEVEL_1,
+                    "Attempt to set reserved CR3 bit(s): %lx",
+                    value);
+        return X86EMUL_EXCEPTION;
+    }
+
+    if ( may_defer && unlikely(currd->arch.monitor.write_ctrlreg_enabled &
                                monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3)) )
     {
         ASSERT(v->arch.vm_event);
@@ -2309,13 +2325,12 @@  int hvm_set_cr3(unsigned long value, boo
         }
     }
 
-    if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
+    if ( hvm_paging_enabled(v) && !paging_mode_hap(currd) &&
          (value != v->arch.hvm.guest_cr[3]) )
     {
         /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
         HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
-        page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
-                                 NULL, P2M_ALLOC);
+        page = get_page_from_gfn(currd, value >> PAGE_SHIFT, NULL, P2M_ALLOC);
         if ( !page )
             goto bad_cr3;
 
@@ -2331,7 +2346,7 @@  int hvm_set_cr3(unsigned long value, boo
 
  bad_cr3:
     gdprintk(XENLOG_ERR, "Invalid CR3\n");
-    domain_crash(v->domain);
+    domain_crash(currd);
     return X86EMUL_UNHANDLEABLE;
 }