Re: [PATCH] KVM: X86: Fix exception untrigger on ret to user

From: stsp <stsp2@yandex.ru>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	kvm@vger.kernel.org
Subject: Re: [PATCH] KVM: X86: Fix exception untrigger on ret to user
Date: Mon, 28 Jun 2021 12:12:41 +0300	[thread overview]
Message-ID: <b3ee97c8-318a-3134-07c7-75114e96b7cf@yandex.ru> (raw)
In-Reply-To: <87zgva3162.fsf@vitty.brq.redhat.com>

28.06.2021 10:20, Vitaly Kuznetsov пишет:
> Stas Sergeev <stsp2@yandex.ru> 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.

Unfortunately the problem happens
only on core2 CPU. I believe the reason
is perhaps that more modern CPUs do
not go to software for the exception
injection?

>   'exception.injected' can even be set through
> KVM_SET_VCPU_EVENTS and then we call KVM_SET_REGS.

Does this mean I shouldn't add
WARN_ON_ONCE()?

>   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?
Even if you have the right CPU
to reproduce that (Core2), you also
need the _TIF_SIGPENDING at the
right moment to provoke the cancel_injection
path. This is like triggering a race.
If you don't get _TIF_SIGPENDING
then it will just re-enter guest and
inject the exception properly.