[PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 17584 bytes --]

Most invocations of the instruction emulator are for VM exits where the
set of legitimate instructions (i.e. ones capable of causing the
respective exit) is rather small. Restrict the permitted sets via a new
callback, at once eliminating the abuse of handle_mmio() for non-MMIO
operations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Better way to cover FPU/SIMD insns in x86_insn_is_mem_write()?

Note that hvm_emulate_is_mem_*() (for now) intentionally don't include
implicit memory operands: I don't think we mean to support namely
the stack to live in MMIO, but otoh we may need to permit that.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
     return hvmemul_write(seg, offset, p_new, bytes, ctxt);
 }
 
+static int hvmemul_validate(
+    const struct x86_emulate_state *state,
+    struct x86_emulate_ctxt *ctxt)
+{
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+
+    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
+                                  : X86EMUL_OKAY;
+}
+
 static int hvmemul_rep_ins(
     uint16_t src_port,
     enum x86_segment dst_seg,
@@ -1663,6 +1674,7 @@ static const struct x86_emulate_ops hvm_
     .insn_fetch    = hvmemul_insn_fetch,
     .write         = hvmemul_write,
     .cmpxchg       = hvmemul_cmpxchg,
+    .validate      = hvmemul_validate,
     .rep_ins       = hvmemul_rep_ins,
     .rep_outs      = hvmemul_rep_outs,
     .rep_movs      = hvmemul_rep_movs,
@@ -1808,7 +1820,8 @@ int hvm_emulate_one_mmio(unsigned long m
     else
         ops = &hvm_ro_emulate_ops_mmio;
 
-    hvm_emulate_init_once(&ctxt, guest_cpu_user_regs());
+    hvm_emulate_init_once(&ctxt, hvm_emulate_is_mem_write,
+                          guest_cpu_user_regs());
     ctxt.ctxt.data = &mmio_ro_ctxt;
     rc = _hvm_emulate_one(&ctxt, ops);
     switch ( rc )
@@ -1833,7 +1846,7 @@ void hvm_emulate_one_vm_event(enum emul_
     struct hvm_emulate_ctxt ctx = {{ 0 }};
     int rc;
 
-    hvm_emulate_init_once(&ctx, guest_cpu_user_regs());
+    hvm_emulate_init_once(&ctx, NULL, guest_cpu_user_regs());
 
     switch ( kind )
     {
@@ -1887,6 +1900,7 @@ void hvm_emulate_one_vm_event(enum emul_
 
 void hvm_emulate_init_once(
     struct hvm_emulate_ctxt *hvmemul_ctxt,
+    hvm_emulate_validate_t *validate,
     struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
@@ -1897,6 +1911,7 @@ void hvm_emulate_init_once(
     hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
     hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
 
+    hvmemul_ctxt->validate = validate;
     hvmemul_ctxt->ctxt.regs = regs;
     hvmemul_ctxt->ctxt.vendor = curr->domain->arch.x86_vendor;
     hvmemul_ctxt->ctxt.force_writeback = true;
@@ -1991,6 +2006,56 @@ struct segment_register *hvmemul_get_seg
     return &hvmemul_ctxt->seg_reg[idx];
 }
 
+int hvm_emulate_is_mem_access(const struct x86_emulate_state *state,
+                              struct hvm_emulate_ctxt *ctxt)
+{
+    return x86_insn_is_mem_access(state, &ctxt->ctxt) ? X86EMUL_OKAY
+                                                      : X86EMUL_UNHANDLEABLE;
+}
+
+int hvm_emulate_is_mem_write(const struct x86_emulate_state *state,
+                             struct hvm_emulate_ctxt *ctxt)
+{
+    return x86_insn_is_mem_write(state, &ctxt->ctxt) ? X86EMUL_OKAY
+                                                     : X86EMUL_UNHANDLEABLE;
+}
+
+int hvm_emulate_is_portio(const struct x86_emulate_state *state,
+                          struct hvm_emulate_ctxt *ctxt)
+{
+    switch ( ctxt->ctxt.opcode )
+    {
+    case 0x6c ... 0x6f: /* INS / OUTS */
+    case 0xe4 ... 0xe7: /* IN / OUT imm8 */
+    case 0xec ... 0xef: /* IN / OUT %dx */
+        return X86EMUL_OKAY;
+    }
+
+    return X86EMUL_UNHANDLEABLE;
+}
+
+int hvm_emulate_is_cr_access(const struct x86_emulate_state *state,
+                             struct hvm_emulate_ctxt *ctxt)
+{
+    switch ( ctxt->ctxt.opcode )
+    {
+        unsigned int ext;
+
+    case X86EMUL_OPC(0x0f, 0x01):
+        x86_insn_modrm(state, NULL, &ext);
+        if ( (ext & 5) == 4 ) /* SMSW / LMSW */
+            return X86EMUL_OKAY;
+        break;
+
+    case X86EMUL_OPC(0x0f, 0x06): /* CLTS */
+    case X86EMUL_OPC(0x0f, 0x20): /* MOV from CRn */
+    case X86EMUL_OPC(0x0f, 0x22): /* MOV to CRn */
+        return X86EMUL_OKAY;
+    }
+
+    return X86EMUL_UNHANDLEABLE;
+}
+
 static const char *guest_x86_mode_to_str(int mode)
 {
     switch ( mode )
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4004,7 +4004,7 @@ void hvm_ud_intercept(struct cpu_user_re
         cur->domain->arch.x86_vendor != boot_cpu_data.x86_vendor;
     struct hvm_emulate_ctxt ctxt;
 
-    hvm_emulate_init_once(&ctxt, regs);
+    hvm_emulate_init_once(&ctxt, NULL, regs);
 
     if ( opt_hvm_fep )
     {
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -78,7 +78,7 @@ void send_invalidate_req(void)
         gprintk(XENLOG_ERR, "Unsuccessful map-cache invalidate\n");
 }
 
-bool handle_mmio(void)
+bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate)
 {
     struct hvm_emulate_ctxt ctxt;
     struct vcpu *curr = current;
@@ -87,7 +87,7 @@ bool handle_mmio(void)
 
     ASSERT(!is_pvh_vcpu(curr));
 
-    hvm_emulate_init_once(&ctxt, guest_cpu_user_regs());
+    hvm_emulate_init_once(&ctxt, validate, guest_cpu_user_regs());
 
     rc = hvm_emulate_one(&ctxt);
 
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -168,7 +168,7 @@ bool_t handle_hvm_io_completion(struct v
     {
         struct hvm_emulate_ctxt ctxt;
 
-        hvm_emulate_init_once(&ctxt, guest_cpu_user_regs());
+        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
         vmx_realmode_emulate_one(&ctxt);
         hvm_emulate_writeback(&ctxt);
 
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -107,7 +107,7 @@ int __get_instruction_length_from_list(s
 #endif
 
     ASSERT(v == current);
-    hvm_emulate_init_once(&ctxt, guest_cpu_user_regs());
+    hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
     hvm_emulate_init_per_insn(&ctxt, NULL, 0);
     state = x86_decode_insn(&ctxt.ctxt, hvmemul_insn_fetch);
     if ( IS_ERR_OR_NULL(state) )
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2175,6 +2175,16 @@ static void svm_invlpg_intercept(unsigne
     paging_invlpg(current, vaddr);
 }
 
+static int is_invlpg(const struct x86_emulate_state *state,
+                     struct hvm_emulate_ctxt *ctxt)
+{
+    unsigned int ext;
+
+    return ctxt->ctxt.opcode == X86EMUL_OPC(0x0f, 0x01) &&
+           x86_insn_modrm(state, NULL, &ext) != 3 &&
+           (ext & 7) == 7 ? X86EMUL_OKAY : X86EMUL_UNHANDLEABLE;
+}
+
 static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
 {
     svm_asid_g_invlpg(v, vaddr);
@@ -2520,7 +2530,7 @@ void svm_vmexit_handler(struct cpu_user_
             if ( handle_pio(port, bytes, dir) )
                 __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
         }
-        else if ( !handle_mmio() )
+        else if ( !hvm_emulate_one_insn(hvm_emulate_is_portio) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;
 
@@ -2528,7 +2538,7 @@ void svm_vmexit_handler(struct cpu_user_
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
         if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
             svm_vmexit_do_cr_access(vmcb, regs);
-        else if ( !handle_mmio() ) 
+        else if ( !hvm_emulate_one_insn(hvm_emulate_is_cr_access) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;
 
@@ -2538,7 +2548,7 @@ void svm_vmexit_handler(struct cpu_user_
             svm_invlpg_intercept(vmcb->exitinfo1);
             __update_guest_eip(regs, vmcb->nextrip - vmcb->rip);
         }
-        else if ( !handle_mmio() )
+        else if ( !hvm_emulate_one_insn(is_invlpg) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;
 
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -179,7 +179,7 @@ void vmx_realmode(struct cpu_user_regs *
     if ( intr_info & INTR_INFO_VALID_MASK )
         __vmwrite(VM_ENTRY_INTR_INFO, 0);
 
-    hvm_emulate_init_once(&hvmemul_ctxt, regs);
+    hvm_emulate_init_once(&hvmemul_ctxt, NULL, regs);
 
     /* Only deliver interrupts into emulated real mode. */
     if ( !(curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) &&
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3733,7 +3733,7 @@ void vmx_vmexit_handler(struct cpu_user_
         {
             /* INS, OUTS */
             if ( unlikely(is_pvh_vcpu(v)) /* PVH fixme */ ||
-                 !handle_mmio() )
+                 !hvm_emulate_one_insn(hvm_emulate_is_portio) )
                 hvm_inject_hw_exception(TRAP_gp_fault, 0);
         }
         else
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3774,7 +3774,7 @@ x86_emulate(
                 emulate_fpu_insn_memsrc("flds", src.val);
                 dst.type = OP_NONE;
                 break;
-            case 2: /* fstp m32fp */
+            case 2: /* fst m32fp */
                 emulate_fpu_insn_memdst("fsts", dst.val);
                 dst.bytes = 4;
                 break;
@@ -6105,6 +6105,148 @@ x86_insn_operand_ea(const struct x86_emu
     return state->ea.mem.off;
 }
 
+bool
+x86_insn_is_mem_access(const struct x86_emulate_state *state,
+                       const struct x86_emulate_ctxt *ctxt)
+{
+    if ( state->ea.type == OP_MEM )
+        return ctxt->opcode != 0x8d /* LEA */ &&
+               (ctxt->opcode != X86EMUL_OPC(0x0f, 0x01) ||
+                (state->modrm_reg & 7) != 7) /* INVLPG */;
+
+    switch ( ctxt->opcode )
+    {
+    case 0x6c ... 0x6f: /* INS / OUTS */
+    case 0xa4 ... 0xa7: /* MOVS / CMPS */
+    case 0xaa ... 0xaf: /* STOS / LODS / SCAS */
+    case 0xd7:          /* XLAT */
+        return true;
+
+    case X86EMUL_OPC(0x0f, 0x01):
+        /* Cover CLZERO. */
+        return (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
+    }
+
+    return false;
+}
+
+bool
+x86_insn_is_mem_write(const struct x86_emulate_state *state,
+                      const struct x86_emulate_ctxt *ctxt)
+{
+    switch ( state->desc & DstMask )
+    {
+    case DstMem:
+        return state->modrm_mod != 3;
+
+    case DstBitBase:
+    case DstImplicit:
+        break;
+
+    default:
+        return false;
+    }
+
+    if ( state->modrm_mod == 3 )
+        /* CLZERO is the odd one. */
+        return ctxt->opcode == X86EMUL_OPC(0x0f, 0x01) &&
+               (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
+
+    switch ( ctxt->opcode )
+    {
+    case 0x6c: case 0x6d:                /* INS */
+    case 0xa4: case 0xa5:                /* MOVS */
+    case 0xaa: case 0xab:                /* STOS */
+    case X86EMUL_OPC(0x0f, 0x11):        /* MOVUPS */
+    case X86EMUL_OPC_VEX(0x0f, 0x11):    /* VMOVUPS */
+    case X86EMUL_OPC_66(0x0f, 0x11):     /* MOVUPD */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x11): /* VMOVUPD */
+    case X86EMUL_OPC_F3(0x0f, 0x11):     /* MOVSS */
+    case X86EMUL_OPC_VEX_F3(0x0f, 0x11): /* VMOVSS */
+    case X86EMUL_OPC_F2(0x0f, 0x11):     /* MOVSD */
+    case X86EMUL_OPC_VEX_F2(0x0f, 0x11): /* VMOVSD */
+    case X86EMUL_OPC(0x0f, 0x29):        /* MOVAPS */
+    case X86EMUL_OPC_VEX(0x0f, 0x29):    /* VMOVAPS */
+    case X86EMUL_OPC_66(0x0f, 0x29):     /* MOVAPD */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x29): /* VMOVAPD */
+    case X86EMUL_OPC(0x0f, 0x2b):        /* MOVNTPS */
+    case X86EMUL_OPC_VEX(0x0f, 0x2b):    /* VMOVNTPS */
+    case X86EMUL_OPC_66(0x0f, 0x2b):     /* MOVNTPD */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x2b): /* VMOVNTPD */
+    case X86EMUL_OPC(0x0f, 0x7e):        /* MOVD/MOVQ */
+    case X86EMUL_OPC_66(0x0f, 0x7e):     /* MOVD/MOVQ */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x7e): /* VMOVD/VMOVQ */
+    case X86EMUL_OPC(0x0f, 0x7f):        /* VMOVQ */
+    case X86EMUL_OPC_66(0x0f, 0x7f):     /* MOVDQA */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x7f): /* VMOVDQA */
+    case X86EMUL_OPC_F3(0x0f, 0x7f):     /* MOVDQU */
+    case X86EMUL_OPC_VEX_F3(0x0f, 0x7f): /* VMOVDQU */
+    case X86EMUL_OPC(0x0f, 0xab):        /* BTS */
+    case X86EMUL_OPC(0x0f, 0xb3):        /* BTR */
+    case X86EMUL_OPC(0x0f, 0xbb):        /* BTC */
+    case X86EMUL_OPC_66(0x0f, 0xd6):     /* MOVQ */
+    case X86EMUL_OPC_VEX_66(0x0f, 0xd6): /* VMOVQ */
+    case X86EMUL_OPC(0x0f, 0xe7):        /* MOVNTQ */
+    case X86EMUL_OPC_66(0x0f, 0xe7):     /* MOVNTDQ */
+    case X86EMUL_OPC_VEX_66(0x0f, 0xe7): /* VMOVNTDQ */
+        return true;
+
+    case 0xd9:
+        switch ( state->modrm_reg & 7 )
+        {
+        case 2: /* FST m32fp */
+        case 3: /* FSTP m32fp */
+        case 6: /* FNSTENV */
+        case 7: /* FNSTCW */
+            return true;
+        }
+        break;
+
+    case 0xdb:
+        switch ( state->modrm_reg & 7 )
+        {
+        case 1: /* FISTTP m32i */
+        case 2: /* FIST m32i */
+        case 3: /* FISTP m32i */
+        case 7: /* FSTP m80fp */
+            return true;
+        }
+        break;
+
+    case 0xdd:
+        switch ( state->modrm_reg & 7 )
+        {
+        case 1: /* FISTTP m64i */
+        case 2: /* FST m64fp */
+        case 3: /* FSTP m64fp */
+        case 6: /* FNSAVE */
+        case 7: /* FNSTSW */
+            return true;
+        }
+        break;
+
+    case 0xdf:
+        switch ( state->modrm_reg & 7 )
+        {
+        case 1: /* FISTTP m16i */
+        case 2: /* FIST m16i */
+        case 3: /* FISTP m16i */
+        case 6: /* FBSTP */
+        case 7: /* FISTP m64i */
+            return true;
+        }
+        break;
+
+    case X86EMUL_OPC(0x0f, 0x01):
+        return !(state->modrm_reg & 6); /* SGDT / SIDT */
+
+    case X86EMUL_OPC(0x0f, 0xba):
+        return (state->modrm_reg & 7) > 4; /* BTS / BTR / BTC */
+    }
+
+    return false;
+}
+
 unsigned long
 x86_insn_immediate(const struct x86_emulate_state *state, unsigned int nr)
 {
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -631,6 +631,12 @@ x86_insn_immediate(const struct x86_emul
 unsigned int
 x86_insn_length(const struct x86_emulate_state *state,
                 const struct x86_emulate_ctxt *ctxt);
+bool
+x86_insn_is_mem_access(const struct x86_emulate_state *state,
+                       const struct x86_emulate_ctxt *ctxt);
+bool
+x86_insn_is_mem_write(const struct x86_emulate_state *state,
+                      const struct x86_emulate_ctxt *ctxt);
 
 #ifdef NDEBUG
 static inline void x86_emulate_free_state(struct x86_emulate_state *state) {}
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -17,9 +17,19 @@
 #include <asm/hvm/hvm.h>
 #include <asm/x86_emulate.h>
 
+struct hvm_emulate_ctxt;
+typedef int hvm_emulate_validate_t(const struct x86_emulate_state *state,
+                                   struct hvm_emulate_ctxt *ctxt);
+
 struct hvm_emulate_ctxt {
     struct x86_emulate_ctxt ctxt;
 
+    /*
+     * validate: Post-decode, pre-emulate hook to allow caller controlled
+     * filtering.
+     */
+    hvm_emulate_validate_t *validate;
+
     /* Cache of 16 bytes of instruction. */
     uint8_t insn_buf[16];
     unsigned long insn_buf_eip;
@@ -41,6 +51,8 @@ enum emul_kind {
     EMUL_KIND_SET_CONTEXT_INSN
 };
 
+bool __nonnull(1) hvm_emulate_one_insn(
+    hvm_emulate_validate_t *validate);
 int hvm_emulate_one(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
 void hvm_emulate_one_vm_event(enum emul_kind kind,
@@ -49,6 +61,7 @@ void hvm_emulate_one_vm_event(enum emul_
 /* Must be called once to set up hvmemul state. */
 void hvm_emulate_init_once(
     struct hvm_emulate_ctxt *hvmemul_ctxt,
+    hvm_emulate_validate_t *validate,
     struct cpu_user_regs *regs);
 /* Must be called once before each instruction emulated. */
 void hvm_emulate_init_per_insn(
@@ -68,6 +81,16 @@ struct segment_register *hvmemul_get_seg
     struct hvm_emulate_ctxt *hvmemul_ctxt);
 int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla);
 
+hvm_emulate_validate_t hvm_emulate_is_mem_access,
+                       hvm_emulate_is_mem_write,
+                       hvm_emulate_is_portio,
+                       hvm_emulate_is_cr_access;
+
+static inline bool handle_mmio(void)
+{
+    return hvm_emulate_one_insn(hvm_emulate_is_mem_access);
+}
+
 int hvmemul_insn_fetch(enum x86_segment seg,
                        unsigned long offset,
                        void *p_data,
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -118,7 +118,6 @@ void relocate_portio_handler(
 
 void send_timeoffset_req(unsigned long timeoff);
 void send_invalidate_req(void);
-bool handle_mmio(void);
 bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
                                   struct npfec);
 bool handle_pio(uint16_t port, unsigned int size, int dir);



[-- Attachment #2: x86-HVM-filter-insn.patch --]
[-- Type: text/plain, Size: 17657 bytes --]

x86/HVM: restrict permitted instructions during special purpose emulation

Most invocations of the instruction emulator are for VM exits where the
set of legitimate instructions (i.e. ones capable of causing the
respective exit) is rather small. Restrict the permitted sets via a new
callback, at once eliminating the abuse of handle_mmio() for non-MMIO
operations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Better way to cover FPU/SIMD insns in x86_insn_is_mem_write()?

Note that hvm_emulate_is_mem_*() (for now) intentionally don't include
implicit memory operands: I don't think we mean to support namely
the stack to live in MMIO, but otoh we may need to permit that.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
     return hvmemul_write(seg, offset, p_new, bytes, ctxt);
 }
 
+static int hvmemul_validate(
+    const struct x86_emulate_state *state,
+    struct x86_emulate_ctxt *ctxt)
+{
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+
+    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
+                                  : X86EMUL_OKAY;
+}
+
 static int hvmemul_rep_ins(
     uint16_t src_port,
     enum x86_segment dst_seg,
@@ -1663,6 +1674,7 @@ static const struct x86_emulate_ops hvm_
     .insn_fetch    = hvmemul_insn_fetch,
     .write         = hvmemul_write,
     .cmpxchg       = hvmemul_cmpxchg,
+    .validate      = hvmemul_validate,
     .rep_ins       = hvmemul_rep_ins,
     .rep_outs      = hvmemul_rep_outs,
     .rep_movs      = hvmemul_rep_movs,
@@ -1808,7 +1820,8 @@ int hvm_emulate_one_mmio(unsigned long m
     else
         ops = &hvm_ro_emulate_ops_mmio;
 
-    hvm_emulate_init_once(&ctxt, guest_cpu_user_regs());
+    hvm_emulate_init_once(&ctxt, hvm_emulate_is_mem_write,
+                          guest_cpu_user_regs());
     ctxt.ctxt.data = &mmio_ro_ctxt;
     rc = _hvm_emulate_one(&ctxt, ops);
     switch ( rc )
@@ -1833,7 +1846,7 @@ void hvm_emulate_one_vm_event(enum emul_
     struct hvm_emulate_ctxt ctx = {{ 0 }};
     int rc;
 
-    hvm_emulate_init_once(&ctx, guest_cpu_user_regs());
+    hvm_emulate_init_once(&ctx, NULL, guest_cpu_user_regs());
 
     switch ( kind )
     {
@@ -1887,6 +1900,7 @@ void hvm_emulate_one_vm_event(enum emul_
 
 void hvm_emulate_init_once(
     struct hvm_emulate_ctxt *hvmemul_ctxt,
+    hvm_emulate_validate_t *validate,
     struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
@@ -1897,6 +1911,7 @@ void hvm_emulate_init_once(
     hvmemul_get_seg_reg(x86_seg_cs, hvmemul_ctxt);
     hvmemul_get_seg_reg(x86_seg_ss, hvmemul_ctxt);
 
+    hvmemul_ctxt->validate = validate;
     hvmemul_ctxt->ctxt.regs = regs;
     hvmemul_ctxt->ctxt.vendor = curr->domain->arch.x86_vendor;
     hvmemul_ctxt->ctxt.force_writeback = true;
@@ -1991,6 +2006,56 @@ struct segment_register *hvmemul_get_seg
     return &hvmemul_ctxt->seg_reg[idx];
 }
 
+int hvm_emulate_is_mem_access(const struct x86_emulate_state *state,
+                              struct hvm_emulate_ctxt *ctxt)
+{
+    return x86_insn_is_mem_access(state, &ctxt->ctxt) ? X86EMUL_OKAY
+                                                      : X86EMUL_UNHANDLEABLE;
+}
+
+int hvm_emulate_is_mem_write(const struct x86_emulate_state *state,
+                             struct hvm_emulate_ctxt *ctxt)
+{
+    return x86_insn_is_mem_write(state, &ctxt->ctxt) ? X86EMUL_OKAY
+                                                     : X86EMUL_UNHANDLEABLE;
+}
+
+int hvm_emulate_is_portio(const struct x86_emulate_state *state,
+                          struct hvm_emulate_ctxt *ctxt)
+{
+    switch ( ctxt->ctxt.opcode )
+    {
+    case 0x6c ... 0x6f: /* INS / OUTS */
+    case 0xe4 ... 0xe7: /* IN / OUT imm8 */
+    case 0xec ... 0xef: /* IN / OUT %dx */
+        return X86EMUL_OKAY;
+    }
+
+    return X86EMUL_UNHANDLEABLE;
+}
+
+int hvm_emulate_is_cr_access(const struct x86_emulate_state *state,
+                             struct hvm_emulate_ctxt *ctxt)
+{
+    switch ( ctxt->ctxt.opcode )
+    {
+        unsigned int ext;
+
+    case X86EMUL_OPC(0x0f, 0x01):
+        x86_insn_modrm(state, NULL, &ext);
+        if ( (ext & 5) == 4 ) /* SMSW / LMSW */
+            return X86EMUL_OKAY;
+        break;
+
+    case X86EMUL_OPC(0x0f, 0x06): /* CLTS */
+    case X86EMUL_OPC(0x0f, 0x20): /* MOV from CRn */
+    case X86EMUL_OPC(0x0f, 0x22): /* MOV to CRn */
+        return X86EMUL_OKAY;
+    }
+
+    return X86EMUL_UNHANDLEABLE;
+}
+
 static const char *guest_x86_mode_to_str(int mode)
 {
     switch ( mode )
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4004,7 +4004,7 @@ void hvm_ud_intercept(struct cpu_user_re
         cur->domain->arch.x86_vendor != boot_cpu_data.x86_vendor;
     struct hvm_emulate_ctxt ctxt;
 
-    hvm_emulate_init_once(&ctxt, regs);
+    hvm_emulate_init_once(&ctxt, NULL, regs);
 
     if ( opt_hvm_fep )
     {
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -78,7 +78,7 @@ void send_invalidate_req(void)
         gprintk(XENLOG_ERR, "Unsuccessful map-cache invalidate\n");
 }
 
-bool handle_mmio(void)
+bool hvm_emulate_one_insn(hvm_emulate_validate_t *validate)
 {
     struct hvm_emulate_ctxt ctxt;
     struct vcpu *curr = current;
@@ -87,7 +87,7 @@ bool handle_mmio(void)
 
     ASSERT(!is_pvh_vcpu(curr));
 
-    hvm_emulate_init_once(&ctxt, guest_cpu_user_regs());
+    hvm_emulate_init_once(&ctxt, validate, guest_cpu_user_regs());
 
     rc = hvm_emulate_one(&ctxt);
 
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -168,7 +168,7 @@ bool_t handle_hvm_io_completion(struct v
     {
         struct hvm_emulate_ctxt ctxt;
 
-        hvm_emulate_init_once(&ctxt, guest_cpu_user_regs());
+        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
         vmx_realmode_emulate_one(&ctxt);
         hvm_emulate_writeback(&ctxt);
 
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -107,7 +107,7 @@ int __get_instruction_length_from_list(s
 #endif
 
     ASSERT(v == current);
-    hvm_emulate_init_once(&ctxt, guest_cpu_user_regs());
+    hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
     hvm_emulate_init_per_insn(&ctxt, NULL, 0);
     state = x86_decode_insn(&ctxt.ctxt, hvmemul_insn_fetch);
     if ( IS_ERR_OR_NULL(state) )
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2175,6 +2175,16 @@ static void svm_invlpg_intercept(unsigne
     paging_invlpg(current, vaddr);
 }
 
+static int is_invlpg(const struct x86_emulate_state *state,
+                     struct hvm_emulate_ctxt *ctxt)
+{
+    unsigned int ext;
+
+    return ctxt->ctxt.opcode == X86EMUL_OPC(0x0f, 0x01) &&
+           x86_insn_modrm(state, NULL, &ext) != 3 &&
+           (ext & 7) == 7 ? X86EMUL_OKAY : X86EMUL_UNHANDLEABLE;
+}
+
 static void svm_invlpg(struct vcpu *v, unsigned long vaddr)
 {
     svm_asid_g_invlpg(v, vaddr);
@@ -2520,7 +2530,7 @@ void svm_vmexit_handler(struct cpu_user_
             if ( handle_pio(port, bytes, dir) )
                 __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
         }
-        else if ( !handle_mmio() )
+        else if ( !hvm_emulate_one_insn(hvm_emulate_is_portio) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;
 
@@ -2528,7 +2538,7 @@ void svm_vmexit_handler(struct cpu_user_
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
         if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
             svm_vmexit_do_cr_access(vmcb, regs);
-        else if ( !handle_mmio() ) 
+        else if ( !hvm_emulate_one_insn(hvm_emulate_is_cr_access) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;
 
@@ -2538,7 +2548,7 @@ void svm_vmexit_handler(struct cpu_user_
             svm_invlpg_intercept(vmcb->exitinfo1);
             __update_guest_eip(regs, vmcb->nextrip - vmcb->rip);
         }
-        else if ( !handle_mmio() )
+        else if ( !hvm_emulate_one_insn(is_invlpg) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         break;
 
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -179,7 +179,7 @@ void vmx_realmode(struct cpu_user_regs *
     if ( intr_info & INTR_INFO_VALID_MASK )
         __vmwrite(VM_ENTRY_INTR_INFO, 0);
 
-    hvm_emulate_init_once(&hvmemul_ctxt, regs);
+    hvm_emulate_init_once(&hvmemul_ctxt, NULL, regs);
 
     /* Only deliver interrupts into emulated real mode. */
     if ( !(curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) &&
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3733,7 +3733,7 @@ void vmx_vmexit_handler(struct cpu_user_
         {
             /* INS, OUTS */
             if ( unlikely(is_pvh_vcpu(v)) /* PVH fixme */ ||
-                 !handle_mmio() )
+                 !hvm_emulate_one_insn(hvm_emulate_is_portio) )
                 hvm_inject_hw_exception(TRAP_gp_fault, 0);
         }
         else
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3774,7 +3774,7 @@ x86_emulate(
                 emulate_fpu_insn_memsrc("flds", src.val);
                 dst.type = OP_NONE;
                 break;
-            case 2: /* fstp m32fp */
+            case 2: /* fst m32fp */
                 emulate_fpu_insn_memdst("fsts", dst.val);
                 dst.bytes = 4;
                 break;
@@ -6105,6 +6105,148 @@ x86_insn_operand_ea(const struct x86_emu
     return state->ea.mem.off;
 }
 
+bool
+x86_insn_is_mem_access(const struct x86_emulate_state *state,
+                       const struct x86_emulate_ctxt *ctxt)
+{
+    if ( state->ea.type == OP_MEM )
+        return ctxt->opcode != 0x8d /* LEA */ &&
+               (ctxt->opcode != X86EMUL_OPC(0x0f, 0x01) ||
+                (state->modrm_reg & 7) != 7) /* INVLPG */;
+
+    switch ( ctxt->opcode )
+    {
+    case 0x6c ... 0x6f: /* INS / OUTS */
+    case 0xa4 ... 0xa7: /* MOVS / CMPS */
+    case 0xaa ... 0xaf: /* STOS / LODS / SCAS */
+    case 0xd7:          /* XLAT */
+        return true;
+
+    case X86EMUL_OPC(0x0f, 0x01):
+        /* Cover CLZERO. */
+        return (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
+    }
+
+    return false;
+}
+
+bool
+x86_insn_is_mem_write(const struct x86_emulate_state *state,
+                      const struct x86_emulate_ctxt *ctxt)
+{
+    switch ( state->desc & DstMask )
+    {
+    case DstMem:
+        return state->modrm_mod != 3;
+
+    case DstBitBase:
+    case DstImplicit:
+        break;
+
+    default:
+        return false;
+    }
+
+    if ( state->modrm_mod == 3 )
+        /* CLZERO is the odd one. */
+        return ctxt->opcode == X86EMUL_OPC(0x0f, 0x01) &&
+               (state->modrm_rm & 7) == 4 && (state->modrm_reg & 7) == 7;
+
+    switch ( ctxt->opcode )
+    {
+    case 0x6c: case 0x6d:                /* INS */
+    case 0xa4: case 0xa5:                /* MOVS */
+    case 0xaa: case 0xab:                /* STOS */
+    case X86EMUL_OPC(0x0f, 0x11):        /* MOVUPS */
+    case X86EMUL_OPC_VEX(0x0f, 0x11):    /* VMOVUPS */
+    case X86EMUL_OPC_66(0x0f, 0x11):     /* MOVUPD */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x11): /* VMOVUPD */
+    case X86EMUL_OPC_F3(0x0f, 0x11):     /* MOVSS */
+    case X86EMUL_OPC_VEX_F3(0x0f, 0x11): /* VMOVSS */
+    case X86EMUL_OPC_F2(0x0f, 0x11):     /* MOVSD */
+    case X86EMUL_OPC_VEX_F2(0x0f, 0x11): /* VMOVSD */
+    case X86EMUL_OPC(0x0f, 0x29):        /* MOVAPS */
+    case X86EMUL_OPC_VEX(0x0f, 0x29):    /* VMOVAPS */
+    case X86EMUL_OPC_66(0x0f, 0x29):     /* MOVAPD */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x29): /* VMOVAPD */
+    case X86EMUL_OPC(0x0f, 0x2b):        /* MOVNTPS */
+    case X86EMUL_OPC_VEX(0x0f, 0x2b):    /* VMOVNTPS */
+    case X86EMUL_OPC_66(0x0f, 0x2b):     /* MOVNTPD */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x2b): /* VMOVNTPD */
+    case X86EMUL_OPC(0x0f, 0x7e):        /* MOVD/MOVQ */
+    case X86EMUL_OPC_66(0x0f, 0x7e):     /* MOVD/MOVQ */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x7e): /* VMOVD/VMOVQ */
+    case X86EMUL_OPC(0x0f, 0x7f):        /* VMOVQ */
+    case X86EMUL_OPC_66(0x0f, 0x7f):     /* MOVDQA */
+    case X86EMUL_OPC_VEX_66(0x0f, 0x7f): /* VMOVDQA */
+    case X86EMUL_OPC_F3(0x0f, 0x7f):     /* MOVDQU */
+    case X86EMUL_OPC_VEX_F3(0x0f, 0x7f): /* VMOVDQU */
+    case X86EMUL_OPC(0x0f, 0xab):        /* BTS */
+    case X86EMUL_OPC(0x0f, 0xb3):        /* BTR */
+    case X86EMUL_OPC(0x0f, 0xbb):        /* BTC */
+    case X86EMUL_OPC_66(0x0f, 0xd6):     /* MOVQ */
+    case X86EMUL_OPC_VEX_66(0x0f, 0xd6): /* VMOVQ */
+    case X86EMUL_OPC(0x0f, 0xe7):        /* MOVNTQ */
+    case X86EMUL_OPC_66(0x0f, 0xe7):     /* MOVNTDQ */
+    case X86EMUL_OPC_VEX_66(0x0f, 0xe7): /* VMOVNTDQ */
+        return true;
+
+    case 0xd9:
+        switch ( state->modrm_reg & 7 )
+        {
+        case 2: /* FST m32fp */
+        case 3: /* FSTP m32fp */
+        case 6: /* FNSTENV */
+        case 7: /* FNSTCW */
+            return true;
+        }
+        break;
+
+    case 0xdb:
+        switch ( state->modrm_reg & 7 )
+        {
+        case 1: /* FISTTP m32i */
+        case 2: /* FIST m32i */
+        case 3: /* FISTP m32i */
+        case 7: /* FSTP m80fp */
+            return true;
+        }
+        break;
+
+    case 0xdd:
+        switch ( state->modrm_reg & 7 )
+        {
+        case 1: /* FISTTP m64i */
+        case 2: /* FST m64fp */
+        case 3: /* FSTP m64fp */
+        case 6: /* FNSAVE */
+        case 7: /* FNSTSW */
+            return true;
+        }
+        break;
+
+    case 0xdf:
+        switch ( state->modrm_reg & 7 )
+        {
+        case 1: /* FISTTP m16i */
+        case 2: /* FIST m16i */
+        case 3: /* FISTP m16i */
+        case 6: /* FBSTP */
+        case 7: /* FISTP m64i */
+            return true;
+        }
+        break;
+
+    case X86EMUL_OPC(0x0f, 0x01):
+        return !(state->modrm_reg & 6); /* SGDT / SIDT */
+
+    case X86EMUL_OPC(0x0f, 0xba):
+        return (state->modrm_reg & 7) > 4; /* BTS / BTR / BTC */
+    }
+
+    return false;
+}
+
 unsigned long
 x86_insn_immediate(const struct x86_emulate_state *state, unsigned int nr)
 {
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -631,6 +631,12 @@ x86_insn_immediate(const struct x86_emul
 unsigned int
 x86_insn_length(const struct x86_emulate_state *state,
                 const struct x86_emulate_ctxt *ctxt);
+bool
+x86_insn_is_mem_access(const struct x86_emulate_state *state,
+                       const struct x86_emulate_ctxt *ctxt);
+bool
+x86_insn_is_mem_write(const struct x86_emulate_state *state,
+                      const struct x86_emulate_ctxt *ctxt);
 
 #ifdef NDEBUG
 static inline void x86_emulate_free_state(struct x86_emulate_state *state) {}
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -17,9 +17,19 @@
 #include <asm/hvm/hvm.h>
 #include <asm/x86_emulate.h>
 
+struct hvm_emulate_ctxt;
+typedef int hvm_emulate_validate_t(const struct x86_emulate_state *state,
+                                   struct hvm_emulate_ctxt *ctxt);
+
 struct hvm_emulate_ctxt {
     struct x86_emulate_ctxt ctxt;
 
+    /*
+     * validate: Post-decode, pre-emulate hook to allow caller controlled
+     * filtering.
+     */
+    hvm_emulate_validate_t *validate;
+
     /* Cache of 16 bytes of instruction. */
     uint8_t insn_buf[16];
     unsigned long insn_buf_eip;
@@ -41,6 +51,8 @@ enum emul_kind {
     EMUL_KIND_SET_CONTEXT_INSN
 };
 
+bool __nonnull(1) hvm_emulate_one_insn(
+    hvm_emulate_validate_t *validate);
 int hvm_emulate_one(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
 void hvm_emulate_one_vm_event(enum emul_kind kind,
@@ -49,6 +61,7 @@ void hvm_emulate_one_vm_event(enum emul_
 /* Must be called once to set up hvmemul state. */
 void hvm_emulate_init_once(
     struct hvm_emulate_ctxt *hvmemul_ctxt,
+    hvm_emulate_validate_t *validate,
     struct cpu_user_regs *regs);
 /* Must be called once before each instruction emulated. */
 void hvm_emulate_init_per_insn(
@@ -68,6 +81,16 @@ struct segment_register *hvmemul_get_seg
     struct hvm_emulate_ctxt *hvmemul_ctxt);
 int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla);
 
+hvm_emulate_validate_t hvm_emulate_is_mem_access,
+                       hvm_emulate_is_mem_write,
+                       hvm_emulate_is_portio,
+                       hvm_emulate_is_cr_access;
+
+static inline bool handle_mmio(void)
+{
+    return hvm_emulate_one_insn(hvm_emulate_is_mem_access);
+}
+
 int hvmemul_insn_fetch(enum x86_segment seg,
                        unsigned long offset,
                        void *p_data,
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -118,7 +118,6 @@ void relocate_portio_handler(
 
 void send_timeoffset_req(unsigned long timeoff);
 void send_invalidate_req(void);
-bool handle_mmio(void);
 bool handle_mmio_with_translation(unsigned long gla, unsigned long gpfn,
                                   struct npfec);
 bool handle_pio(uint16_t port, unsigned int size, int dir);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[Andrew Cooper]

On 03/01/17 13:10, Jan Beulich wrote:
> Most invocations of the instruction emulator are for VM exits where the
> set of legitimate instructions (i.e. ones capable of causing the
> respective exit) is rather small. Restrict the permitted sets via a new
> callback, at once eliminating the abuse of handle_mmio() for non-MMIO
> operations.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: Better way to cover FPU/SIMD insns in x86_insn_is_mem_write()?

Not that I can see.

>
> Note that hvm_emulate_is_mem_*() (for now) intentionally don't include
> implicit memory operands: I don't think we mean to support namely
> the stack to live in MMIO, but otoh we may need to permit that.
>
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>  }
>  
> +static int hvmemul_validate(
> +    const struct x86_emulate_state *state,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> +
> +    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
> +                                  : X86EMUL_OKAY;

There is nothing hvm-specific about any of the validation functions, and
x86_insn_is_{portio,cr_access,is_invlpg} seem more generally useful than
hvm-specific varients.

Do you forsee any validation which would need to peek into hvmeml_ctxt? 
I can't think of anything off the top of my head.

If not, this would be cleaner and shorter to have an x86emul_validate_t
based interface, always passing const struct x86_emulate_ctxt *ctxt.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4004,7 +4004,7 @@ void hvm_ud_intercept(struct cpu_user_re
>          cur->domain->arch.x86_vendor != boot_cpu_data.x86_vendor;
>      struct hvm_emulate_ctxt ctxt;
>  
> -    hvm_emulate_init_once(&ctxt, regs);
> +    hvm_emulate_init_once(&ctxt, NULL, regs);

Please could we have a validation function here which, for the
opt_hvm_fep case permits everything, and for the cross-vendor case
permits only SYS{CALL,RET,ENTER,EXIT}?

This severely limits the attack surface even for a VM configured in
cross-vendor mode, and we only need to cope with instructions which have
different #UD behaviour between vendors.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -3774,7 +3774,7 @@ x86_emulate(
>                  emulate_fpu_insn_memsrc("flds", src.val);
>                  dst.type = OP_NONE;
>                  break;
> -            case 2: /* fstp m32fp */
> +            case 2: /* fst m32fp */

This change looks like it is spurious from a different patch?

~Andrew

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

Re: [PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[Jan Beulich]

>>> On 03.01.17 at 16:22, <andrew.cooper3@citrix.com> wrote:
> On 03/01/17 13:10, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
>>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>>  }
>>  
>> +static int hvmemul_validate(
>> +    const struct x86_emulate_state *state,
>> +    struct x86_emulate_ctxt *ctxt)
>> +{
>> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
>> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>> +
>> +    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
>> +                                  : X86EMUL_OKAY;
> 
> There is nothing hvm-specific about any of the validation functions, and
> x86_insn_is_{portio,cr_access,is_invlpg} seem more generally useful than
> hvm-specific varients.
> 
> Do you forsee any validation which would need to peek into hvmeml_ctxt? 
> I can't think of anything off the top of my head.
> 
> If not, this would be cleaner and shorter to have an x86emul_validate_t
> based interface, always passing const struct x86_emulate_ctxt *ctxt.

I had thought about this, but it feels like a layering violation to
pass a pointer to a function taking x86_emulate_ctxt to functions
in the HVM emulation group. Even if it involves adding slightly more
code, I think it would better stay this way.

>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4004,7 +4004,7 @@ void hvm_ud_intercept(struct cpu_user_re
>>          cur->domain->arch.x86_vendor != boot_cpu_data.x86_vendor;
>>      struct hvm_emulate_ctxt ctxt;
>>  
>> -    hvm_emulate_init_once(&ctxt, regs);
>> +    hvm_emulate_init_once(&ctxt, NULL, regs);
> 
> Please could we have a validation function here which, for the
> opt_hvm_fep case permits everything, and for the cross-vendor case
> permits only SYS{CALL,RET,ENTER,EXIT}?
> 
> This severely limits the attack surface even for a VM configured in
> cross-vendor mode, and we only need to cope with instructions which have
> different #UD behaviour between vendors.

I can certainly do that (albeit I'd pass NULL for the FEP case
instead of a function permitting everything), yet that will
lock us further into the corner where actively emulating insns
without hardware support is rather difficult to achieve.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -3774,7 +3774,7 @@ x86_emulate(
>>                  emulate_fpu_insn_memsrc("flds", src.val);
>>                  dst.type = OP_NONE;
>>                  break;
>> -            case 2: /* fstp m32fp */
>> +            case 2: /* fst m32fp */
> 
> This change looks like it is spurious from a different patch?

It doesn't belong anywhere - I found the comment wrong while
collecting the memory store insns, and putting this in a separate
patch didn't seem worthwhile. I've added a word to the commit
message.

Jan


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

Re: [PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[Andrew Cooper]

On 03/01/17 16:19, Jan Beulich wrote:
>>>> On 03.01.17 at 16:22, <andrew.cooper3@citrix.com> wrote:
>> On 03/01/17 13:10, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
>>>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>>>  }
>>>  
>>> +static int hvmemul_validate(
>>> +    const struct x86_emulate_state *state,
>>> +    struct x86_emulate_ctxt *ctxt)
>>> +{
>>> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
>>> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>> +
>>> +    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
>>> +                                  : X86EMUL_OKAY;
>> There is nothing hvm-specific about any of the validation functions, and
>> x86_insn_is_{portio,cr_access,is_invlpg} seem more generally useful than
>> hvm-specific varients.
>>
>> Do you forsee any validation which would need to peek into hvmeml_ctxt? 
>> I can't think of anything off the top of my head.
>>
>> If not, this would be cleaner and shorter to have an x86emul_validate_t
>> based interface, always passing const struct x86_emulate_ctxt *ctxt.
> I had thought about this, but it feels like a layering violation to
> pass a pointer to a function taking x86_emulate_ctxt to functions
> in the HVM emulation group. Even if it involves adding slightly more
> code, I think it would better stay this way.

Given that one structure is embedded in the other, I am less concerned
about this being a layering violation.

I was specifically thinking along the line of not needing hvm and sh
stubs to call into x86_insn_is_mem_access(), as the hvm/sh nature isn't
relevant to the operation.

>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4004,7 +4004,7 @@ void hvm_ud_intercept(struct cpu_user_re
>>>          cur->domain->arch.x86_vendor != boot_cpu_data.x86_vendor;
>>>      struct hvm_emulate_ctxt ctxt;
>>>  
>>> -    hvm_emulate_init_once(&ctxt, regs);
>>> +    hvm_emulate_init_once(&ctxt, NULL, regs);
>> Please could we have a validation function here which, for the
>> opt_hvm_fep case permits everything, and for the cross-vendor case
>> permits only SYS{CALL,RET,ENTER,EXIT}?
>>
>> This severely limits the attack surface even for a VM configured in
>> cross-vendor mode, and we only need to cope with instructions which have
>> different #UD behaviour between vendors.
> I can certainly do that (albeit I'd pass NULL for the FEP case
> instead of a function permitting everything), yet that will
> lock us further into the corner where actively emulating insns
> without hardware support is rather difficult to achieve.

Well, not really.  When we choose to offer that facility, it should come
with an opt-in, and we can extend the validate function to permit
instructions in classes permitted by the domain featureset, but missing
in the host featureset.

>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -3774,7 +3774,7 @@ x86_emulate(
>>>                  emulate_fpu_insn_memsrc("flds", src.val);
>>>                  dst.type = OP_NONE;
>>>                  break;
>>> -            case 2: /* fstp m32fp */
>>> +            case 2: /* fst m32fp */
>> This change looks like it is spurious from a different patch?
> It doesn't belong anywhere - I found the comment wrong while
> collecting the memory store insns, and putting this in a separate
> patch didn't seem worthwhile. I've added a word to the commit
> message.

Ok.

~Andrew

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

Re: [PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[Jan Beulich]

>>> On 03.01.17 at 18:29, <andrew.cooper3@citrix.com> wrote:
> On 03/01/17 16:19, Jan Beulich wrote:
>>>>> On 03.01.17 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>> On 03/01/17 13:10, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
>>>>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>>>>  }
>>>>  
>>>> +static int hvmemul_validate(
>>>> +    const struct x86_emulate_state *state,
>>>> +    struct x86_emulate_ctxt *ctxt)
>>>> +{
>>>> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
>>>> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>>> +
>>>> +    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
>>>> +                                  : X86EMUL_OKAY;
>>> There is nothing hvm-specific about any of the validation functions, and
>>> x86_insn_is_{portio,cr_access,is_invlpg} seem more generally useful than
>>> hvm-specific varients.
>>>
>>> Do you forsee any validation which would need to peek into hvmeml_ctxt? 
>>> I can't think of anything off the top of my head.
>>>
>>> If not, this would be cleaner and shorter to have an x86emul_validate_t
>>> based interface, always passing const struct x86_emulate_ctxt *ctxt.
>> I had thought about this, but it feels like a layering violation to
>> pass a pointer to a function taking x86_emulate_ctxt to functions
>> in the HVM emulation group. Even if it involves adding slightly more
>> code, I think it would better stay this way.
> 
> Given that one structure is embedded in the other, I am less concerned
> about this being a layering violation.
> 
> I was specifically thinking along the line of not needing hvm and sh
> stubs to call into x86_insn_is_mem_access(), as the hvm/sh nature isn't
> relevant to the operation.

Let me get a 3rd opinion then - Tim, if such filtering was added for
shadow mode code, would you rather see them go straight to an
x86_insn_is_*() function, or have a proper sh_*() layer in between?

Thanks, Jan


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

Re: [PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[Tim Deegan]

At 02:22 -0700 on 04 Jan (1483496577), Jan Beulich wrote:
> >>> On 03.01.17 at 18:29, <andrew.cooper3@citrix.com> wrote:
> > On 03/01/17 16:19, Jan Beulich wrote:
> >>>>> On 03.01.17 at 16:22, <andrew.cooper3@citrix.com> wrote:
> >>> On 03/01/17 13:10, Jan Beulich wrote:
> >>>> --- a/xen/arch/x86/hvm/emulate.c
> >>>> +++ b/xen/arch/x86/hvm/emulate.c
> >>>> @@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
> >>>>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
> >>>>  }
> >>>>  
> >>>> +static int hvmemul_validate(
> >>>> +    const struct x86_emulate_state *state,
> >>>> +    struct x86_emulate_ctxt *ctxt)
> >>>> +{
> >>>> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
> >>>> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> >>>> +
> >>>> +    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
> >>>> +                                  : X86EMUL_OKAY;
> >>> There is nothing hvm-specific about any of the validation functions, and
> >>> x86_insn_is_{portio,cr_access,is_invlpg} seem more generally useful than
> >>> hvm-specific varients.
> >>>
> >>> Do you forsee any validation which would need to peek into hvmeml_ctxt? 
> >>> I can't think of anything off the top of my head.
> >>>
> >>> If not, this would be cleaner and shorter to have an x86emul_validate_t
> >>> based interface, always passing const struct x86_emulate_ctxt *ctxt.
> >> I had thought about this, but it feels like a layering violation to
> >> pass a pointer to a function taking x86_emulate_ctxt to functions
> >> in the HVM emulation group. Even if it involves adding slightly more
> >> code, I think it would better stay this way.
> > 
> > Given that one structure is embedded in the other, I am less concerned
> > about this being a layering violation.
> > 
> > I was specifically thinking along the line of not needing hvm and sh
> > stubs to call into x86_insn_is_mem_access(), as the hvm/sh nature isn't
> > relevant to the operation.
> 
> Let me get a 3rd opinion then - Tim, if such filtering was added for
> shadow mode code, would you rather see them go straight to an
> x86_insn_is_*() function, or have a proper sh_*() layer in between?

I think checks on _kinds_ of instructions, like is_portio,
is_mem_access &c are best provided as generic x86_insn_is_*.  I don't
think I'd want to add sh_ wrappers that just called the x86_insn ones.

I'd also be OK with an enum passed to the emulator and no callback
function at all, if we can convince ourselves that every caller will
want to check for exactly 0 or 1 classes, and no other filtering --
maybe we can?

I have no problem with shadow-code functions taking pointers to
functions that take pointers to emulator context; and indeed I'd be
happy to put e.g. x86_insn_is_mem_write directly into the struct
x86_emulate_ops or pass it as another argument to x86_emulate().

Cheers,

Tim.

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

Re: [PATCH] x86/HVM: restrict permitted instructions during special purpose emulation

[Andrew Cooper]

On 04/01/17 10:10, Tim Deegan wrote:
> At 02:22 -0700 on 04 Jan (1483496577), Jan Beulich wrote:
>>>>> On 03.01.17 at 18:29, <andrew.cooper3@citrix.com> wrote:
>>> On 03/01/17 16:19, Jan Beulich wrote:
>>>>>>> On 03.01.17 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>>>> On 03/01/17 13:10, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>>>> @@ -1039,6 +1039,17 @@ static int hvmemul_cmpxchg(
>>>>>>      return hvmemul_write(seg, offset, p_new, bytes, ctxt);
>>>>>>  }
>>>>>>  
>>>>>> +static int hvmemul_validate(
>>>>>> +    const struct x86_emulate_state *state,
>>>>>> +    struct x86_emulate_ctxt *ctxt)
>>>>>> +{
>>>>>> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
>>>>>> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>>>>> +
>>>>>> +    return hvmemul_ctxt->validate ? hvmemul_ctxt->validate(state, hvmemul_ctxt)
>>>>>> +                                  : X86EMUL_OKAY;
>>>>> There is nothing hvm-specific about any of the validation functions, and
>>>>> x86_insn_is_{portio,cr_access,is_invlpg} seem more generally useful than
>>>>> hvm-specific varients.
>>>>>
>>>>> Do you forsee any validation which would need to peek into hvmeml_ctxt? 
>>>>> I can't think of anything off the top of my head.
>>>>>
>>>>> If not, this would be cleaner and shorter to have an x86emul_validate_t
>>>>> based interface, always passing const struct x86_emulate_ctxt *ctxt.
>>>> I had thought about this, but it feels like a layering violation to
>>>> pass a pointer to a function taking x86_emulate_ctxt to functions
>>>> in the HVM emulation group. Even if it involves adding slightly more
>>>> code, I think it would better stay this way.
>>> Given that one structure is embedded in the other, I am less concerned
>>> about this being a layering violation.
>>>
>>> I was specifically thinking along the line of not needing hvm and sh
>>> stubs to call into x86_insn_is_mem_access(), as the hvm/sh nature isn't
>>> relevant to the operation.
>> Let me get a 3rd opinion then - Tim, if such filtering was added for
>> shadow mode code, would you rather see them go straight to an
>> x86_insn_is_*() function, or have a proper sh_*() layer in between?
> I think checks on _kinds_ of instructions, like is_portio,
> is_mem_access &c are best provided as generic x86_insn_is_*.  I don't
> think I'd want to add sh_ wrappers that just called the x86_insn ones.
>
> I'd also be OK with an enum passed to the emulator and no callback
> function at all, if we can convince ourselves that every caller will
> want to check for exactly 0 or 1 classes, and no other filtering --
> maybe we can?

I considered suggesting this, but priv_op_validate() used by the PV path
really does need a full functions worth of flexibility.

A hybrid approach might also be an option if we end up with a lot of
simple tests like this, but I am not sure it is worth introducing at
this point.

~Andrew

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