Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

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

On 24/09/14 14:24, Boris Ostrovsky wrote:
> On 09/24/2014 09:04 AM, Andrew Cooper wrote:
>> On 24/09/14 14:01, Boris Ostrovsky wrote:
>>> On 09/23/2014 11:03 AM, Andrew Cooper wrote:
>>>> AMD SVM requires all software events to have their injection
>>>> emulated if
>>>> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1)
>>>> injection
>>>> requires emulation in all cases, even with hardware NextRIP support.
>>>>
>>>> Emulating full control transfers is overkill for our needs.  All that
>>>> matters
>>>> is that guest userspace can't bypass the descriptor DPL check.  Any
>>>> guest OS
>>>> which would incur other faults as part of injection is going to end
>>>> up with a
>>>> double fault instead, and won't be in a position to care that the
>>>> faulting eip
>>>> is wrong.
>>>>
>>>> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>>> ---
>>>>    xen/arch/x86/hvm/emulate.c             |    8 +++
>>>>    xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>>>>    xen/arch/x86/mm.c                      |    2 +
>>>>    xen/arch/x86/mm/shadow/common.c        |    1 +
>>>>    xen/arch/x86/x86_emulate/x86_emulate.c |  122
>>>> ++++++++++++++++++++++++++++++--
>>>>    xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>>>>    6 files changed, 191 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>>> index 7ee146b..463ccfb 100644
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -21,6 +21,7 @@
>>>>    #include <asm/hvm/hvm.h>
>>>>    #include <asm/hvm/trace.h>
>>>>    #include <asm/hvm/support.h>
>>>> +#include <asm/hvm/svm/svm.h>
>>>>      static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>>>>    {
>>>> @@ -1328,6 +1329,13 @@ static int _hvm_emulate_one(struct
>>>> hvm_emulate_ctxt *hvmemul_ctxt,
>>>>        vio->mmio_retrying = vio->mmio_retry;
>>>>        vio->mmio_retry = 0;
>>>>    +    if ( cpu_has_vmx )
>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
>>>> +    else if ( cpu_has_svm_nrips )
>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
>>>> +    else
>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
>>>> +
>>>>        rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>>>>          if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>>> index de982fd..b6beefc 100644
>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap
>>>> *trap)
>>>>        struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>>>>        eventinj_t event = vmcb->eventinj;
>>>>        struct hvm_trap _trap = *trap;
>>>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>>          switch ( _trap.vector )
>>>>        {
>>>>        case TRAP_debug:
>>>> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>>>> +        if ( regs->eflags & X86_EFLAGS_TF )
>>>>            {
>>>>                __restore_debug_registers(vmcb, curr);
>>>>                vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
>>>> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap
>>>> *trap)
>>>>          event.bytes = 0;
>>>>        event.fields.v = 1;
>>>> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>>        event.fields.vector = _trap.vector;
>>>> -    event.fields.ev = (_trap.error_code !=
>>>> HVM_DELIVER_NO_ERROR_CODE);
>>>> -    event.fields.errorcode = _trap.error_code;
>>>> +
>>>> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event
>>>> Injection. */
>>>> +    switch ( _trap.type )
>>>> +    {
>>>> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
>>>> +        /*
>>>> +         * Injection type 4 (software interrupt) is only supported
>>>> with
>>>> +         * NextRIP support.  Without NextRIP, the emulator will have
>>>> performed
>>>> +         * DPL and presence checks for us.
>>>> +         */
>>>> +        if ( cpu_has_svm_nrips )
>>>> +        {
>>>> +            vmcb->nextrip = regs->eip + _trap.insn_len;
>>>> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
>>>> +        }
>>>> +        else
>>>> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>> +        break;
>>>> +
>>>> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
>>>> +        /*
>>>> +         * icebp's injection must always be emulated.  Software
>>>> injection help
>>>> +         * in x86_emulate has moved eip forward, but NextRIP (if
>>>> used) still
>>>> +         * needs setting or execution will resume from 0.
>>>> +         */
>>>
>>> Can you tell me where eip is updated? I don't see any difference
>>> between how, for example, int3 is emulated differently from icebp.
>>>
>>> -boris
>> The second hunk in this series, chooses between x86_swint_emulate_icebp
>> and x86_swint_emulate_all based on NRIP support.
>>
>> This cause inject_swint() to either emulate the injection or not.
>
> I was asking about why you are calculating nextrip differently (i.e
> not adding _trap.insn_len) for icebp vs. int3. I read the comment as
> if it was saying that x86_emulate() has already taken care of that.
> And I don't understand where that is done.

With NRIPS support, hardware can deal with injecting the trap and doing
DPL/presence checks, which means the eip in the exception frame might
either be a trap or a fault.

regs->eip and vmcb->nextrip bound the trapping instruction, with eip
pointing at it, and nextrip pointing after it.  These are the two
choices which get set in the exception frame, after hardware has decided
whether to send the trap, or fault because of a bad IDT setup.

Without NRIP support, the emulator must move regs->eip on to get the
trap frame eip pointing at the correct address as it is the only
available choice for an exception frame.

For ICEBP, there is no hardware support for injection at all (even with
NRIP available).  The emulator must move regs->eip forward to get the
correct trap eip for the non-NRIP case, but with NRIP support,
vmcb->nextrip must be set, or the resulting trap frame contains an eip of 0.

I suppose this looks a little weird because of the intersection of
x86_swint_emulate_icebp and x86_swint_emulate_all, but it is not correct
to inject a software event with NRIP support for icebp, as that would
cause hardware to do a DPL check and fail to set the external bit in a
#NP error code.

~Andrew