Re: [Xen-devel] [PATCH] arm/mem_access: properly handle traps caused by no-longer current settings

[Julien Grall <julien.grall@xxxxxxx>]

Hello Tamas,

Thank you for taking care of this bug.

On 02/08/2016 19:53, Tamas K Lengyel wrote:
> When mem_access settings change, the active vCPUs may still cause a violation
> until the TLB gets flushed. Instead of just reinjecting the violation to the
> guest, in this patch we direct the vCPU to retry the access where
> appropriate or we crash the domain where the mem_access settings are
> corrupted.
With this patch p2m_mem_access_check will always return false. So I am 
not sure why you want to return in p2m_mem_access_check.
> Requested-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/p2m.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 40a0b80..a4b6b7b 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1657,8 +1657,26 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, 
> const struct npfec npfec)
>          return true;
>
>      rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), &xma);
> -    if ( rc )
> -        return true;
> +    switch (rc )
> +    {
> +    case -ESRCH:
> +        /*
> +         * If we can't find any mem_access setting for this page then the page
> +         * might have just been removed and the event was triggered by no 
> longer
> +         * valid settings. The vCPU should just retry to get to the proper 
> error
> +         * path.
> +         */
> +        return false;
> +    case -ERANGE:
> +        /*
> +         * The mem_access settings are corrupted. Crashing the domain is the
> +         * appropriate step in this case.
> +         */
> +        domain_crash(v->domain);
> +        return false;
> +    };
> +
> +    ASSERT(!rc);
It would be easier to do:

rc = p2m_mem_access_check(gpa, gva, npfec);
if (!rc)
  return;

by

p2m_mem_access_check(gpa, gva, npfec);
return;

in both do_trap_instr_abort_guest and do_trap_data_abort_guest.

This would also helps to fallback on another permission check if in the 
future we decide to use permission for other reasons.
Or is there any reason you may want to inject a data abort to the guest 
if memaccess has failed (i.e return true)?
Regards,

--
Julien Grall

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