On 18/05/2016 16:48, Roman Kagan wrote:
> The function to update APICv on/off state (in particular, to deactivate
> it when enabling Hyper-V SynIC), used to be incomplete: it didn't adjust
> APICv-related fields among secondary processor-based VM-execution
> controls.
>
> As a result, Windows 2012 guests would get stuck when SynIC-based
> auto-EOI interrupt intersected with e.g. an IPI in the guest.
>
> In addition, the MSR intercept bitmap wasn't updated to correspond to
> whether "virtualize x2APIC mode" was enabled. This path used not to be
> triggered, since Windows didn't use x2APIC but rather their own
> synthetic APIC access MSRs; however it represented a security risk
> because the guest running in a SynIC-enabled VM could switch to x2APIC
> and thus obtain direct access to host APIC MSRs (thanks to Yang Zhang
> <yang.zhang.wz@xxxxxxxxx> for spotting this).
>
> The patch fixes those omissions.
>
> Signed-off-by: Roman Kagan <rkagan@xxxxxxxxxxxxx>
> Cc: Steve Rutherford <srutherford@xxxxxxxxxx>
> Cc: Yang Zhang <yang.zhang.wz@xxxxxxxxx>
Thanks, I am queuing this patch for testing. Just a little nit, commit
messages usually refer to bugs in the present tense:
kvm:vmx: more complete state update on APICv on/off
The function to update APICv on/off state (in particular, to deactivate
it when enabling Hyper-V SynIC) is incomplete: it doesn't adjust
APICv-related fields among secondary processor-based VM-execution
controls. As a result, Windows 2012 guests get stuck when SynIC-based
auto-EOI interrupt intersected with e.g. an IPI in the guest.
In addition, the MSR intercept bitmap isn't updated every time "virtualize
x2APIC mode" is toggled. This path can only be triggered by a malicious
guest, because Windows didn't use x2APIC but rather their own synthetic
APIC access MSRs; however a guest running in a SynIC-enabled VM could
switch to x2APIC and thus obtain direct access to host APIC MSRs.
The patch fixes those omissions.
The idea is that the commit message is the body of an email message, and
therefore it describes the situation to the recipient before the change
is applied.
Thanks,
Paolo
> ---
> v2 -> v3:
> - only switch to x2apic msr bitmap if virtualize x2apic mode is on in vmcs
>
> v1 -> v2:
> - only update relevant bits in the secondary exec control
> - update msr intercept bitmap (also make x2apic msr bitmap always
> correspond to APICv)
>
> arch/x86/kvm/vmx.c | 48 ++++++++++++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ee1c8a9..cef741a 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2418,7 +2418,9 @@ static void vmx_set_msr_bitmap(struct kvm_vcpu *vcpu)
>
> if (is_guest_mode(vcpu))
> msr_bitmap = vmx_msr_bitmap_nested;
> - else if (vcpu->arch.apic_base & X2APIC_ENABLE) {
> + else if (cpu_has_secondary_exec_ctrls() &&
> + (vmcs_read32(SECONDARY_VM_EXEC_CONTROL) &
> + SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE)) {
> if (is_long_mode(vcpu))
> msr_bitmap = vmx_msr_bitmap_longmode_x2apic;
> else
> @@ -4783,6 +4785,19 @@ static void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
> struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, vmx_pin_based_exec_ctrl(vmx));
> + if (cpu_has_secondary_exec_ctrls()) {
> + if (kvm_vcpu_apicv_active(vcpu))
> + vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
> + SECONDARY_EXEC_APIC_REGISTER_VIRT |
> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> + else
> + vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
> + SECONDARY_EXEC_APIC_REGISTER_VIRT |
> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
> + }
> +
> + if (cpu_has_vmx_msr_bitmap())
> + vmx_set_msr_bitmap(vcpu);
> }
>
> static u32 vmx_exec_control(struct vcpu_vmx *vmx)
> @@ -6329,23 +6344,20 @@ static __init int hardware_setup(void)
>
> set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
>
> - if (enable_apicv) {
> - for (msr = 0x800; msr <= 0x8ff; msr++)
> - vmx_disable_intercept_msr_read_x2apic(msr);
> -
> - /* According SDM, in x2apic mode, the whole id reg is used.
> - * But in KVM, it only use the highest eight bits. Need to
> - * intercept it */
> - vmx_enable_intercept_msr_read_x2apic(0x802);
> - /* TMCCT */
> - vmx_enable_intercept_msr_read_x2apic(0x839);
> - /* TPR */
> - vmx_disable_intercept_msr_write_x2apic(0x808);
> - /* EOI */
> - vmx_disable_intercept_msr_write_x2apic(0x80b);
> - /* SELF-IPI */
> - vmx_disable_intercept_msr_write_x2apic(0x83f);
> - }
> + for (msr = 0x800; msr <= 0x8ff; msr++)
> + vmx_disable_intercept_msr_read_x2apic(msr);
> +
> + /* According SDM, in x2apic mode, the whole id reg is used. But in
> + * KVM, it only use the highest eight bits. Need to intercept it */
> + vmx_enable_intercept_msr_read_x2apic(0x802);
> + /* TMCCT */
> + vmx_enable_intercept_msr_read_x2apic(0x839);
> + /* TPR */
> + vmx_disable_intercept_msr_write_x2apic(0x808);
> + /* EOI */
> + vmx_disable_intercept_msr_write_x2apic(0x80b);
> + /* SELF-IPI */
> + vmx_disable_intercept_msr_write_x2apic(0x83f);
>
> if (enable_ept) {
> kvm_mmu_set_mask_ptes(0ull,
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
