Re: [PATCH for-4.5 v11 5/9] xen/arm: Allow hypervisor access to mem_access protected pages

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

Hi Tamas,

On 09/29/2014 12:36 PM, 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 access_in_use 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@sec.in.tum.de>
> ---
>  xen/arch/arm/guestcopy.c | 120 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 117 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 0173597..b0727b1 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -6,6 +6,111 @@
>  
>  #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;
> +    xenmem_access_t xma;
> +    p2m_type_t t;
> +
> +    rc = gva_to_ipa(gva, &ipa);

I think you have to extend gva_to_ipa to take the flag in parameter.
Otherwise you may end up to write in read-only page from the guest POV.


> +    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:

access_rwx is used to say there is no permission, right? If so, why
don't you continue to check permission?

> +    case XENMEM_access_rw:
> +        return -EFAULT;
> +    case XENMEM_access_n2rwx:
> +    case XENMEM_access_n:
> +    case XENMEM_access_x:
> +        break;
> +    case XENMEM_access_wx:
> +    case XENMEM_access_w:
> +        if ( flag == GV2M_READ )
> +            break;
> +        else return -EFAULT;
> +    case XENMEM_access_rx2rw:
> +    case XENMEM_access_rx:
> +    case XENMEM_access_r:
> +        if ( flag == GV2M_WRITE )
> +            break;
> +        else return -EFAULT;
> +    }
> +
> +    /*
> +     * We had a mem_access permission limiting the access, but the page type
> +     * could also be limiting, so we need to check that as well.
> +     */

With your current solution, if an hypercall is trying to read/write to a
restricted hypercall page, it will fail.

I though the goal was to skip mem access stuff even if the access is
restricted?

> +    maddr = p2m_lookup(current->domain, ipa, &t);
> +    if ( maddr == INVALID_PADDR )
> +        return -EFAULT;
> +
> +    /*
> +     * All page types are readable so we only have to check the
> +     * type if writing.
> +     */
> +    if ( flag == GV2M_WRITE )
> +    {
> +        switch ( t )
> +        {
> +        case p2m_ram_rw:
> +        case p2m_iommu_map_rw:
> +        case p2m_map_foreign:
> +        case p2m_grant_map_rw:
> +        case p2m_mmio_direct:
> +            /* Base type allows writing, we are good to get the page. */
> +            break;
> +        default:
> +            return -EFAULT;
> +        }
> +    }
> +
> +    *page = mfn_to_page(maddr >> PAGE_SHIFT);

You can use maddr_to_page here.

> +    ASSERT(*page);

mfn_to_page only returns a valid pointer if the MFN is valid (see
mfn_valid).

Regards,

-- 
Julien Grall