Stas Sergeev <stsp2@xxxxxxxxx> writes:
> When returning to user, the special care is taken about the
> exception that was already injected to VMCS but not yet to guest.
> cancel_injection removes such exception from VMCS. It is set as
> pending, and if the user does KVM_SET_REGS, it gets completely canceled.
>
> This didn't happen though, because the vcpu->arch.exception.injected
> and vcpu->arch.exception.pending were forgotten to update in
> cancel_injection. As the result, KVM_SET_REGS didn't cancel out
> anything, and the exception was re-injected on the next KVM_RUN,
> even though the guest registers (like EIP) were already modified.
> This was leading to an exception coming from the "wrong place".
It shouldn't be that hard to reproduce this in selftests, I
believe. 'exception.injected' can even be set through
KVM_SET_VCPU_EVENTS and then we call KVM_SET_REGS. Alternatively, we can
trigger a real exception from the guest. Could you maybe add something
like this to tools/testing/selftests/kvm/x86_64/set_sregs_test.c?
>
> This patch makes sure the vcpu->arch.exception.injected and
> vcpu->arch.exception.pending are in sync with the reality (and
> with VMCS). Also it adds WARN_ON_ONCE() to __set_regs() to make
> sure vcpu->arch.exception.injected is never set here, because
> if it is, the exception context is going to be corrupted the same
> way it was before that patch.
> Adding WARN_ON_ONCE() alone, without the fix, was verified to
> actually trigger and detect a buggy scenario.
>
> Signed-off-by: Stas Sergeev <stsp2@xxxxxxxxx>
>
> CC: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> CC: Sean Christopherson <seanjc@xxxxxxxxxx>
> CC: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> CC: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> CC: Jim Mattson <jmattson@xxxxxxxxxx>
> CC: Joerg Roedel <joro@xxxxxxxxxx>
> CC: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> CC: Ingo Molnar <mingo@xxxxxxxxxx>
> CC: Borislav Petkov <bp@xxxxxxxxx>
> CC: x86@xxxxxxxxxx
> CC: "H. Peter Anvin" <hpa@xxxxxxxxx>
> CC: kvm@xxxxxxxxxxxxxxx
> ---
> arch/x86/kvm/x86.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e0f4a46649d7..bc6ca8641824 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9450,7 +9450,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> cancel_injection:
> if (req_immediate_exit)
> kvm_make_request(KVM_REQ_EVENT, vcpu);
> - static_call(kvm_x86_cancel_injection)(vcpu);
> + if (vcpu->arch.exception.injected) {
> + static_call(kvm_x86_cancel_injection)(vcpu);
> + vcpu->arch.exception.injected = false;
> + vcpu->arch.exception.pending = true;
> + }
> if (unlikely(vcpu->arch.apic_attention))
> kvm_lapic_sync_from_vapic(vcpu);
> out:
> @@ -9822,6 +9826,7 @@ static void __set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> kvm_rip_write(vcpu, regs->rip);
> kvm_set_rflags(vcpu, regs->rflags | X86_EFLAGS_FIXED);
>
> + WARN_ON_ONCE(vcpu->arch.exception.injected);
> vcpu->arch.exception.pending = false;
>
> kvm_make_request(KVM_REQ_EVENT, vcpu);
--
Vitaly
