Re: [Xen-devel] [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-03-06 at 22:24 +0100, 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 do a software-based type checking in case
> the MMU based approach failed, but only if mem_access_enabled is set.
> 
> These memory accesses are not forwarded to the mem_event listener. Accesses
> performed by the hypervisor are currently not part of the mem_access scheme.
> This is consistent behaviour with the x86 side as well.
> 
> Signed-off-by: Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx>
> ---
> v12: - Check for mfn_valid as well.
> ---
>  xen/arch/arm/guestcopy.c | 124 
> +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 121 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 7dbaeca..d42a469 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -6,6 +6,115 @@
>  
>  #include <asm/mm.h>
>  #include <asm/guest_access.h>
> +#include <asm/p2m.h>
> +
> +/*
> + * If mem_access is in use it might have been the reason why 
> get_page_from_gva
> + * failed to fetch the page, as it uses the MMU for the permission checking.
> + * Only in these cases we do a software-based type check and fetch the page 
> if
> + * we indeed found a conflicting mem_access setting.
> + */
> +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> +                               struct page_info** page)
> +{
> +    long rc;
> +    paddr_t ipa;
> +    unsigned long maddr;
> +    unsigned long mfn;
> +    xenmem_access_t xma;
> +    p2m_type_t t;
> +
> +    rc = gva_to_ipa(gva, &ipa);
> +    if ( rc < 0 )
> +        return rc;
> +
> +    /*
> +     * We do this first as this is faster in the default case when no
> +     * permission is set on the page.
> +     */
> +    rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
> +    if ( rc < 0 )
> +        return rc;
> +
> +    /* Let's check if mem_access limited the access. */
> +    switch ( xma )
> +    {
> +    default:
> +    case XENMEM_access_rwx:
> +    case XENMEM_access_rw:

If I've understood correctly then this is deliberately backwards
looking.

i.e. if we have gotten here then we have failed an earlier
get_page_from_gva check, so if xma is XENMEM_access_rwx that means that
the result of that first get_page_from_gva is valid because memaccess
hasn't been messing with the permissions.

Since this interface only does reads or writes and not executableness
rwx and rw are effectively the same.

Is my understanding correct?

> +        return -EFAULT;
> +    case XENMEM_access_n2rwx:
> +    case XENMEM_access_n:
> +    case XENMEM_access_x:
> +        break;

If xenaccess contains no rw perms at all then we need to check what the
original p2m type was in order to decide what to do.

> +    case XENMEM_access_wx:
> +    case XENMEM_access_w:
> +        if ( flag == GV2M_READ )
> +            break;
> +        else return -EFAULT;

If thing was a read then it might be because of xenaccess, so we must
check, but if it was a write then the origianl get_page_from_gva fault
was correct.


> +    case XENMEM_access_rx2rw:
> +    case XENMEM_access_rx:
> +    case XENMEM_access_r:
> +        if ( flag == GV2M_WRITE )
> +            break;
> +        else return -EFAULT;

and vice versa.

(sorry to be so tedious, I wanted to work through them all to ensure I'd
understood correctly and that they were right).

> @@ -68,7 +180,10 @@ unsigned long raw_clear_guest(void *to, unsigned len)
>  
>          page = get_page_from_gva(current->domain, (vaddr_t) to, GV2M_WRITE);
>          if ( page == NULL )
> -            return len;
> +        {
> +            if ( check_mem_access((vaddr_t) to, GV2M_WRITE, &page) < 0 )
> +                return len;
> +        }

I think this would be better done by a making check_mem_access include
the initial call to get_page_from_gva and calling that helper here
instead instead of repeating this code.

I'd even consider putting the memaccess check as a call at the tail end
of get_page_from_gva, I don't think there are any callers who don't want
this behaviour.



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