Re: [Xen-devel] [RFC PATCH v3 10/10] arm/mem_access: Walk the guest's pt in software

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jun 15, 2017 at 5:05 AM, Sergej Proskurin
<proskurin@xxxxxxxxxxxxx> wrote:
> In this commit, we make use of the gpt walk functionality introduced in
> the previous commits. If mem_access is active, hardware-based gva to ipa
> translation might fail, as gva_to_ipa uses the guest's translation
> tables, access to which might be restricted by the active VTTBR. To
> side-step potential translation errors in the function
> p2m_mem_access_check_and_get_page due to restricted memory (e.g. to the
> guest's page tables themselves), we walk the guest's page tables in
> software.
>
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> Cc: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
> v2: Check the returned access rights after walking the guest's page tables in
>     the function p2m_mem_access_check_and_get_page.
>
> v3: Adapt Function names and parameter.
> ---
>  xen/arch/arm/mem_access.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index 04b1506b00..acb5539bb6 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -22,6 +22,7 @@
>  #include <xen/vm_event.h>
>  #include <public/vm_event.h>
>  #include <asm/event.h>
> +#include <asm/guest_walk.h>
>
>  static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>                                  xenmem_access_t *access)
> @@ -101,6 +102,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag,
>                                    const struct vcpu *v)
>  {
>      long rc;
> +    unsigned int perms;
>      paddr_t ipa;
>      gfn_t gfn;
>      mfn_t mfn;
> @@ -110,8 +112,25 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned 
> long flag,
>      struct p2m_domain *p2m = &v->domain->arch.p2m;
>
>      rc = gva_to_ipa(gva, &ipa, flag);
> +
> +    /*
> +     * In case mem_access is active, hardware-based gva_to_ipa translation
> +     * might fail. Since gva_to_ipa uses the guest's translation tables, 
> access
> +     * to which might be restricted by the active VTTBR, we perform a gva to
> +     * ipa translation in software.
> +     */
>      if ( rc < 0 )
> -        goto err;
> +    {
> +        if ( guest_walk_tables(v, gva, &ipa, &perms) < 0 )
> +            /*
> +             * The software gva to ipa translation can still fail, e.g., if 
> the
> +             * gva is not mapped.
> +             */
> +            goto err;
> +
> +        if ( ((flag & GV2M_WRITE) == GV2M_WRITE) && !(perms & GV2M_WRITE) )

Wouldn't it be enough to do (flag & GV2M_WRITE) without the following
comparison? Also, a comment explaining why this is an error-condition
would be nice.

> +            goto err;
> +    }
>
>      gfn = _gfn(paddr_to_pfn(ipa));
>
> --
> 2.12.2

Thanks,
Tamas

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