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

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

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