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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Tue, Aug 2, 2016 at 2:02 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> 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.

That's not the case, it returns true if mem_access is not enabled on
the domain, which means whatever caused the trap wasn't mem_access and
thus we should fall back on the default behavior, which is injecting
the fault to the guest.

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

Yes, if the fault wasn't caused by mem_access (ie. it's not enabled on
the domain).

Tamas

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