From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH for-4.5 v10 15/19] xen/arm: Temporarily
 disable mem_access for hypervisor access
Date: Thu, 25 Sep 2014 18:19:09 +0200
Message-ID: <5424407D.70904@linaro.org>
References: <1411646212-17041-1-git-send-email-tklengyel@sec.in.tum.de>
	<1411646212-17041-16-git-send-email-tklengyel@sec.in.tum.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1411646212-17041-16-git-send-email-tklengyel@sec.in.tum.de>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Tamas K Lengyel <tklengyel@sec.in.tum.de>, xen-devel@lists.xen.org
Cc: ian.campbell@citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, stefano.stabellini@citrix.com, andres@lagarcavilla.org, jbeulich@suse.com, dgdegra@tycho.nsa.gov
List-Id: xen-devel@lists.xenproject.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