Xen project Mailing List

On Mon, Sep 29, 2014 at 4:12 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

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@xxxxxxxxxxxxx>
> ---
> 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?

So things are backward here. There has already been a MMU fault (get_page_from_gva failed) and we are trying to determine the cause of that fault. If the mem_access permission is rwx or rw, that means it had nothing to do with it so we go back to the original path.

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

It's the other way around. If the hypercall is trying to read/write to a mem_access protected page, it will be allowed to go through (provided that the base-type also allows the operation).

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

Ack.

> + ASSERT(*page);

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

Hm, I copied this from get_page_from_gva but I'll remove it.

Thanks!

Tamas

Regards,

--
Julien Grall

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

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