Xen project Mailing List

On Thu, Sep 25, 2014 at 6:19 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

Hello Tamas,

On 25/09/2014 13:56, Tamas K Lengyel wrote:

The guestcopy helpers use the MMU to verify that the given guest has read/write
access to a given page during hypercalls. As we may have custom mem_access
permissions set on these pages, we temporarily disable them to allow Xen to
finish the hypercalls. This is permissible as mem_access events are only
reported for events when the guest directly accesses protected memory on x86
as well.

IHMO, copying data from/to the guest could be consider as a guest access.

How does x86 handle this case?

In the thread I mentioned in the previous discussion, on x86 mem_access doesn't trap on accesses performed by the hypervisor or if another domain is accessing the memory via grants. On x86 mem_access is purely EPT traps that the guest caused. This is not ideal and in the future it could be extended to make mem_access into a real memory-protection system, not just a memory-protection system against the particular guest, but that would require significant more work.

The problem is that memory accesses are re-tried automatically after the second-stage trap was hit and the permission are cleared. Here we would need to make the hypervisor wait for a mem_access reply before it can continue (or fail), and that has a lot of pitfalls (faulty mem_access listener etc..).

Signed-off-by: Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx>
---
xen/arch/arm/guestcopy.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 0173597..4aa041f 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -6,6 +6,43 @@

#include <asm/mm.h>
#include <asm/guest_access.h>
+#include <asm/p2m.h>
+
+/*
+ * Temporarily disable mem_access permission restrictions.
+ * Note: In the future, events generated by the hypervisor accessing
+ * protected memory regions could be added here.
+ */
+static long temp_disable_mem_access(vaddr_t gva, unsigned long *gfn,
+ xenmem_access_t *xma)
+{
+ long rc;
+ paddr_t gpa;
+
+ rc = gva_to_ipa((vaddr_t) gva, &gpa);
+ if ( rc < 0 )
+ return rc;
+
+ *gfn = paddr_to_pfn(gpa);
+
+ rc = p2m_get_mem_access(current->domain, *gfn, xma);
+ if ( rc < 0 )
+ return rc;
+
+ if ( *xma != XENMEM_access_rwx )
+ rc = p2m_set_mem_access(current->domain, *gfn, 1, 0, ~0,
+ XENMEM_access_rwx);
+
+ return rc;
+}

When mem_access is not in use you are adding another translation and therefore slowing down hypercall for everyone.

I could check if access_in_use is flipped and only do the translation+lookup then.

I don't think that modifying temporary the permission is the right thing to do because:
- p2m_set_mem_access is called 2 times which means 2 TLB flush (and I'm not counting the table mapping), ie it's very slow
- The other VCPU of the guest are still running. So you may not catch unwanted access.

That is a problem. The only way around that I see is to pause the domain for the duration of this copy in case the mem_access permissions need to be disabled.

IHMO, the best solution would be smth like:

page = get_page_from_gva(...)
if ( !page )
{
check mem access and getting the page
if ( !page )
return rc;
}

So you mean only check the mem_access permissions when we failed to get the page. I'm not sure what you propose afterwards. If there is a mem_access restriction, just return an -errno? It would mean if a mem_access listener is trapped that page than the guest can't execute the hypercall. Since we would also want this system to be invisible to the guest, that I'm affraid is not a good approach.

Tamas

copy the data

Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH for-4.5 v10 15/19] xen/arm: Temporarily disable mem_access for hypervisor access