RE: [PATCH 06/16] vmx: nest: handling VMX instruction exits

["Dong, Eddie" <eddie.dong@intel.com>]

Tim Deegan wrote:
> At 16:22 +0100 on 08 Sep (1283962934), Qing He wrote:
>> diff -r f1c1d3077337 xen/arch/x86/hvm/vmx/nest.c
>> +/*
>> + * VMX instructions support functions
>> + */
>> +
>> +enum vmx_regs_enc {
>> +    VMX_REG_RAX,
>> +    VMX_REG_RCX,
>> +    VMX_REG_RDX,
>> +    VMX_REG_RBX,
>> +    VMX_REG_RSP,
>> +    VMX_REG_RBP,
>> +    VMX_REG_RSI,
>> +    VMX_REG_RDI,
>> +#ifdef CONFIG_X86_64
>> +    VMX_REG_R8,
>> +    VMX_REG_R9,
>> +    VMX_REG_R10,
>> +    VMX_REG_R11,
>> +    VMX_REG_R12,
>> +    VMX_REG_R13,
>> +    VMX_REG_R14,
>> +    VMX_REG_R15,
>> +#endif
>> +};
>> +
>> +enum vmx_sregs_enc {
>> +    VMX_SREG_ES,
>> +    VMX_SREG_CS,
>> +    VMX_SREG_SS,
>> +    VMX_SREG_DS,
>> +    VMX_SREG_FS,
>> +    VMX_SREG_GS,
>> +};
>> +
>> +enum x86_segment sreg_to_index[] = {
>> +    [VMX_SREG_ES] = x86_seg_es,
>> +    [VMX_SREG_CS] = x86_seg_cs,
>> +    [VMX_SREG_SS] = x86_seg_ss,
>> +    [VMX_SREG_DS] = x86_seg_ds,
>> +    [VMX_SREG_FS] = x86_seg_fs,
>> +    [VMX_SREG_GS] = x86_seg_gs,
>> +};
>
> Since you dislike adding new namespaces and translations, I'm sure you
> can get rid of these. :)  It might even simplify some of the macros
> below.

True, some dupcation here. Regarding following definition in x86_emulate.c, we can reuse.


static enum x86_segment
decode_segment(uint8_t modrm_reg)
{
    switch ( modrm_reg )
    {
    case 0: return x86_seg_es;
    case 1: return x86_seg_cs;
    case 2: return x86_seg_ss;
    case 3: return x86_seg_ds;
    case 4: return x86_seg_fs;
    case 5: return x86_seg_gs;
    default: break;
    }
    return decode_segment_failed;
}

BTW, should we use MACROs rather than digital # here? and can we modify x86_segment structure to use same naming space?

>
>> +union vmx_inst_info {
>> +    struct {
>> +        unsigned int scaling           :2; /* bit 0-1 */
>> +        unsigned int __rsvd0           :1; /* bit 2 */
>> +        unsigned int reg1              :4; /* bit 3-6 */
>> +        unsigned int addr_size         :3; /* bit 7-9 */
>> +        unsigned int memreg            :1; /* bit 10 */
>> +        unsigned int __rsvd1           :4; /* bit 11-14 */
>> +        unsigned int segment           :3; /* bit 15-17 */
>> +        unsigned int index_reg         :4; /* bit 18-21 */
>> +        unsigned int index_reg_invalid :1; /* bit 22 */
>> +        unsigned int base_reg          :4; /* bit 23-26 */
>> +        unsigned int base_reg_invalid  :1; /* bit 27 */
>> +        unsigned int reg2              :4; /* bit 28-31 */ +    }
>> fields; +    u32 word;
>> +};
>> +
>> +struct vmx_inst_decoded {
>> +#define VMX_INST_MEMREG_TYPE_MEMORY 0
>> +#define VMX_INST_MEMREG_TYPE_REG    1
>> +    int type;
>> +    union {
>> +        struct {
>> +            unsigned long mem;
>> +            unsigned int  len;
>> +        };
>> +        enum vmx_regs_enc reg1;
>> +    };
>> +
>> +    enum vmx_regs_enc reg2;
>> +};
>> +
>> +enum vmx_ops_result {
>> +    VMSUCCEED,
>> +    VMFAIL_VALID,
>> +    VMFAIL_INVALID,
>> +};
>> +
>> +#define CASE_SET_REG(REG, reg)      \
>> +    case VMX_REG_ ## REG: regs->reg = value; break
>> +#define CASE_GET_REG(REG, reg)      \
>> +    case VMX_REG_ ## REG: value = regs->reg; break +
>> +#define CASE_EXTEND_SET_REG         \
>> +    CASE_EXTEND_REG(S)
>> +#define CASE_EXTEND_GET_REG         \
>> +    CASE_EXTEND_REG(G)
>> +
>> +#ifdef __i386__
>> +#define CASE_EXTEND_REG(T)
>> +#else
>> +#define CASE_EXTEND_REG(T)          \
>> +    CASE_ ## T ## ET_REG(R8, r8);   \
>> +    CASE_ ## T ## ET_REG(R9, r9);   \
>> +    CASE_ ## T ## ET_REG(R10, r10); \
>> +    CASE_ ## T ## ET_REG(R11, r11); \
>> +    CASE_ ## T ## ET_REG(R12, r12); \
>> +    CASE_ ## T ## ET_REG(R13, r13); \
>> +    CASE_ ## T ## ET_REG(R14, r14); \
>> +    CASE_ ## T ## ET_REG(R15, r15)
>> +#endif
>> +
>> +static unsigned long reg_read(struct cpu_user_regs *regs,
>> +                              enum vmx_regs_enc index) +{
>> +    unsigned long value = 0;
>> +
>> +    switch ( index ) {
>> +    CASE_GET_REG(RAX, eax);
>> +    CASE_GET_REG(RCX, ecx);
>> +    CASE_GET_REG(RDX, edx);
>> +    CASE_GET_REG(RBX, ebx);
>> +    CASE_GET_REG(RBP, ebp);
>> +    CASE_GET_REG(RSI, esi);
>> +    CASE_GET_REG(RDI, edi);
>> +    CASE_GET_REG(RSP, esp);
>> +    CASE_EXTEND_GET_REG;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return value;
>> +}
>> +
>> +static void reg_write(struct cpu_user_regs *regs,
>> +                      enum vmx_regs_enc index,
>> +                      unsigned long value)
>> +{
>> +    switch ( index ) {
>> +    CASE_SET_REG(RAX, eax);
>> +    CASE_SET_REG(RCX, ecx);
>> +    CASE_SET_REG(RDX, edx);
>> +    CASE_SET_REG(RBX, ebx);
>> +    CASE_SET_REG(RBP, ebp);
>> +    CASE_SET_REG(RSI, esi);
>> +    CASE_SET_REG(RDI, edi);
>> +    CASE_SET_REG(RSP, esp);
>> +    CASE_EXTEND_SET_REG;
>> +    default:
>> +        break;
>> +    }
>> +}
>> +
>> +static int decode_vmx_inst(struct cpu_user_regs *regs,
>> +                           struct vmx_inst_decoded *decode) +{
>> +    struct vcpu *v = current;
>> +    union vmx_inst_info info;
>> +    struct segment_register seg;
>> +    unsigned long base, index, seg_base, disp, offset; +    int
>> scale; +
>> +    info.word = __vmread(VMX_INSTRUCTION_INFO);
>> +
>> +    if ( info.fields.memreg ) {
>> +        decode->type = VMX_INST_MEMREG_TYPE_REG;
>> +        decode->reg1 = info.fields.reg1;
>> +    }
>> +    else
>> +    {
>> +        decode->type = VMX_INST_MEMREG_TYPE_MEMORY;
>> +        hvm_get_segment_register(v,
>> sreg_to_index[info.fields.segment], &seg); +        /* TODO: segment
>> type check */
>
> Indeed. :)
>
>> +        seg_base = seg.base;
>> +
>> +        base = info.fields.base_reg_invalid ? 0 :
>> +            reg_read(regs, info.fields.base_reg);
>> +
>> +        index = info.fields.index_reg_invalid ? 0 :
>> +            reg_read(regs, info.fields.index_reg); +
>> +        scale = 1 << info.fields.scaling;
>> +
>> +        disp = __vmread(EXIT_QUALIFICATION);
>> +
>> +        offset = base + index * scale + disp;
>> +        if ( offset > seg.limit )
>
> DYM if ( offset + len > limit )?
>
> Would it be worth trying to fold this into the existing x86_emulate
> code, which already has careful memory access checks?


Can you give more details? Re-construct hvm_virtual_to_linear_addr?


>
>> +            goto gp_fault;
>> +
>> +        decode->mem = seg_base + base + index * scale + disp;
>> +        decode->len = 1 << (info.fields.addr_size + 1); +    }
>> +
>> +    decode->reg2 = info.fields.reg2;
>> +
>> +    return X86EMUL_OKAY;
>> +
>> +gp_fault:
>> +    hvm_inject_exception(TRAP_gp_fault, 0, 0);
>> +    return X86EMUL_EXCEPTION;
>> +}
>> +
>> +static int vmx_inst_check_privilege(struct cpu_user_regs *regs) +{
>> +    struct vcpu *v = current;
>> +    struct segment_register cs;
>> +
>> +    hvm_get_segment_register(v, x86_seg_cs, &cs);
>> +
>> +    if ( !(v->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) ||
>> +         !(v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_VMXE) ||
>> +         (regs->eflags & X86_EFLAGS_VM) ||
>> +         (hvm_long_mode_enabled(v) && cs.attr.fields.l == 0) ) +
>> goto invalid_op; +
>> +    if ( (cs.sel & 3) > 0 )
>> +        goto gp_fault;
>> +
>> +    return X86EMUL_OKAY;
>> +
>> +invalid_op:
>> +    hvm_inject_exception(TRAP_invalid_op, 0, 0);
>> +    return X86EMUL_EXCEPTION;
>> +
>> +gp_fault:
>> +    hvm_inject_exception(TRAP_gp_fault, 0, 0);
>> +    return X86EMUL_EXCEPTION;
>> +}
>> +
>> +static void vmreturn(struct cpu_user_regs *regs, enum
>> vmx_ops_result res) +{ +    unsigned long eflags = regs->eflags;
>> +    unsigned long mask = X86_EFLAGS_CF | X86_EFLAGS_PF |
>> X86_EFLAGS_AF | +                         X86_EFLAGS_ZF |
>> X86_EFLAGS_SF | X86_EFLAGS_OF; + +    eflags &= ~mask;
>> +
>> +    switch ( res ) {
>> +    case VMSUCCEED:
>> +        break;
>> +    case VMFAIL_VALID:
>> +        /* TODO: error number, useful for guest VMM debugging */
>> +        eflags |= X86_EFLAGS_ZF;
>> +        break;
>> +    case VMFAIL_INVALID:
>> +    default:
>> +        eflags |= X86_EFLAGS_CF;
>> +        break;
>> +    }
>> +
>> +    regs->eflags = eflags;
>> +}
>> +
>> +static int __clear_current_vvmcs(struct vmx_nest_struct *nest) +{
>> +    int rc;
>> +
>> +    if ( nest->svmcs )
>> +        __vmpclear(virt_to_maddr(nest->svmcs));
>> +
>> +#if !CONFIG_VVMCS_MAPPING
>> +    rc = hvm_copy_to_guest_phys(nest->gvmcs_pa, nest->vvmcs,
>> PAGE_SIZE); +    if ( rc != HVMCOPY_okay )
>> +        return X86EMUL_EXCEPTION;
>> +#endif
>> +
>> +    nest->vmcs_valid = 0;
>> +
>> +    return X86EMUL_OKAY;
>> +}
>> +
>> +/*
>> + * VMX instructions handling
>> + */
>> +
>> +int vmx_nest_handle_vmxon(struct cpu_user_regs *regs) +{
>> +    struct vcpu *v = current;
>> +    struct vmx_nest_struct *nest = &v->arch.hvm_vmx.nest;
>> +    struct vmx_inst_decoded decode;
>> +    unsigned long gpa = 0;
>> +    int rc;
>> +
>> +    if ( !is_nested_avail(v->domain) )
>> +        goto invalid_op;
>> +
>> +    rc = vmx_inst_check_privilege(regs);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>
> I think you could fold these checks and the goto target into
> decode_vmx_inst(), just to avoid repeating them in every
> vmx_nest_handle_* function.
>
>> +    rc = decode_vmx_inst(regs, &decode);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>> +
>> +    ASSERT(decode.type == VMX_INST_MEMREG_TYPE_MEMORY);
>> +    rc = hvm_copy_from_guest_virt(&gpa, decode.mem, decode.len, 0);
>> +    if ( rc != HVMCOPY_okay )
>> +        return X86EMUL_EXCEPTION;
>> +
>> +    nest->guest_vmxon_pa = gpa;
>> +    nest->gvmcs_pa = 0;
>> +    nest->vmcs_valid = 0;
>> +#if !CONFIG_VVMCS_MAPPING
>> +    nest->vvmcs = alloc_xenheap_page();
>> +    if ( !nest->vvmcs )
>> +    {
>> +        gdprintk(XENLOG_ERR, "nest: allocation for virtual vmcs
>> failed\n"); +        vmreturn(regs, VMFAIL_INVALID);
>> +        goto out;
>> +    }
>> +#endif
>> +    nest->svmcs = alloc_xenheap_page();
>> +    if ( !nest->svmcs )
>> +    {
>> +        gdprintk(XENLOG_ERR, "nest: allocation for shadow vmcs
>> failed\n"); +        free_xenheap_page(nest->vvmcs);
>> +        vmreturn(regs, VMFAIL_INVALID);
>> +        goto out;
>> +    }
>> +
>> +    /*
>> +     * `fork' the host vmcs to shadow_vmcs
>> +     * vmcs_lock is not needed since we are on current +     */
>> +    nest->hvmcs = v->arch.hvm_vmx.vmcs;
>> +    __vmpclear(virt_to_maddr(nest->hvmcs));
>> +    memcpy(nest->svmcs, nest->hvmcs, PAGE_SIZE);
>> +    __vmptrld(virt_to_maddr(nest->hvmcs));
>> +    v->arch.hvm_vmx.launched = 0;
>> +
>> +    vmreturn(regs, VMSUCCEED);
>> +
>> +out:
>> +    return X86EMUL_OKAY;
>> +
>> +invalid_op:
>> +    hvm_inject_exception(TRAP_invalid_op, 0, 0);
>> +    return X86EMUL_EXCEPTION;
>> +}
>> +
>> +int vmx_nest_handle_vmxoff(struct cpu_user_regs *regs) +{
>> +    struct vcpu *v = current;
>> +    struct vmx_nest_struct *nest = &v->arch.hvm_vmx.nest; +    int
>> rc; +
>> +    if ( unlikely(!nest->guest_vmxon_pa) )
>> +        goto invalid_op;
>> +
>> +    rc = vmx_inst_check_privilege(regs);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>> +
>> +    nest->guest_vmxon_pa = 0;
>> +    __vmpclear(virt_to_maddr(nest->svmcs));
>> +
>> +#if !CONFIG_VVMCS_MAPPING
>> +    free_xenheap_page(nest->vvmcs);
>> +#endif
>> +    free_xenheap_page(nest->svmcs);
>
> These also need to be freed on domain teardown.
>
>> +    vmreturn(regs, VMSUCCEED);
>> +    return X86EMUL_OKAY;
>> +
>> +invalid_op:
>> +    hvm_inject_exception(TRAP_invalid_op, 0, 0);
>> +    return X86EMUL_EXCEPTION;
>> +}
>> +
>> +int vmx_nest_handle_vmptrld(struct cpu_user_regs *regs) +{
>> +    struct vcpu *v = current;
>> +    struct vmx_inst_decoded decode;
>> +    struct vmx_nest_struct *nest = &v->arch.hvm_vmx.nest; +
>> unsigned long gpa = 0; +    int rc;
>> +
>> +    if ( unlikely(!nest->guest_vmxon_pa) )
>> +        goto invalid_op;
>> +
>> +    rc = vmx_inst_check_privilege(regs);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>> +
>> +    rc = decode_vmx_inst(regs, &decode);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>> +
>> +    ASSERT(decode.type == VMX_INST_MEMREG_TYPE_MEMORY);
>> +    rc = hvm_copy_from_guest_virt(&gpa, decode.mem, decode.len, 0);
>> +    if ( rc != HVMCOPY_okay )
>> +        return X86EMUL_EXCEPTION;
>> +
>> +    if ( gpa == nest->guest_vmxon_pa || gpa & 0xfff ) +    {
>> +        vmreturn(regs, VMFAIL_INVALID);
>> +        goto out;
>> +    }
>> +
>> +    if ( nest->gvmcs_pa != gpa )
>> +    {
>> +        if ( nest->vmcs_valid )
>> +        {
>> +            rc = __clear_current_vvmcs(nest);
>> +            if ( rc != X86EMUL_OKAY )
>> +                return rc;
>> +        }
>> +        nest->gvmcs_pa = gpa;
>> +        ASSERT(nest->vmcs_valid == 0);
>> +    }
>> +
>> +
>> +    if ( !nest->vmcs_valid )
>> +    {
>> +#if CONFIG_VVMCS_MAPPING
>> +        unsigned long mfn;
>> +        p2m_type_t p2mt;
>> +
>> +        mfn = mfn_x(gfn_to_mfn(p2m_get_hostp2m(v->domain),
>> +                               nest->gvmcs_pa >> PAGE_SHIFT,
>> &p2mt)); +        nest->vvmcs = map_domain_page_global(mfn); +#else
>> +        rc = hvm_copy_from_guest_phys(nest->vvmcs, nest->gvmcs_pa,
>> PAGE_SIZE); +        if ( rc != HVMCOPY_okay )
>> +            return X86EMUL_EXCEPTION;
>> +#endif
>> +        nest->vmcs_valid = 1;
>> +    }
>> +
>> +    vmreturn(regs, VMSUCCEED);
>> +
>> +out:
>> +    return X86EMUL_OKAY;
>> +
>> +invalid_op:
>> +    hvm_inject_exception(TRAP_invalid_op, 0, 0);
>> +    return X86EMUL_EXCEPTION;
>> +}
>> +
>> +int vmx_nest_handle_vmptrst(struct cpu_user_regs *regs) +{
>> +    struct vcpu *v = current;
>> +    struct vmx_inst_decoded decode;
>> +    struct vmx_nest_struct *nest = &v->arch.hvm_vmx.nest; +
>> unsigned long gpa = 0; +    int rc;
>> +
>> +    if ( unlikely(!nest->guest_vmxon_pa) )
>> +        goto invalid_op;
>> +
>> +    rc = vmx_inst_check_privilege(regs);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>> +
>> +    rc = decode_vmx_inst(regs, &decode);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>> +
>> +    ASSERT(decode.type == VMX_INST_MEMREG_TYPE_MEMORY); +
>> +    gpa = nest->gvmcs_pa;
>> +
>> +    rc = hvm_copy_to_guest_virt(decode.mem, &gpa, decode.len, 0);
>> +    if ( rc != HVMCOPY_okay )
>> +        return X86EMUL_EXCEPTION;
>> +
>> +    vmreturn(regs, VMSUCCEED);
>> +    return X86EMUL_OKAY;
>> +
>> +invalid_op:
>> +    hvm_inject_exception(TRAP_invalid_op, 0, 0);
>> +    return X86EMUL_EXCEPTION;
>> +}
>> +
>> +int vmx_nest_handle_vmclear(struct cpu_user_regs *regs) +{
>> +    struct vcpu *v = current;
>> +    struct vmx_inst_decoded decode;
>> +    struct vmx_nest_struct *nest = &v->arch.hvm_vmx.nest; +
>> unsigned long gpa = 0; +    int rc;
>> +
>> +    if ( unlikely(!nest->guest_vmxon_pa) )
>> +        goto invalid_op;
>> +
>> +    rc = vmx_inst_check_privilege(regs);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>> +
>> +    rc = decode_vmx_inst(regs, &decode);
>> +    if ( rc != X86EMUL_OKAY )
>> +        return rc;
>> +
>> +    ASSERT(decode.type == VMX_INST_MEMREG_TYPE_MEMORY);
>> +    rc = hvm_copy_from_guest_virt(&gpa, decode.mem, decode.len, 0);
>
> Is it guaranteed that decode.len is always <= sizeof gpa here, even
> with a malicious guest?

Reusing hvm_virtual_to_linear_addr w/ consideration of last byte may be the best choice :)

Thx, Eddie