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

[Julien Grall <julien.grall@linaro.org>]

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?

> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> ---
>   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 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.

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;
      }

      copy the data

Regards,

-- 
Julien Grall