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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/08/15 13:32, Ben Catterall wrote:
>
>
> On 06/08/15 22:24, Andrew Cooper wrote:
>> On 06/08/2015 17:45, Ben Catterall wrote:
>>> Added trap handlers to catch exceptions such as a page fault, general
>>> protection fault, etc. These handlers will crash the domain as such
>>> exceptions
>>> would indicate that either there is a bug in deprivileged mode or it
>>> has been
>>> compromised by an attacker.
>>>
>>> Signed-off-by: Ben Catterall <Ben.Catterall@citrix.com>
>>> ---
>>>   xen/arch/x86/mm/hap/hap.c |  9 +++++++++
>>>   xen/arch/x86/traps.c      | 41
>>> ++++++++++++++++++++++++++++++++++++++++-
>>>   2 files changed, 49 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
>>> index abc5113..43bde89 100644
>>> --- a/xen/arch/x86/mm/hap/hap.c
>>> +++ b/xen/arch/x86/mm/hap/hap.c
>>> @@ -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);
>> %pv is your friend.  Like Linux, we have custom printk formats.  In this
>> case, passing 'v' as a parameter to %pv will cause d$Xv$Y to be
>> printed.  (The example below predates %pv being introduced).
> ok, will do. thanks!
>>
>>> +        domain_crash_synchronous();
>> No need for _synchronous() here.  _synchronous() should only be used
>> when you can't safely recover.  It ends up spinning in a tight loop
>> waiting for the next timer interrupt, is anything up to 30ms away.
> I'm not sure if we can safely recover from this. This will only be
> triggered if there is a bug in depriv mode
> or if the mode has been compromised and an attacker has tried to
> access unavailable memory.
> From my understanding (am I missing something?): domain_crash
> effectively sets flags to tell the scheduler that
> it should be killed the next time the scheduler runs and then returns.
> In which case, if we don't do a
> synchronous crash, this return path would return back into the
> deprivileged mode, we would not
> have mapped in the page (as we shouldn't), and then we get another fault.
>
> What do you think is the best way forward? Thanks!
>

Given that there is a use of domain_crash(d) in context below, it is
clearly safe to use from here.  (Although my general point about hap vs
shadow code still applies, meaning that hap_page_fault() is not the
correct function to hook like this.)

domain_crash() sets a flag, but exiting out from a fault handler heading
back towards ring3 code should check for pending softirqs.  However,
because of the way you have hooked return-to-depriv, you might have
broken this.

~Andrew