Re: [RFC 4/4] HVM x86 deprivileged mode: Trap handlers for deprivileged mode

[Ben Catterall <Ben.Catterall@citrix.com>]

On 11/08/15 11:33, Ben Catterall wrote:
>
>
> On 10/08/15 11:07, Tim Deegan wrote:
>> Hi,
>>
>>> @@ -685,8 +685,17 @@ static int hap_page_fault(struct vcpu *v,
>>> unsigned long va,
>>>   {
>>>       struct domain *d = v->domain;
>>>
>>> +    /* If we get a page fault whilst in HVM security user mode */
>>> +    if( v->user_mode == 1 )
>>> +    {
>>> +        printk("HVM: #PF (%u:%u) whilst in user mode\n",
>>> +                 d->domain_id, v->vcpu_id);
>>> +        domain_crash_synchronous();
>>> +    }
>>> +
>>
>> This should happen in paging_fault() so it can guard the
>> shadow-pagetable paths too.  Once it's there, it'll need a check for
>> is_hvm_vcpu() as well as for user_mode.  Maybe have a helper function
>> 'is_hvm_deprivileged_vcpu()' to do both checks, also used in
>> hvm_deprivileged_check_trap() &c.
I've moved this and now need to add shadow page table support as this 
currently only supports HAP.
>>
> Ok, I'll make this change.
>>>       HAP_ERROR("Intercepted a guest #PF (%u:%u) with HAP enabled.\n",
>>>                 d->domain_id, v->vcpu_id);
>>> +
>>>       domain_crash(d);
>>>       return 0;
>>>   }
>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>>> index 9f5a6c6..19d465f 100644
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -74,6 +74,7 @@
>>>   #include <asm/vpmu.h>
>>>   #include <public/arch-x86/cpuid.h>
>>>   #include <xsm/xsm.h>
>>> +#include <xen/hvm/deprivileged.h>
>>>
>>>   /*
>>>    * opt_nmi: one of 'ignore', 'dom0', or 'fatal'.
>>> @@ -500,6 +501,11 @@ static void do_guest_trap(
>>>       struct trap_bounce *tb;
>>>       const struct trap_info *ti;
>>>
>>> +    /* If we take the trap whilst in HVM deprivileged mode
>>> +     * then we should crash the domain.
>>> +     */
>>> +    hvm_deprivileged_check_trap(__FUNCTION__);
>>
>> I wonder whether it would be better to switch to an IDT with all
>> unacceptable traps stubbed out, rather than have to blacklist them all
>> separately.  Probably not - this check is cheap, and maintaining the
>> parallel tables would be a pain.
>>
>> Or maybe there's some single point upstream of here, in the asm
>> handlers, that would catch all the cases where this check is needed?
>>
> Yep, I think this can be done.
Had a deeper look at this. There is a point where all exceptions come in 
in the asm (handle_exception in entry.S) and we could branch out at this 
point. It does mean that we'd treat _all_ exceptions that occur in this 
mode the same way whereas the current way means that we can treat them 
differently (e.g. get __func__). So, should I make all exceptions go to 
the same point or keep as is?

Thanks!
>> In any case, the check needs to return an error code so the caller
>> knows to return without running the rest of the handler (and likewise
>> elsewhere).
>>
> understood.
>> Cheers,
>>
>> Tim.
>>