| Message ID | 1530302344-9111-1-git-send-email-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Gentle ping. On 29/06/18 22:59, Liran Alon wrote: > When eVMCS is enabled, all VMCS allocated to be used by KVM are marked > with revision_id of KVM_EVMCS_VERSION instead of revision_id reported > by MSR_IA32_VMX_BASIC. > > However, even though not explictly documented by TLFS, VMXArea passed > as VMXON argument should still be marked with revision_id reported by > physical CPU. > > This issue was found by the following setup: > * L0 = KVM which expose eVMCS to it's L1 guest. > * L1 = KVM which consume eVMCS reported by L0. > This setup caused the following to occur: > 1) L1 execute hardware_enable(). > 2) hardware_enable() calls kvm_cpu_vmxon() to execute VMXON. > 3) L0 intercept L1 VMXON and execute handle_vmon() which notes > vmxarea->revision_id != VMCS12_REVISION and therefore fails with > nested_vmx_failInvalid() which sets RFLAGS.CF. > 4) L1 kvm_cpu_vmxon() don't check RFLAGS.CF for failure and therefore > hardware_enable() continues as usual. > 5) L1 hardware_enable() then calls ept_sync_global() which executes > INVEPT. > 6) L0 intercept INVEPT and execute handle_invept() which notes > !vmx->nested.vmxon and thus raise a #UD to L1. > 7) Raised #UD caused L1 to panic. > > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/kvm/vmx.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 1689f433f3a0..13d765e574ca 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -4322,11 +4322,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) > vmcs_conf->order = get_order(vmcs_conf->size); > vmcs_conf->basic_cap = vmx_msr_high & ~0x1fff; > > - /* KVM supports Enlightened VMCS v1 only */ > - if (static_branch_unlikely(&enable_evmcs)) > - vmcs_conf->revision_id = KVM_EVMCS_VERSION; > - else > - vmcs_conf->revision_id = vmx_msr_low; > + vmcs_conf->revision_id = vmx_msr_low; > > vmcs_conf->pin_based_exec_ctrl = _pin_based_exec_control; > vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control; > @@ -4396,7 +4392,13 @@ static struct vmcs *alloc_vmcs_cpu(int cpu) > return NULL; > vmcs = page_address(pages); > memset(vmcs, 0, vmcs_config.size); > - vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id */ > + > + /* KVM supports Enlightened VMCS v1 only */ > + if (static_branch_unlikely(&enable_evmcs)) > + vmcs->revision_id = KVM_EVMCS_VERSION; > + else > + vmcs->revision_id = vmcs_config.revision_id; > + > return vmcs; > } > > @@ -4564,6 +4566,19 @@ static __init int alloc_kvm_area(void) > return -ENOMEM; > } > > + /* > + * When eVMCS is enabled, alloc_vmcs_cpu() sets > + * vmcs->revision_id to KVM_EVMCS_VERSION instead of > + * revision_id reported by MSR_IA32_VMX_BASIC. > + * > + * However, even though not explictly documented by > + * TLFS, VMXArea passed as VMXON argument should > + * still be marked with revision_id reported by > + * physical CPU. > + */ > + if (static_branch_unlikely(&enable_evmcs)) > + vmcs->revision_id = vmcs_config.revision_id; > + > per_cpu(vmxarea, cpu) = vmcs; > } > return 0;
On 29/06/2018 21:59, Liran Alon wrote: > When eVMCS is enabled, all VMCS allocated to be used by KVM are marked > with revision_id of KVM_EVMCS_VERSION instead of revision_id reported > by MSR_IA32_VMX_BASIC. > > However, even though not explictly documented by TLFS, VMXArea passed > as VMXON argument should still be marked with revision_id reported by > physical CPU. > > This issue was found by the following setup: > * L0 = KVM which expose eVMCS to it's L1 guest. > * L1 = KVM which consume eVMCS reported by L0. > This setup caused the following to occur: > 1) L1 execute hardware_enable(). > 2) hardware_enable() calls kvm_cpu_vmxon() to execute VMXON. > 3) L0 intercept L1 VMXON and execute handle_vmon() which notes > vmxarea->revision_id != VMCS12_REVISION and therefore fails with > nested_vmx_failInvalid() which sets RFLAGS.CF. > 4) L1 kvm_cpu_vmxon() don't check RFLAGS.CF for failure and therefore > hardware_enable() continues as usual. > 5) L1 hardware_enable() then calls ept_sync_global() which executes > INVEPT. > 6) L0 intercept INVEPT and execute handle_invept() which notes > !vmx->nested.vmxon and thus raise a #UD to L1. > 7) Raised #UD caused L1 to panic. > > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> Queued for -rc, thanks. Paolo > --- > arch/x86/kvm/vmx.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 1689f433f3a0..13d765e574ca 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -4322,11 +4322,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) > vmcs_conf->order = get_order(vmcs_conf->size); > vmcs_conf->basic_cap = vmx_msr_high & ~0x1fff; > > - /* KVM supports Enlightened VMCS v1 only */ > - if (static_branch_unlikely(&enable_evmcs)) > - vmcs_conf->revision_id = KVM_EVMCS_VERSION; > - else > - vmcs_conf->revision_id = vmx_msr_low; > + vmcs_conf->revision_id = vmx_msr_low; > > vmcs_conf->pin_based_exec_ctrl = _pin_based_exec_control; > vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control; > @@ -4396,7 +4392,13 @@ static struct vmcs *alloc_vmcs_cpu(int cpu) > return NULL; > vmcs = page_address(pages); > memset(vmcs, 0, vmcs_config.size); > - vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id */ > + > + /* KVM supports Enlightened VMCS v1 only */ > + if (static_branch_unlikely(&enable_evmcs)) > + vmcs->revision_id = KVM_EVMCS_VERSION; > + else > + vmcs->revision_id = vmcs_config.revision_id; > + > return vmcs; > } > > @@ -4564,6 +4566,19 @@ static __init int alloc_kvm_area(void) > return -ENOMEM; > } > > + /* > + * When eVMCS is enabled, alloc_vmcs_cpu() sets > + * vmcs->revision_id to KVM_EVMCS_VERSION instead of > + * revision_id reported by MSR_IA32_VMX_BASIC. > + * > + * However, even though not explictly documented by > + * TLFS, VMXArea passed as VMXON argument should > + * still be marked with revision_id reported by > + * physical CPU. > + */ > + if (static_branch_unlikely(&enable_evmcs)) > + vmcs->revision_id = vmcs_config.revision_id; > + > per_cpu(vmxarea, cpu) = vmcs; > } > return 0; >
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 1689f433f3a0..13d765e574ca 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -4322,11 +4322,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) vmcs_conf->order = get_order(vmcs_conf->size); vmcs_conf->basic_cap = vmx_msr_high & ~0x1fff; - /* KVM supports Enlightened VMCS v1 only */ - if (static_branch_unlikely(&enable_evmcs)) - vmcs_conf->revision_id = KVM_EVMCS_VERSION; - else - vmcs_conf->revision_id = vmx_msr_low; + vmcs_conf->revision_id = vmx_msr_low; vmcs_conf->pin_based_exec_ctrl = _pin_based_exec_control; vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control; @@ -4396,7 +4392,13 @@ static struct vmcs *alloc_vmcs_cpu(int cpu) return NULL; vmcs = page_address(pages); memset(vmcs, 0, vmcs_config.size); - vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id */ + + /* KVM supports Enlightened VMCS v1 only */ + if (static_branch_unlikely(&enable_evmcs)) + vmcs->revision_id = KVM_EVMCS_VERSION; + else + vmcs->revision_id = vmcs_config.revision_id; + return vmcs; } @@ -4564,6 +4566,19 @@ static __init int alloc_kvm_area(void) return -ENOMEM; } + /* + * When eVMCS is enabled, alloc_vmcs_cpu() sets + * vmcs->revision_id to KVM_EVMCS_VERSION instead of + * revision_id reported by MSR_IA32_VMX_BASIC. + * + * However, even though not explictly documented by + * TLFS, VMXArea passed as VMXON argument should + * still be marked with revision_id reported by + * physical CPU. + */ + if (static_branch_unlikely(&enable_evmcs)) + vmcs->revision_id = vmcs_config.revision_id; + per_cpu(vmxarea, cpu) = vmcs; } return 0;