[PATCH] VMX: allocate APIC access page from domain heap

[PATCH] VMX: allocate APIC access page from domain heap

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]

... since we don't need its virtual address anywhere (it's a
placeholder page only after all). For this wo work (and possibly be
done elsewhere too) share_xen_page_with_guest() needs to mark pages
handed to it as Xen heap ones.

To be on the safe side, also explicitly clear the page (not having done
so was okay due to the XSA-100 fix, but is still a latent bug since we
don't formally guarantee allocations to come out zeroed, and in fact
this property may disappear again as soon as the asynchronous runtime
scrubbing patches arrive).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Alternatives might be to use a
- global (or perhaps per-node) page across VMs (on the basis that VMs
  shouldn't be writing into that page anyway)
- fake MFN pointing into nowhere (would need to ensure no side effects
  can occur, like PCIe errors or NMIs)

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2402,19 +2402,21 @@ gp_fault:
 
 static int vmx_alloc_vlapic_mapping(struct domain *d)
 {
-    void *apic_va;
+    struct page_info *pg;
+    unsigned long mfn;
 
     if ( !cpu_has_vmx_virtualize_apic_accesses )
         return 0;
 
-    apic_va = alloc_xenheap_page();
-    if ( apic_va == NULL )
+    pg = alloc_domheap_page(d, MEMF_no_owner);
+    if ( !pg )
         return -ENOMEM;
-    share_xen_page_with_guest(virt_to_page(apic_va), d, XENSHARE_writable);
-    d->arch.hvm_domain.vmx.apic_access_mfn = virt_to_mfn(apic_va);
-    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
-                       _mfn(virt_to_mfn(apic_va)), PAGE_ORDER_4K,
-                       p2m_get_hostp2m(d)->default_access);
+    mfn = page_to_mfn(pg);
+    clear_domain_page(mfn);
+    share_xen_page_with_guest(pg, d, XENSHARE_writable);
+    d->arch.hvm_domain.vmx.apic_access_mfn = mfn;
+    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), _mfn(mfn),
+                       PAGE_ORDER_4K, p2m_get_hostp2m(d)->default_access);
 
     return 0;
 }
@@ -2422,8 +2424,16 @@ static int vmx_alloc_vlapic_mapping(stru
 static void vmx_free_vlapic_mapping(struct domain *d)
 {
     unsigned long mfn = d->arch.hvm_domain.vmx.apic_access_mfn;
+
     if ( mfn != 0 )
-        free_xenheap_page(mfn_to_virt(mfn));
+    {
+        struct page_info *pg = mfn_to_page(mfn);
+
+        if ( test_and_clear_bit(_PGC_allocated, &pg->count_info) )
+            put_page(pg);
+        clear_bit(_PGC_xen_heap, &pg->count_info);
+        free_domheap_page(pg);
+    }
 }
 
 static void vmx_install_vlapic_mapping(struct vcpu *v)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -454,7 +454,7 @@ void share_xen_page_with_guest(
     /* Only add to the allocation list if the domain isn't dying. */
     if ( !d->is_dying )
     {
-        page->count_info |= PGC_allocated | 1;
+        page->count_info |= PGC_xen_heap | PGC_allocated | 1;
         if ( unlikely(d->xenheap_pages++ == 0) )
             get_knownalive_domain(d);
         page_list_add_tail(page, &d->xenpage_list);




[-- Attachment #2: VMX-domheap-APIC-access-page.patch --]
[-- Type: text/plain, Size: 3130 bytes --]

VMX: allocate APIC access page from domain heap

... since we don't need its virtual address anywhere (it's a
placeholder page only after all). For this wo work (and possibly be
done elsewhere too) share_xen_page_with_guest() needs to mark pages
handed to it as Xen heap ones.

To be on the safe side, also explicitly clear the page (not having done
so was okay due to the XSA-100 fix, but is still a latent bug since we
don't formally guarantee allocations to come out zeroed, and in fact
this property may disappear again as soon as the asynchronous runtime
scrubbing patches arrive).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Alternatives might be to use a
- global (or perhaps per-node) page across VMs (on the basis that VMs
  shouldn't be writing into that page anyway)
- fake MFN pointing into nowhere (would need to ensure no side effects
  can occur, like PCIe errors or NMIs)

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2402,19 +2402,21 @@ gp_fault:
 
 static int vmx_alloc_vlapic_mapping(struct domain *d)
 {
-    void *apic_va;
+    struct page_info *pg;
+    unsigned long mfn;
 
     if ( !cpu_has_vmx_virtualize_apic_accesses )
         return 0;
 
-    apic_va = alloc_xenheap_page();
-    if ( apic_va == NULL )
+    pg = alloc_domheap_page(d, MEMF_no_owner);
+    if ( !pg )
         return -ENOMEM;
-    share_xen_page_with_guest(virt_to_page(apic_va), d, XENSHARE_writable);
-    d->arch.hvm_domain.vmx.apic_access_mfn = virt_to_mfn(apic_va);
-    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
-                       _mfn(virt_to_mfn(apic_va)), PAGE_ORDER_4K,
-                       p2m_get_hostp2m(d)->default_access);
+    mfn = page_to_mfn(pg);
+    clear_domain_page(mfn);
+    share_xen_page_with_guest(pg, d, XENSHARE_writable);
+    d->arch.hvm_domain.vmx.apic_access_mfn = mfn;
+    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), _mfn(mfn),
+                       PAGE_ORDER_4K, p2m_get_hostp2m(d)->default_access);
 
     return 0;
 }
@@ -2422,8 +2424,16 @@ static int vmx_alloc_vlapic_mapping(stru
 static void vmx_free_vlapic_mapping(struct domain *d)
 {
     unsigned long mfn = d->arch.hvm_domain.vmx.apic_access_mfn;
+
     if ( mfn != 0 )
-        free_xenheap_page(mfn_to_virt(mfn));
+    {
+        struct page_info *pg = mfn_to_page(mfn);
+
+        if ( test_and_clear_bit(_PGC_allocated, &pg->count_info) )
+            put_page(pg);
+        clear_bit(_PGC_xen_heap, &pg->count_info);
+        free_domheap_page(pg);
+    }
 }
 
 static void vmx_install_vlapic_mapping(struct vcpu *v)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -454,7 +454,7 @@ void share_xen_page_with_guest(
     /* Only add to the allocation list if the domain isn't dying. */
     if ( !d->is_dying )
     {
-        page->count_info |= PGC_allocated | 1;
+        page->count_info |= PGC_xen_heap | PGC_allocated | 1;
         if ( unlikely(d->xenheap_pages++ == 0) )
             get_knownalive_domain(d);
         page_list_add_tail(page, &d->xenpage_list);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] VMX: allocate APIC access page from domain heap

[Andrew Cooper]

On 16/12/15 07:57, Jan Beulich wrote:
> ... since we don't need its virtual address anywhere (it's a
> placeholder page only after all). For this wo work (and possibly be

"to work"

> done elsewhere too) share_xen_page_with_guest() needs to mark pages
> handed to it as Xen heap ones.
>
> To be on the safe side, also explicitly clear the page (not having done
> so was okay due to the XSA-100 fix, but is still a latent bug since we
> don't formally guarantee allocations to come out zeroed, and in fact
> this property may disappear again as soon as the asynchronous runtime
> scrubbing patches arrive).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Alternatives might be to use a
> - global (or perhaps per-node) page across VMs (on the basis that VMs
>   shouldn't be writing into that page anyway)

I am surprised by the concern about a zeroed page, yet not concerned by
sharing this page across VMs.

Even though we can get away with sharing it across VMs, we should
probably avoid it, to be on the safe side.  It is only 1 page per VM.

> - fake MFN pointing into nowhere (would need to ensure no side effects
>   can occur, like PCIe errors or NMIs)
>
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2402,19 +2402,21 @@ gp_fault:
>  
>  static int vmx_alloc_vlapic_mapping(struct domain *d)
>  {
> -    void *apic_va;
> +    struct page_info *pg;
> +    unsigned long mfn;
>  
>      if ( !cpu_has_vmx_virtualize_apic_accesses )
>          return 0;
>  
> -    apic_va = alloc_xenheap_page();
> -    if ( apic_va == NULL )
> +    pg = alloc_domheap_page(d, MEMF_no_owner);
> +    if ( !pg )
>          return -ENOMEM;
> -    share_xen_page_with_guest(virt_to_page(apic_va), d, XENSHARE_writable);
> -    d->arch.hvm_domain.vmx.apic_access_mfn = virt_to_mfn(apic_va);
> -    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
> -                       _mfn(virt_to_mfn(apic_va)), PAGE_ORDER_4K,
> -                       p2m_get_hostp2m(d)->default_access);
> +    mfn = page_to_mfn(pg);
> +    clear_domain_page(mfn);
> +    share_xen_page_with_guest(pg, d, XENSHARE_writable);
> +    d->arch.hvm_domain.vmx.apic_access_mfn = mfn;
> +    set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), _mfn(mfn),
> +                       PAGE_ORDER_4K, p2m_get_hostp2m(d)->default_access);
>  
>      return 0;
>  }
> @@ -2422,8 +2424,16 @@ static int vmx_alloc_vlapic_mapping(stru
>  static void vmx_free_vlapic_mapping(struct domain *d)
>  {
>      unsigned long mfn = d->arch.hvm_domain.vmx.apic_access_mfn;
> +
>      if ( mfn != 0 )
> -        free_xenheap_page(mfn_to_virt(mfn));
> +    {
> +        struct page_info *pg = mfn_to_page(mfn);
> +
> +        if ( test_and_clear_bit(_PGC_allocated, &pg->count_info) )
> +            put_page(pg);
> +        clear_bit(_PGC_xen_heap, &pg->count_info);

This is a different teardown procedure to the allocation.  Instead, it
would be better to either ASSERT that _PGC_allocated is set, or move the
_PGC_xen_heap change into the if() condition.

~Andrew

> +        free_domheap_page(pg);
> +    }
>  }
>  
>  static void vmx_install_vlapic_mapping(struct vcpu *v)
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -454,7 +454,7 @@ void share_xen_page_with_guest(
>      /* Only add to the allocation list if the domain isn't dying. */
>      if ( !d->is_dying )
>      {
> -        page->count_info |= PGC_allocated | 1;
> +        page->count_info |= PGC_xen_heap | PGC_allocated | 1;
>          if ( unlikely(d->xenheap_pages++ == 0) )
>              get_knownalive_domain(d);
>          page_list_add_tail(page, &d->xenpage_list);
>
>
>

Re: [PATCH] VMX: allocate APIC access page from domain heap

[Jan Beulich]

>>> On 16.12.15 at 13:11, <andrew.cooper3@citrix.com> wrote:
> On 16/12/15 07:57, Jan Beulich wrote:
>> Alternatives might be to use a
>> - global (or perhaps per-node) page across VMs (on the basis that VMs
>>   shouldn't be writing into that page anyway)
> 
> I am surprised by the concern about a zeroed page, yet not concerned by
> sharing this page across VMs.

Well, exposing Xen data would be a problem, wouldn't it? Having a
buggy VM expose its internals to another VM wouldn't be a concern
to me.

>> @@ -2422,8 +2424,16 @@ static int vmx_alloc_vlapic_mapping(stru
>>  static void vmx_free_vlapic_mapping(struct domain *d)
>>  {
>>      unsigned long mfn = d->arch.hvm_domain.vmx.apic_access_mfn;
>> +
>>      if ( mfn != 0 )
>> -        free_xenheap_page(mfn_to_virt(mfn));
>> +    {
>> +        struct page_info *pg = mfn_to_page(mfn);
>> +
>> +        if ( test_and_clear_bit(_PGC_allocated, &pg->count_info) )
>> +            put_page(pg);
>> +        clear_bit(_PGC_xen_heap, &pg->count_info);
> 
> This is a different teardown procedure to the allocation.  Instead, it
> would be better to either ASSERT that _PGC_allocated is set, or move the
> _PGC_xen_heap change into the if() condition.

I don't follow - the put_page() + clear_bit() invert the
share_xen_page_with_guest(). Less the clear_bit() (needed just
because of this being a domheap allocation) that's how this gets
done elsewhere too. Or maybe I don't understand your concern?

Jan

Re: [PATCH] VMX: allocate APIC access page from domain heap

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, December 16, 2015 3:58 PM
> 
> ... since we don't need its virtual address anywhere (it's a
> placeholder page only after all). For this wo work (and possibly be
> done elsewhere too) share_xen_page_with_guest() needs to mark pages
> handed to it as Xen heap ones.
> 
> To be on the safe side, also explicitly clear the page (not having done
> so was okay due to the XSA-100 fix, but is still a latent bug since we
> don't formally guarantee allocations to come out zeroed, and in fact
> this property may disappear again as soon as the asynchronous runtime
> scrubbing patches arrive).

Do you mean that asynchronous scrubbing may scrub page after they
are re-allocated? It's unrelated but a bit risky to me.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Alternatives might be to use a
> - global (or perhaps per-node) page across VMs (on the basis that VMs
>   shouldn't be writing into that page anyway)
> - fake MFN pointing into nowhere (would need to ensure no side effects
>   can occur, like PCIe errors or NMIs)

one page per VM should be fine. We don't need go that route.

the patch overall looks OK to me:

Acked-by: Kevin Tian <kevin.tian@intel.com>

Thanks
Kevin

Re: [PATCH] VMX: allocate APIC access page from domain heap

[Jan Beulich]

>>> On 18.12.15 at 07:29, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, December 16, 2015 3:58 PM
>> 
>> ... since we don't need its virtual address anywhere (it's a
>> placeholder page only after all). For this wo work (and possibly be
>> done elsewhere too) share_xen_page_with_guest() needs to mark pages
>> handed to it as Xen heap ones.
>> 
>> To be on the safe side, also explicitly clear the page (not having done
>> so was okay due to the XSA-100 fix, but is still a latent bug since we
>> don't formally guarantee allocations to come out zeroed, and in fact
>> this property may disappear again as soon as the asynchronous runtime
>> scrubbing patches arrive).
> 
> Do you mean that asynchronous scrubbing may scrub page after they
> are re-allocated? It's unrelated but a bit risky to me.

No, the issue would have been exposure of hypervisor or foreign VM
data, i.e. an information leak.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Alternatives might be to use a
>> - global (or perhaps per-node) page across VMs (on the basis that VMs
>>   shouldn't be writing into that page anyway)
>> - fake MFN pointing into nowhere (would need to ensure no side effects
>>   can occur, like PCIe errors or NMIs)
> 
> one page per VM should be fine. We don't need go that route.
> 
> the patch overall looks OK to me:

Except that it's broken - it turns out that I only thought I've tested
this, while in fact I had tested a stale binary. v2 on its way.

Jan