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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 9/23/2014 10: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@xxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@xxxxxxx>
> ---
>   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/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;
Why remove the above line?
It's common for all cases below.
We can leave it as it is and set it selectively to 
X86_EVENTTYPE_SW_INTERRUPT as
you do in 'case X86_EVENTTYPE_SW_INTERRUPT ' right?

-Aravind.

>       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.
> +         */
> +        if ( cpu_has_svm_nrips )
> +            vmcb->nextrip = regs->eip;
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        break;
> +
> +    case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */
> +        /*
> +         * The AMD manual states that .type=3 (HW exception), .vector=3 or 4,
> +         * will perform DPL checks.  Experimentally, DPL and presence checks
> +         * are indeed performed, even without NextRIP support.
> +         *
> +         * However without NextRIP support, the event injection still needs
> +         * fully emulating to get the correct eip in the trap frame, yet get
> +         * the correct faulting eip should a fault occur.
> +         */
> +        if ( cpu_has_svm_nrips )
> +            vmcb->nextrip = regs->eip + _trap.insn_len;
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        break;
> +
> +    default:
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
> +        event.fields.errorcode = _trap.error_code;
> +        break;
> +    }
>   

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