|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH 1/2] x86/hvm: Don't raise #GP behind the emulators back for CR accesses
hvm_set_cr{0,4}() are reachable from the emulator, but use
hvm_inject_hw_exception() directly.
Alter the API to make the callers of hvm_set_cr{0,3,4}() responsible for
raising #GP, and apply this change to all existing callers.
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
CC: Kevin Tian <kevin.tian@xxxxxxxxx>
CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
Issues identified which I am purposefully not fixing in this patch:
(I will try to get around to them, but probably not in the 4.9 timeframe, at
this point.)
* hvm_set_cr3() doesn't handle bad 32bit PAE PDPTRs properly, as it doesn't
actually have a path which raises #GP.
* There is a lot of redundancy in our HVM CR setting routines, but not enough
to trivially dedup at this point.
* Both nested VT-x and SVM are liable raise #GP with L1, rather than failing
the virtual vmentry/vmexit. This is not a change in behaviour, but is far
more obvious now.
* The hvm_do_resume() path for vm_event processing has the same bug as the
MSR side, where exceptions are raised after %rip has moved forwards. This
is also not a change in behaviour.
---
xen/arch/x86/hvm/emulate.c | 24 ++++++++++++----
xen/arch/x86/hvm/hvm.c | 59 ++++++++++++++++++++++-----------------
xen/arch/x86/hvm/svm/nestedsvm.c | 14 ++++++++++
xen/arch/x86/hvm/vmx/vmx.c | 7 ++++-
xen/arch/x86/hvm/vmx/vvmx.c | 29 +++++++++++++++----
xen/include/asm-x86/hvm/support.h | 6 +++-
6 files changed, 101 insertions(+), 38 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 93782d0..1c66010 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1520,23 +1520,37 @@ static int hvmemul_write_cr(
unsigned long val,
struct x86_emulate_ctxt *ctxt)
{
+ int rc;
+
HVMTRACE_LONG_2D(CR_WRITE, reg, TRC_PAR_LONG(val));
switch ( reg )
{
case 0:
- return hvm_set_cr0(val, 1);
+ rc = hvm_set_cr0(val, 1);
+ break;
+
case 2:
current->arch.hvm_vcpu.guest_cr[2] = val;
- return X86EMUL_OKAY;
+ rc = X86EMUL_OKAY;
+ break;
+
case 3:
- return hvm_set_cr3(val, 1);
+ rc = hvm_set_cr3(val, 1);
+ break;
+
case 4:
- return hvm_set_cr4(val, 1);
+ rc = hvm_set_cr4(val, 1);
+ break;
+
default:
+ rc = X86EMUL_UNHANDLEABLE;
break;
}
- return X86EMUL_UNHANDLEABLE;
+ if ( rc == X86EMUL_EXCEPTION )
+ x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
+
+ return rc;
}
static int hvmemul_read_msr(
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7432c70..ccfae4f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -527,19 +527,25 @@ void hvm_do_resume(struct vcpu *v)
if ( w->do_write.cr0 )
{
- hvm_set_cr0(w->cr0, 0);
+ if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
w->do_write.cr0 = 0;
}
if ( w->do_write.cr4 )
{
- hvm_set_cr4(w->cr4, 0);
+ if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
w->do_write.cr4 = 0;
}
if ( w->do_write.cr3 )
{
- hvm_set_cr3(w->cr3, 0);
+ if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
w->do_write.cr3 = 0;
}
}
@@ -2068,6 +2074,7 @@ int hvm_mov_to_cr(unsigned int cr, unsigned int gpr)
{
struct vcpu *curr = current;
unsigned long val, *reg;
+ int rc;
if ( (reg = decode_register(gpr, guest_cpu_user_regs(), 0)) == NULL )
{
@@ -2082,16 +2089,20 @@ int hvm_mov_to_cr(unsigned int cr, unsigned int gpr)
switch ( cr )
{
case 0:
- return hvm_set_cr0(val, 1);
+ rc = hvm_set_cr0(val, 1);
+ break;
case 3:
- return hvm_set_cr3(val, 1);
+ rc = hvm_set_cr3(val, 1);
+ break;
case 4:
- return hvm_set_cr4(val, 1);
+ rc = hvm_set_cr4(val, 1);
+ break;
case 8:
vlapic_set_reg(vcpu_vlapic(curr), APIC_TASKPRI, ((val & 0x0f) << 4));
+ rc = X86EMUL_OKAY;
break;
default:
@@ -2099,7 +2110,10 @@ int hvm_mov_to_cr(unsigned int cr, unsigned int gpr)
goto exit_and_crash;
}
- return X86EMUL_OKAY;
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+ return rc;
exit_and_crash:
domain_crash(curr->domain);
@@ -2199,7 +2213,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
HVM_DBG_LOG(DBG_LEVEL_1,
"Guest attempts to set upper 32 bits in CR0: %lx",
value);
- goto gpf;
+ return X86EMUL_EXCEPTION;
}
value &= ~HVM_CR0_GUEST_RESERVED_BITS;
@@ -2209,7 +2223,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
if ( !nestedhvm_vmswitch_in_progress(v) &&
(value & (X86_CR0_PE | X86_CR0_PG)) == X86_CR0_PG )
- goto gpf;
+ return X86EMUL_EXCEPTION;
/* A pvh is not expected to change to real mode. */
if ( is_pvh_domain(d) &&
@@ -2217,7 +2231,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
{
printk(XENLOG_G_WARNING
"PVH attempting to turn off PE/PG. CR0:%lx\n", value);
- goto gpf;
+ return X86EMUL_EXCEPTION;
}
if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled &
@@ -2243,7 +2257,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
!nestedhvm_vmswitch_in_progress(v) )
{
HVM_DBG_LOG(DBG_LEVEL_1, "Enable paging before PAE enable");
- goto gpf;
+ return X86EMUL_EXCEPTION;
}
HVM_DBG_LOG(DBG_LEVEL_1, "Enabling long mode");
v->arch.hvm_vcpu.guest_efer |= EFER_LMA;
@@ -2276,7 +2290,7 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
{
HVM_DBG_LOG(DBG_LEVEL_1, "Guest attempts to clear CR0.PG "
"while CR4.PCIDE=1");
- goto gpf;
+ return X86EMUL_EXCEPTION;
}
/* When CR0.PG is cleared, LMA is cleared immediately. */
@@ -2310,10 +2324,6 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
}
return X86EMUL_OKAY;
-
- gpf:
- hvm_inject_hw_exception(TRAP_gp_fault, 0);
- return X86EMUL_EXCEPTION;
}
int hvm_set_cr3(unsigned long value, bool_t may_defer)
@@ -2373,7 +2383,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
HVM_DBG_LOG(DBG_LEVEL_1,
"Guest attempts to set reserved bit in CR4: %lx",
value);
- goto gpf;
+ return X86EMUL_EXCEPTION;
}
if ( !(value & X86_CR4_PAE) )
@@ -2382,12 +2392,12 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
{
HVM_DBG_LOG(DBG_LEVEL_1, "Guest cleared CR4.PAE while "
"EFER.LMA is set");
- goto gpf;
+ return X86EMUL_EXCEPTION;
}
if ( is_pvh_vcpu(v) )
{
HVM_DBG_LOG(DBG_LEVEL_1, "32-bit PVH guest cleared CR4.PAE");
- goto gpf;
+ return X86EMUL_EXCEPTION;
}
}
@@ -2399,7 +2409,7 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
{
HVM_DBG_LOG(DBG_LEVEL_1, "Guest attempts to change CR4.PCIDE from "
"0 to 1 while either EFER.LMA=0 or CR3[11:0]!=000H");
- goto gpf;
+ return X86EMUL_EXCEPTION;
}
if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled &
@@ -2434,10 +2444,6 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
}
return X86EMUL_OKAY;
-
- gpf:
- hvm_inject_hw_exception(TRAP_gp_fault, 0);
- return X86EMUL_EXCEPTION;
}
bool_t hvm_virtual_to_linear_addr(
@@ -3020,7 +3026,10 @@ void hvm_task_switch(
if ( hvm_load_segment_selector(x86_seg_ldtr, tss.ldt, 0) )
goto out;
- if ( hvm_set_cr3(tss.cr3, 1) )
+ rc = hvm_set_cr3(tss.cr3, 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+ if ( rc != X86EMUL_OKAY )
goto out;
regs->rip = tss.eip;
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index f7b7ada..d4fc81f 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -286,6 +286,8 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct
cpu_user_regs *regs)
/* CR4 */
v->arch.hvm_vcpu.guest_cr[4] = n1vmcb->_cr4;
rc = hvm_set_cr4(n1vmcb->_cr4, 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
if (rc != X86EMUL_OKAY)
gdprintk(XENLOG_ERR, "hvm_set_cr4 failed, rc: %u\n", rc);
@@ -295,6 +297,8 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct
cpu_user_regs *regs)
v->arch.hvm_vcpu.guest_cr[0] = n1vmcb->_cr0 | X86_CR0_PE;
n1vmcb->rflags &= ~X86_EFLAGS_VM;
rc = hvm_set_cr0(n1vmcb->_cr0 | X86_CR0_PE, 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
if (rc != X86EMUL_OKAY)
gdprintk(XENLOG_ERR, "hvm_set_cr0 failed, rc: %u\n", rc);
svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0];
@@ -321,6 +325,8 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct
cpu_user_regs *regs)
/* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
}
rc = hvm_set_cr3(n1vmcb->_cr3, 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
if (rc != X86EMUL_OKAY)
gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
@@ -548,6 +554,8 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct
cpu_user_regs *regs)
/* CR4 */
v->arch.hvm_vcpu.guest_cr[4] = ns_vmcb->_cr4;
rc = hvm_set_cr4(ns_vmcb->_cr4, 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
if (rc != X86EMUL_OKAY)
gdprintk(XENLOG_ERR, "hvm_set_cr4 failed, rc: %u\n", rc);
@@ -556,6 +564,8 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct
cpu_user_regs *regs)
cr0 = nestedsvm_fpu_vmentry(svm->ns_cr0, ns_vmcb, n1vmcb, n2vmcb);
v->arch.hvm_vcpu.guest_cr[0] = ns_vmcb->_cr0;
rc = hvm_set_cr0(cr0, 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
if (rc != X86EMUL_OKAY)
gdprintk(XENLOG_ERR, "hvm_set_cr0 failed, rc: %u\n", rc);
@@ -572,6 +582,8 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct
cpu_user_regs *regs)
/* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
rc = hvm_set_cr3(ns_vmcb->_cr3, 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
if (rc != X86EMUL_OKAY)
gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
} else if (paging_mode_hap(v->domain)) {
@@ -584,6 +596,8 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct
cpu_user_regs *regs)
*/
/* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
rc = hvm_set_cr3(ns_vmcb->_cr3, 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
if (rc != X86EMUL_OKAY)
gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
} else {
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 5b1717d..c2cd12d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2459,13 +2459,18 @@ static int vmx_cr_access(unsigned long
exit_qualification)
}
case VMX_CONTROL_REG_ACCESS_TYPE_LMSW: {
unsigned long value = curr->arch.hvm_vcpu.guest_cr[0];
+ int rc;
/* LMSW can (1) set PE; (2) set or clear MP, EM, and TS. */
value = (value & ~(X86_CR0_MP|X86_CR0_EM|X86_CR0_TS)) |
(VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
(X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS));
HVMTRACE_LONG_1D(LMSW, value);
- return hvm_set_cr0(value, 1);
+
+ if ( (rc = hvm_set_cr0(value, 1)) == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+ return rc;
}
default:
BUG();
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 0aef7a7..8a486f5 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1046,9 +1046,18 @@ static void load_shadow_guest_state(struct vcpu *v)
nvcpu->guest_cr[0] = get_vvmcs(v, CR0_READ_SHADOW);
nvcpu->guest_cr[4] = get_vvmcs(v, CR4_READ_SHADOW);
- hvm_set_cr0(get_vvmcs(v, GUEST_CR0), 1);
- hvm_set_cr4(get_vvmcs(v, GUEST_CR4), 1);
- hvm_set_cr3(get_vvmcs(v, GUEST_CR3), 1);
+
+ rc = hvm_set_cr0(get_vvmcs(v, GUEST_CR0), 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+ rc = hvm_set_cr4(get_vvmcs(v, GUEST_CR4), 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+ rc = hvm_set_cr3(get_vvmcs(v, GUEST_CR3), 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
control = get_vvmcs(v, VM_ENTRY_CONTROLS);
if ( control & VM_ENTRY_LOAD_GUEST_PAT )
@@ -1237,9 +1246,17 @@ static void load_vvmcs_host_state(struct vcpu *v)
__vmwrite(vmcs_h2g_field[i].guest_field, r);
}
- hvm_set_cr0(get_vvmcs(v, HOST_CR0), 1);
- hvm_set_cr4(get_vvmcs(v, HOST_CR4), 1);
- hvm_set_cr3(get_vvmcs(v, HOST_CR3), 1);
+ rc = hvm_set_cr0(get_vvmcs(v, HOST_CR0), 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+ rc = hvm_set_cr4(get_vvmcs(v, HOST_CR4), 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+ rc = hvm_set_cr3(get_vvmcs(v, HOST_CR3), 1);
+ if ( rc == X86EMUL_EXCEPTION )
+ hvm_inject_hw_exception(TRAP_gp_fault, 0);
control = get_vvmcs(v, VM_EXIT_CONTROLS);
if ( control & VM_EXIT_LOAD_HOST_PAT )
diff --git a/xen/include/asm-x86/hvm/support.h
b/xen/include/asm-x86/hvm/support.h
index cb41364..632eb90 100644
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -119,7 +119,11 @@ int __must_check hvm_handle_xsetbv(u32 index, u64 new_bv);
void hvm_shadow_handle_cd(struct vcpu *v, unsigned long value);
-/* These functions all return X86EMUL return codes. */
+/*
+ * These functions all return X86EMUL return codes. For hvm_set_*(), the
+ * caller is responsible for injecting #GP[0] if X86EMUL_EXCEPTION is
+ * returned.
+ */
int hvm_set_efer(uint64_t value);
int hvm_set_cr0(unsigned long value, bool_t may_defer);
int hvm_set_cr3(unsigned long value, bool_t may_defer);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |