|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [PATCH v3] x86/nestedhvm: process the MSR-load and MSR-store areas of L1's VMCS
Nothing in vvmx.c reads the VM-entry MSR-load, VM-exit MSR-store or VM-exit
MSR-load area of L1's VMCS, so L2 runs with the listed MSRs as the previous
L2 or L1 left them, and L1 gets neither L2's values stored nor its own
loaded back. Process each area where the SDM does, with the SDM's checks,
an entry as a WRMSR or RDMSR at CPL 0 that L1 does not intercept. A bad
VM-entry entry fails the VM entry with exit reason 34 and leaves the VMCS
unlaunched, a bad VM-exit entry is a VMX abort, which crashes the domain,
and IA32_VMX_MISC now recommends the 512 entries an area may hold.
Fixes: 9390642b7fa5 ("Nested VMX: Switch shadow/virtual VMCS between n1/n2
guests.")
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Mahmoud Abumandour <ma.mandourr@xxxxxxxxx>
---
Changes in v3:
- add signed-off-by line
Changes in v2:
- use paddr_t and unsigned int in msr_area_valid() (Jan)
- reword process_msr_area()'s comment (Jan)
Found by a fuzzer for VMX isolation.
A VMX abort crashes the domain as vvmx.c does elsewhere when nested VMX
cannot continue.
Alternatives: hvm_triple_fault(), which applies the domain's triple-fault
action (reboot by default), as KVM does, or hvm_vcpu_down() on the aborting
vCPU alone with the VMX-abort indicator written, which is closest to the
SDM but needs more code. Happy to switch to either.
xen/arch/x86/hvm/vmx/vvmx.c | 136 ++++++++++++++++++++----
xen/arch/x86/include/asm/hvm/vmx/vmcs.h | 1 +
2 files changed, 118 insertions(+), 19 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 3309432873..dc5f4b7bfe 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -23,6 +23,8 @@
static DEFINE_PER_CPU(u64 *, vvmcs_buf);
static void nvmx_purge_vvmcs(struct vcpu *v);
+static int set_vvmcs_launched(struct list_head *launched_list,
+ unsigned long vvmcs_mfn);
static bool nvmx_vcpu_in_vmx(const struct vcpu *v)
{
@@ -1212,11 +1214,68 @@ static void nvmx_clear_vmcs_pointer(struct vcpu *v,
struct vmcs_struct *vvmcs)
__vmwrite(VMWRITE_BITMAP, 0);
}
-static void virtual_vmentry(struct cpu_user_regs *regs)
+/* The longest MSR list that IA32_VMX_MISC recommends to L1. */
+#define NVMX_MAX_MSRS 512
+
+/* A nonempty MSR area must be 16-byte aligned and within MAXPHYADDR. */
+static bool msr_area_valid(const struct vcpu *v, unsigned int addr_field,
+ unsigned int count_field)
+{
+ paddr_t addr = get_vvmcs(v, addr_field);
+ unsigned int count = get_vvmcs(v, count_field);
+ unsigned int width = v->domain->arch.cpuid->extd.maxphysaddr;
+
+ return !count ||
+ (!(addr & 0xf) && !(addr >> width) &&
+ !((addr + count * sizeof(struct vmx_msr_entry) - 1) >> width));
+}
+
+/*
+ * Process an MSR load or store area of L1's VMCS, each entry as an
+ * unintercepted WRMSR or RDMSR. Returns 0, or the 1-based index of the
+ * failing entry.
+ */
+static unsigned int process_msr_area(struct vcpu *v, unsigned int addr_field,
+ unsigned int count_field)
+{
+ paddr_t addr = get_vvmcs(v, addr_field);
+ unsigned int i, count = get_vvmcs(v, count_field);
+ struct vmx_msr_entry ent;
+
+ for ( i = 0; i < count; i++, addr += sizeof(ent) )
+ {
+ if ( i == NVMX_MAX_MSRS ||
+ hvm_copy_from_guest_phys(&ent, addr,
+ sizeof(ent)) != HVMTRANS_okay ||
+ ent.mbz ||
+ (ent.index >= MSR_X2APIC_FIRST && ent.index <= MSR_X2APIC_LAST) )
+ return i + 1;
+
+ if ( addr_field == VM_EXIT_MSR_STORE_ADDR )
+ {
+ if ( hvm_msr_read_intercept(ent.index, &ent.data) != X86EMUL_OKAY
||
+ hvm_copy_to_guest_phys(addr + offsetof(struct vmx_msr_entry,
+ data),
+ &ent.data, sizeof(ent.data),
+ v) != HVMTRANS_okay )
+ return i + 1;
+ }
+ else if ( ent.index == MSR_FS_BASE || ent.index == MSR_GS_BASE ||
+ hvm_msr_write_intercept(ent.index, ent.data,
+ false) != X86EMUL_OKAY )
+ return i + 1;
+ }
+
+ return 0;
+}
+
+/* Returns 0, or the 1-based index of the failing MSR-load entry. */
+static unsigned int virtual_vmentry(struct cpu_user_regs *regs)
{
struct vcpu *v = current;
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
unsigned long lm_l1, lm_l2;
+ unsigned int msr_fail;
vmx_vmcs_switch(v->arch.hvm.vmx.vmcs_pa, nvcpu->nv_n2vmcx_pa);
@@ -1277,6 +1336,15 @@ static void virtual_vmentry(struct cpu_user_regs *regs)
}
}
+ /* MSRs load last, and only a successful entry launches the VMCS. */
+ msr_fail = process_msr_area(v, VM_ENTRY_MSR_LOAD_ADDR,
+ VM_ENTRY_MSR_LOAD_COUNT);
+ if ( !msr_fail &&
+ set_vvmcs_launched(&vcpu_2_nvmx(v).launched_list,
+ PFN_DOWN(v->arch.hvm.vmx.vmcs_shadow_maddr)) < 0 )
+ domain_crash(v->domain);
+
+ return msr_fail;
}
static void sync_vvmcs_guest_state(struct vcpu *v, struct cpu_user_regs *regs)
@@ -1454,19 +1522,37 @@ static void nvmx_update_apicv(struct vcpu *v)
}
}
-static void virtual_vmexit(struct cpu_user_regs *regs)
+/*
+ * A VM exit, or with msr_fail a VM entry that failed loading MSRs, which
+ * records only the exit reason and qualification and saves nothing.
+ */
+static void virtual_vmexit(struct cpu_user_regs *regs, unsigned int msr_fail)
{
struct vcpu *v = current;
struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
- unsigned long lm_l1, lm_l2;
+ unsigned long lm_l1, lm_l2, intr_info = 0;
- sync_vvmcs_ro(v);
- sync_vvmcs_guest_state(v, regs);
- sync_exception_state(v);
+ if ( msr_fail )
+ {
+ intr_info = get_vvmcs(v, VM_ENTRY_INTR_INFO);
+ set_vvmcs(v, VM_EXIT_REASON,
+ VMX_EXIT_REASONS_FAILED_VMENTRY | EXIT_REASON_MSR_LOADING);
+ set_vvmcs(v, EXIT_QUALIFICATION, msr_fail);
+ }
+ else
+ {
+ sync_vvmcs_ro(v);
+ sync_vvmcs_guest_state(v, regs);
+ sync_exception_state(v);
- if ( nvmx_ept_enabled(v) && hvm_pae_enabled(v) &&
- !hvm_long_mode_active(v) )
- shadow_to_vvmcs_bulk(v, ARRAY_SIZE(gpdpte_fields), gpdpte_fields);
+ if ( nvmx_ept_enabled(v) && hvm_pae_enabled(v) &&
+ !hvm_long_mode_active(v) )
+ shadow_to_vvmcs_bulk(v, ARRAY_SIZE(gpdpte_fields), gpdpte_fields);
+
+ if ( process_msr_area(v, VM_EXIT_MSR_STORE_ADDR,
+ VM_EXIT_MSR_STORE_COUNT) )
+ domain_crash(v->domain, "%pv VMX abort: storing MSRs failed\n", v);
+ }
/* This will clear current pCPU bit in p2m->dirty_cpumask */
np2m_schedule(NP2M_SCHEDLE_OUT);
@@ -1490,6 +1576,9 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
vmx_update_exception_bitmap(v);
load_vvmcs_host_state(v);
+ /* A failed VM entry keeps its interruption information. */
+ if ( msr_fail )
+ set_vvmcs(v, VM_ENTRY_INTR_INFO, intr_info);
if ( lm_l1 != lm_l2 )
paging_update_paging_modes(v);
@@ -1499,6 +1588,9 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
/* VM exit clears all bits except bit 1 */
regs->rflags = X86_EFLAGS_MBS;
+ if ( process_msr_area(v, VM_EXIT_MSR_LOAD_ADDR, VM_EXIT_MSR_LOAD_COUNT) )
+ domain_crash(v->domain, "%pv VMX abort: loading MSRs failed\n", v);
+
if ( cpu_has_vmx_virtual_intr_delivery )
nvmx_update_apicv(v);
@@ -1553,9 +1645,14 @@ void asmlinkage nvmx_switch_guest(void)
return;
if ( nestedhvm_vcpu_in_guestmode(v) && nvcpu->nv_vmexit_pending )
- virtual_vmexit(regs);
+ virtual_vmexit(regs, 0);
else if ( !nestedhvm_vcpu_in_guestmode(v) && nvcpu->nv_vmentry_pending )
- virtual_vmentry(regs);
+ {
+ unsigned int msr_fail = virtual_vmentry(regs);
+
+ if ( msr_fail )
+ virtual_vmexit(regs, msr_fail);
+ }
}
/*
@@ -1704,6 +1801,11 @@ static enum vmx_insn_errno nvmx_vmresume(struct vcpu *v)
goto invalid_control_state;
}
+ if ( !msr_area_valid(v, VM_EXIT_MSR_STORE_ADDR, VM_EXIT_MSR_STORE_COUNT) ||
+ !msr_area_valid(v, VM_EXIT_MSR_LOAD_ADDR, VM_EXIT_MSR_LOAD_COUNT) ||
+ !msr_area_valid(v, VM_ENTRY_MSR_LOAD_ADDR, VM_ENTRY_MSR_LOAD_COUNT) )
+ goto invalid_control_state;
+
nvcpu->nv_vmentry_pending = 1;
return VMX_INSN_SUCCEED;
@@ -1777,15 +1879,10 @@ static int nvmx_handle_vmlaunch(struct cpu_user_regs
*regs)
return X86EMUL_OKAY;
}
else {
+ /* virtual_vmentry() launches the VMCS. */
rc = nvmx_vmresume(v);
if ( rc )
vmfail_valid(regs, rc);
- else
- {
- if ( set_vvmcs_launched(&nvmx->launched_list,
-
PFN_DOWN(v->arch.hvm.vmx.vmcs_shadow_maddr)) < 0 )
- return X86EMUL_UNHANDLEABLE;
- }
rc = X86EMUL_OKAY;
}
return rc;
@@ -2354,8 +2451,9 @@ int nvmx_msr_read_intercept(unsigned int msr, u64
*msr_content)
data = hvm_cr4_guest_valid_bits(d);
break;
case MSR_IA32_VMX_MISC:
- /* Do not support CR3-targets or activity states. */
- data = host_data & ~(VMX_MISC_CR3_TARGET | VMX_MISC_ACTIVITY_MASK);
+ /* No CR3-targets or activity states; MSR lists of NVMX_MAX_MSRS. */
+ data = host_data & ~(VMX_MISC_CR3_TARGET | VMX_MISC_ACTIVITY_MASK |
+ VMX_MISC_MAX_MSRS);
break;
case MSR_IA32_VMX_EPT_VPID_CAP:
data = nept_get_ept_vpid_cap();
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
index d0716e97d2..24b502382f 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -304,6 +304,7 @@ void vmx_vmcs_reload(struct vcpu *v);
#define VMX_MISC_ACTIVITY_MASK 0x000001c0
#define VMX_MISC_PROC_TRACE 0x00004000
#define VMX_MISC_CR3_TARGET 0x01ff0000
+#define VMX_MISC_MAX_MSRS 0x0e000000
#define VMX_MISC_VMWRITE_ALL 0x20000000
#define VMX_TSC_MULTIPLIER_MAX 0xffffffffffffffffULL
--
2.25.1
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |