Xen devel - Jul 2013 - [PATCH 1/1 V3] x86/AMD: Fix nested svm crash due to assertion in __virt_to_maddr

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Fix assertion in __virt_to_maddr when starting nested SVM guest
in debug mode. Investigation has shown that svm_vmsave/svm_vmload
make use of __pa() with invalid address.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
Changes in V3:
	- Map L1-GPA to MPA before calling svm_vmload/svm_vmsave

Changes in V2:
	- Uses paddr_t instead of uint64_t (suggested by Tim).
	- Rename the nestedsvm_vmxxxx() to svm_vmxxxx_by_paddr() (suggested by Tim).
	- Wrapped svm_vmxxxx_by_paddr() with svm_vmxxxx() (suggested by Jan).
 xen/arch/x86/hvm/svm/svm.c          |   27 +++++++++++++++++++++++++--
 xen/arch/x86/mm/hap/nested_hap.c    |   13 +++++++++++++
 xen/include/asm-x86/hvm/nestedhvm.h |    3 +++
 xen/include/asm-x86/hvm/svm/svm.h   |   11 +++++++----
 4 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 4a7aeee..efc821e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1798,6 +1798,7 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
                      struct vcpu *v, uint64_t vmcbaddr)
 {
     int ret;
+    paddr_t mpa;
     unsigned int inst_len;
     struct nestedvcpu *nv = &vcpu_nestedhvm(v);
 
@@ -1816,7 +1817,18 @@ svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
         goto inject;
     }
 
-    svm_vmload(nv->nv_vvmcx);
+    /* Need to translate L1-GPA to MPA */
+    if ( NESTEDHVM_PAGEFAULT_DONE != nestedhvm_walk_L0_p2m(
+                                        v, nv->nv_vvmcxaddr, &mpa) )
+    {
+        gdprintk(XENLOG_ERR,
+            "VMLOAD: mapping vmcb L1-GPA to MPA failed, injecting
#UD\n");
+        ret = TRAP_invalid_op;
+        goto inject;
+    }
+
+    svm_vmload_pa(mpa);
+
     /* State in L1 VMCB is stale now */
     v->arch.hvm_svm.vmcb_in_sync = 0;
 
@@ -1834,6 +1846,7 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
                      struct vcpu *v, uint64_t vmcbaddr)
 {
     int ret;
+    paddr_t mpa;
     unsigned int inst_len;
     struct nestedvcpu *nv = &vcpu_nestedhvm(v);
 
@@ -1852,7 +1865,17 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
         goto inject;
     }
 
-    svm_vmsave(nv->nv_vvmcx);
+    /* Need to translate L1-GPA to MPA */
+    if ( NESTEDHVM_PAGEFAULT_DONE != nestedhvm_walk_L0_p2m(
+                                        v, nv->nv_vvmcxaddr, &mpa) )
+    {
+        gdprintk(XENLOG_ERR,
+            "VMSAVE: mapping vmcb L1-GPA to MPA failed, injecting
#UD\n");
+        ret = TRAP_invalid_op;
+        goto inject;
+    }
+
+    svm_vmsave_pa(mpa);
 
     __update_guest_eip(regs, inst_len);
     return;
diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index c2ef1d1..af557d5 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -191,6 +191,19 @@ out:
     return rc;
 }
 
+int
+nestedhvm_walk_L0_p2m(struct vcpu *v, paddr_t L1_gpa, paddr_t *L0_gpa)
+{
+    p2m_type_t p2mt_10;
+    unsigned int page_order_10;
+    p2m_access_t p2ma_10 = p2m_access_rwx;
+
+    return nestedhap_walk_L0_p2m ( p2m_get_hostp2m(v->domain),
+                                 L1_gpa, L0_gpa,
+                                 &p2mt_10, &p2ma_10,
&page_order_10,
+                                 0, 0, 0);
+}
+
 /*
  * The following function, nestedhap_page_fault(), is for steps (3)--(10).
  *
diff --git a/xen/include/asm-x86/hvm/nestedhvm.h
b/xen/include/asm-x86/hvm/nestedhvm.h
index 649c511..228b6eb 100644
--- a/xen/include/asm-x86/hvm/nestedhvm.h
+++ b/xen/include/asm-x86/hvm/nestedhvm.h
@@ -56,6 +56,9 @@ bool_t nestedhvm_vcpu_in_guestmode(struct vcpu *v);
 int nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t *L2_gpa,
     bool_t access_r, bool_t access_w, bool_t access_x);
 
+int nestedhvm_walk_L0_p2m(struct vcpu *v,
+    paddr_t L1_gpa, paddr_t *L0_gpa);
+
 /* IO permission map */
 unsigned long *nestedhvm_vcpu_iomap_get(bool_t ioport_80, bool_t ioport_ed);
 
diff --git a/xen/include/asm-x86/hvm/svm/svm.h
b/xen/include/asm-x86/hvm/svm/svm.h
index 64e7e25..1ffe6d6 100644
--- a/xen/include/asm-x86/hvm/svm/svm.h
+++ b/xen/include/asm-x86/hvm/svm/svm.h
@@ -41,18 +41,21 @@
 #define SVM_REG_R14 (14)
 #define SVM_REG_R15 (15)
 
-static inline void svm_vmload(void *vmcb)
+#define svm_vmload(x)     svm_vmload_pa(__pa(x))
+#define svm_vmsave(x)     svm_vmsave_pa(__pa(x))
+
+static inline void svm_vmload_pa(paddr_t vmcb)
 {
     asm volatile (
         ".byte 0x0f,0x01,0xda" /* vmload */
-        : : "a" (__pa(vmcb)) : "memory" );
+        : : "a" (vmcb) : "memory" );
 }
 
-static inline void svm_vmsave(void *vmcb)
+static inline void svm_vmsave_pa(paddr_t vmcb)
 {
     asm volatile (
         ".byte 0x0f,0x01,0xdb" /* vmsave */
-        : : "a" (__pa(vmcb)) : "memory" );
+        : : "a" (vmcb) : "memory" );
 }
 
 static inline void svm_invlpga(unsigned long vaddr, uint32_t asid)
-- 
1.7.10.4

>>> On 11.07.13 at 19:34, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/arch/x86/mm/hap/nested_hap.c
> +++ b/xen/arch/x86/mm/hap/nested_hap.c
> @@ -191,6 +191,19 @@ out:
>      return rc;
>  }
>  
> +int
> +nestedhvm_walk_L0_p2m(struct vcpu *v, paddr_t L1_gpa, paddr_t *L0_gpa)
> +{
> +    p2m_type_t p2mt_10;
> +    unsigned int page_order_10;
> +    p2m_access_t p2ma_10 = p2m_access_rwx;
Pointless initializer?
> +
> +    return nestedhap_walk_L0_p2m ( p2m_get_hostp2m(v->domain),
Extra spaces around "(".
> +                                 L1_gpa, L0_gpa,
> +                                 &p2mt_10, &p2ma_10,
&page_order_10,
> +                                 0, 0, 0);
Wouldn''t the caller''s use of the GPA imply that you want read
and
write access here?
> +}
I''m not clear about the need for this new wrapper: Is it really
benign to the caller what type, access, and order get returned
here? Is it really too much of a burden to have the two call
sites do the call here directly? The more that (see above) you''d
really need to give the caller control over the access requested?

Finally, considering that now you change a file under
xen/arch/x86/mm/, you should have Cc''ed Tim on the patch
submission.

Jan

On 7/12/2013 3:01 AM, Jan Beulich wrote:
>>>> On 11.07.13 at 19:34, <suravee.suthikulpanit@amd.com>
wrote:
>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>> @@ -191,6 +191,19 @@ out:
>>       return rc;
>>   }
>>   
>> +int
>> +nestedhvm_walk_L0_p2m(struct vcpu *v, paddr_t L1_gpa, paddr_t *L0_gpa)
>> +{
>> +    p2m_type_t p2mt_10;
>> +    unsigned int page_order_10;
>> +    p2m_access_t p2ma_10 = p2m_access_rwx;
> Pointless initializer?
These are mostly part of the required function prototype.  However, I 
noticed that I don''t need to specify page
order.
>
>> +
>> +    return nestedhap_walk_L0_p2m ( p2m_get_hostp2m(v->domain),
> Extra spaces around "(".
Ah, thanks.
>
>> +                                 L1_gpa, L0_gpa,
>> +                                 &p2mt_10, &p2ma_10,
&page_order_10,
>> +                                 0, 0, 0);
> Wouldn''t the caller''s use of the GPA imply that you want
read and
> write access here?
Actually, access_r and access_x is not used in the 
"nestedhap_walk_L0_p2m" function.
Since we are not writing to this GPA, would we need the write
access?
>
>> +}
> I''m not clear about the need for this new wrapper: Is it really
> benign to the caller what type, access, and order get returned
> here? Is it really too much of a burden to have the two call
> sites do the call here directly? The more that (see above) you''d
> really need to give the caller control over the access requested?
Ok, I will just making the nestedhap_walk_L0_p2m not static and add the 
prototype in the svm.h then.
> Finally, considering that now you change a file under
> xen/arch/x86/mm/, you should have Cc''ed Tim on the patch
> submission.
Thanks for pointing out.

Suravee

>>> On 15.07.13 at 23:17, Suravee Suthikulanit
<suravee.suthikulpanit@amd.com>
wrote:
> On 7/12/2013 3:01 AM, Jan Beulich wrote:
>>>>> On 11.07.13 at 19:34, <suravee.suthikulpanit@amd.com>
wrote:
>>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>>> @@ -191,6 +191,19 @@ out:
>>>       return rc;
>>>   }
>>>   
>>> +int
>>> +nestedhvm_walk_L0_p2m(struct vcpu *v, paddr_t L1_gpa, paddr_t
*L0_gpa)
>>> +{
>>> +    p2m_type_t p2mt_10;
>>> +    unsigned int page_order_10;
>>> +    p2m_access_t p2ma_10 = p2m_access_rwx;
>> Pointless initializer?
> These are mostly part of the required function prototype.  However, I 
> noticed that I don''t need to specify page order.
I don''t follow - how is p2ma_10 getting (pointlessly afaict)
initialized related to the prototype of the function?
>>> +                                 L1_gpa, L0_gpa,
>>> +                                 &p2mt_10, &p2ma_10,
&page_order_10,
>>> +                                 0, 0, 0);
>> Wouldn''t the caller''s use of the GPA imply that you
want read and
>> write access here?
> Actually, access_r and access_x is not used in the 
> "nestedhap_walk_L0_p2m" function.
> Since we are not writing to this GPA, would we need the write access?
I may be misunderstanding this (and would hope for Tim to correct
me if so), but generally page table walks get done with the goal of
using the translation for a specific kind of access(es). And the
types of these accesses ought to determine what permissions you
want to be checked for during the walk. In the case here you want
the VMCB to be read from and written to, so you''d want to ask for
read and write permission.

Jan

At 09:01 +0100 on 12 Jul (1373619676), Jan Beulich
wrote:
> >>> On 11.07.13 at 19:34, <suravee.suthikulpanit@amd.com>
wrote:
> > --- a/xen/arch/x86/mm/hap/nested_hap.c
> > +++ b/xen/arch/x86/mm/hap/nested_hap.c
> > @@ -191,6 +191,19 @@ out:
> >      return rc;
> >  }
> >  
> > +int
> > +nestedhvm_walk_L0_p2m(struct vcpu *v, paddr_t L1_gpa, paddr_t
*L0_gpa)
> > +{
> > +    p2m_type_t p2mt_10;
> > +    unsigned int page_order_10;
> > +    p2m_access_t p2ma_10 = p2m_access_rwx;
> 
> Pointless initializer?
Indeed, p2ma_10 could be left alone.
> > +
> > +    return nestedhap_walk_L0_p2m ( p2m_get_hostp2m(v->domain),
> 
> Extra spaces around "(".
> 
> > +                                 L1_gpa, L0_gpa,
> > +                                 &p2mt_10, &p2ma_10,
&page_order_10,
> > +                                 0, 0, 0);
> 
> Wouldn''t the caller''s use of the GPA imply that you want
read and
> write access here?
Yes, at least for the callers you''ve added.
> I''m not clear about the need for this new wrapper: Is it really
> benign to the caller what type, access, and order get returned
> here? Is it really too much of a burden to have the two call
> sites do the call here directly? The more that (see above) you''d
> really need to give the caller control over the access requested?
Yeah, I''m not sure the wrapper is needed.  Can the callers just use
get_page_from_gfn() to do the translation from guest-MFN -- i.e. will we
always be in non-nested mode when we''re emulating VMLOAD/VMSAVE?

Cheers,

Tim.

On 17.07.13 21:43, Tim Deegan wrote:
>> I''m not clear about the need for this new wrapper: Is it
really
>> benign to the caller what type, access, and order get returned
>> here? Is it really too much of a burden to have the two call
>> sites do the call here directly? The more that (see above)
you''d
>> really need to give the caller control over the access requested?
> 
> Yeah, I''m not sure the wrapper is needed.  Can the callers just
use
> get_page_from_gfn() to do the translation from guest-MFN -- i.e. will we
> always be in non-nested mode when we''re emulating VMLOAD/VMSAVE?
When you run an L2 hypervisor then you are in nested mode.

Christoph

On 18.07.13 10:14, Egger, Christoph wrote:
> On 17.07.13 21:43, Tim Deegan wrote:
>>> I''m not clear about the need for this new wrapper: Is it
really
>>> benign to the caller what type, access, and order get returned
>>> here? Is it really too much of a burden to have the two call
>>> sites do the call here directly? The more that (see above)
you''d
>>> really need to give the caller control over the access requested?
>>
>> Yeah, I''m not sure the wrapper is needed.  Can the callers
just use
>> get_page_from_gfn() to do the translation from guest-MFN -- i.e. will
we
>> always be in non-nested mode when we''re emulating
VMLOAD/VMSAVE?
> 
> When you run an L2 hypervisor then you are in nested mode.
Continue thinking...
in this case the l1 hypervisor emulates VMLOAD/VMSAVE.
The l1 hypervisor is in non-nested mode. When the l1 hypervisor will use
the VMLOAD/VMSAVE instructions they get intercepted and will be
emulated by the host hypervisor and is in non-nested mode.

Tim: The answer to your question is yes, we are always in non-nested
mode when we''re emulating VMLOAD/VMSAVE
while at intercept time we are not always in non-nested mode.

Christoph

At 10:24 +0200 on 18 Jul (1374143076), Egger, Christoph
wrote:
> On 18.07.13 10:14, Egger, Christoph wrote:
> > On 17.07.13 21:43, Tim Deegan wrote:
> >>> I''m not clear about the need for this new wrapper: Is
it really
> >>> benign to the caller what type, access, and order get returned
> >>> here? Is it really too much of a burden to have the two call
> >>> sites do the call here directly? The more that (see above)
you''d
> >>> really need to give the caller control over the access
requested?
> >>
> >> Yeah, I''m not sure the wrapper is needed.  Can the
callers just use
> >> get_page_from_gfn() to do the translation from guest-MFN -- i.e.
will we
> >> always be in non-nested mode when we''re emulating
VMLOAD/VMSAVE?
> > 
> > When you run an L2 hypervisor then you are in nested mode.
> 
> Continue thinking...
> in this case the l1 hypervisor emulates VMLOAD/VMSAVE.
> The l1 hypervisor is in non-nested mode. When the l1 hypervisor will use
> the VMLOAD/VMSAVE instructions they get intercepted and will be
> emulated by the host hypervisor and is in non-nested mode.
> 
> Tim: The answer to your question is yes, we are always in non-nested
> mode when we''re emulating VMLOAD/VMSAVE
Good -- so in that case we can use get_page_from_gfn(P2M_ALLOC|P2M_UNSHARE).
The callers should also check that p2m_is_ram() && !p2m_is_readonly()
on the returned type.

Cheers,

Tim.