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

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

[Stas Sergeev]

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".

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@yandex.ru>

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Sean Christopherson <seanjc@google.com>
CC: Vitaly Kuznetsov <vkuznets@redhat.com>
CC: Wanpeng Li <wanpengli@tencent.com>
CC: Jim Mattson <jmattson@google.com>
CC: Joerg Roedel <joro@8bytes.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: x86@kernel.org
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: kvm@vger.kernel.org
---
 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);
-- 
2.32.0

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

[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. '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@yandex.ru>
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Sean Christopherson <seanjc@google.com>
> CC: Vitaly Kuznetsov <vkuznets@redhat.com>
> CC: Wanpeng Li <wanpengli@tencent.com>
> CC: Jim Mattson <jmattson@google.com>
> CC: Joerg Roedel <joro@8bytes.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: x86@kernel.org
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: kvm@vger.kernel.org
> ---
>  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

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

[stsp]

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.

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

[Vitaly Kuznetsov]

stsp <stsp2@yandex.ru> writes:

> 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?

Hm, I've completely missed that from the original description. As I read
it, 'cancel_injection' path in vcpu_enter_guest() is always broken when
vcpu->arch.exception.injected is set as we forget to clear it...

>
>
>>   '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()?

WARN_ON_ONCE() is fine IMO in case there's no valid case when
'vcpu->arch.exception.injected' is set during __set_regs(). selftest is
needed to check for '... this was leading to an exception coming from
the "wrong place"'.

>
>
>>   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.

I'd like to understand the hardware dependency first. Is it possible
that the exception which causes the problem is not triggered on other
CPUs? We can find a different way to trigger an exception from selftest
then.

(Maybe it's just me who still struggles to see the full picure here,
hope Sean/Paolo will see the problem you're trying to address in no
time)

-- 
Vitaly

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

[stsp]

28.06.2021 13:07, Vitaly Kuznetsov пишет:
> stsp <stsp2@yandex.ru> writes:
>
>> 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?
> Hm, I've completely missed that from the original description. As I read
> it, 'cancel_injection' path in vcpu_enter_guest() is always broken when
> vcpu->arch.exception.injected is set as we forget to clear it...

Yes, cancel_injection is supposed to
be always broken indeed. But there
are a few more things to it.
Namely:
- Other CPUs do not seem to exhibit
that path. My guess here is that they
just handle the exception in hardware,
without returning to KVM for that. I
am not sure why Core2 vmexits per
each page fault. Is it incapable of
handling the PF in hardware, or maybe
some other bug is around?

- Even if you followed the broken
path, in most cases everything is still
fine: the exception will just be re-injected.
The unfortunate scenario is when you
have _TIF_SIGPENDING at exactly
right place. Then you go to user-space,
and the user-space is unlucky to use
SET_REGS right here. These conditions
are not very likely to happen. I wrote a
test-case for it, but it involves the entire
buildroot setup and you need to wait
a bit while it is trying to trigger the race.


>>>    '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()?
> WARN_ON_ONCE() is fine IMO in case there's no valid case when
> 'vcpu->arch.exception.injected' is set during __set_regs().

But you said:

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

... which makes such scenario valid?

  

>>
>>>    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.
> I'd like to understand the hardware dependency first. Is it possible
> that the exception which causes the problem is not triggered on other
> CPUs?

No, exception is triggered, but I
have never seen the race on any
other CPUs, and none of the people
who reported that problem to me,
have seen it on any other CPU.
I think other CPU just injects the PF
without doing any vmexit, but I've
no idea why Core2 does not do the
same thing. Should it?

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

[Vitaly Kuznetsov]

stsp <stsp2@yandex.ru> writes:

> 28.06.2021 13:07, Vitaly Kuznetsov пишет:
>> stsp <stsp2@yandex.ru> writes:
>>
>>> 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?
>> Hm, I've completely missed that from the original description. As I read
>> it, 'cancel_injection' path in vcpu_enter_guest() is always broken when
>> vcpu->arch.exception.injected is set as we forget to clear it...
>
> Yes, cancel_injection is supposed to
> be always broken indeed. But there
> are a few more things to it.
> Namely:
> - Other CPUs do not seem to exhibit
> that path. My guess here is that they
> just handle the exception in hardware,
> without returning to KVM for that. I
> am not sure why Core2 vmexits per
> each page fault. Is it incapable of
> handling the PF in hardware, or maybe
> some other bug is around?

Wild guess: no EPT support and running on shadow pages?

>
> - Even if you followed the broken
> path, in most cases everything is still
> fine: the exception will just be re-injected.
> The unfortunate scenario is when you
> have _TIF_SIGPENDING at exactly
> right place. Then you go to user-space,
> and the user-space is unlucky to use
> SET_REGS right here. These conditions
> are not very likely to happen. I wrote a
> test-case for it, but it involves the entire
> buildroot setup and you need to wait
> a bit while it is trying to trigger the race.

Maybe there's an easier way to trigger imminent exit to userspace which
doesn't involve 

>
>
>>>>    '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()?
>> WARN_ON_ONCE() is fine IMO in case there's no valid case when
>> 'vcpu->arch.exception.injected' is set during __set_regs().
>
> But you said:
>
>> 'exception.injected' can even be set through
>> KVM_SET_VCPU_EVENTS and then we call KVM_SET_REGS.
>
> ... which makes such scenario valid?
>

We should not add userspace-triggerable WARNs in kernel, right. I was
not sure if the WARN you add stays triggerable post-patch. 

>   
>
>>>
>>>>    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.
>> I'd like to understand the hardware dependency first. Is it possible
>> that the exception which causes the problem is not triggered on other
>> CPUs?
>
> No, exception is triggered, but I
> have never seen the race on any
> other CPUs, and none of the people
> who reported that problem to me,
> have seen it on any other CPU.
> I think other CPU just injects the PF
> without doing any vmexit, but I've
> no idea why Core2 does not do the
> same thing. Should it?

Maybe the huge amount of injected #PFs (which are triggered because
there's no EPT) contribute to the easiness of the reproduction? Purely
from from looking at the code of your patch, the issue should also
happen with other exceptions, KVM just doesn't inject them that
often. It doesn't mean that we can't craft something from selftests,
just need to understand the required conditions...

-- 
Vitaly

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

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> stsp <stsp2@yandex.ru> writes:
>
...

> I wrote a
>> test-case for it, but it involves the entire
>> buildroot setup and you need to wait
>> a bit while it is trying to trigger the race.
>
> Maybe there's an easier way to trigger imminent exit to userspace which
> doesn't involve 

[my test editor swallowed the ending of the phrase, sorry about that]

Maybe there's an easier way to trigger imminent exit to userspace which
doesn't involve signals.

-- 
Vitaly

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

[stsp]

28.06.2021 13:56, Vitaly Kuznetsov пишет:
> stsp <stsp2@yandex.ru> writes:
>
>> Yes, cancel_injection is supposed to
>> be always broken indeed. But there
>> are a few more things to it.
>> Namely:
>> - Other CPUs do not seem to exhibit
>> that path. My guess here is that they
>> just handle the exception in hardware,
>> without returning to KVM for that. I
>> am not sure why Core2 vmexits per
>> each page fault. Is it incapable of
>> handling the PF in hardware, or maybe
>> some other bug is around?
> Wild guess: no EPT support and running on shadow pages?

That's something you should tell
me, and not the other way around. :)
I am just working with kvm as a user.


>> - Even if you followed the broken
>> path, in most cases everything is still
>> fine: the exception will just be re-injected.
>> The unfortunate scenario is when you
>> have _TIF_SIGPENDING at exactly
>> right place. Then you go to user-space,
>> and the user-space is unlucky to use
>> SET_REGS right here. These conditions
>> are not very likely to happen. I wrote a
>> test-case for it, but it involves the entire
>> buildroot setup and you need to wait
>> a bit while it is trying to trigger the race.
> Maybe there's an easier way to trigger imminent exit to userspace which
> doesn't involve

Any API to intercept all guest exceptions?
But even if there is, I am afraid in that
case cancel_injection is not going to be
executed. It is executed only when
kvm_vcpu_exit_request(vcpu) returns true.


>> ... which makes such scenario valid?
>>
> We should not add userspace-triggerable WARNs in kernel, right. I was
> not sure if the WARN you add stays triggerable post-patch.

I thought its not - at least not when
the exceptions are coming from the
guest. Maybe WARN_ON() can somehow
check if the exception was injected by
user-space, like by checking the events
bitmask?
Or I'll just remove it.


> Maybe the huge amount of injected #PFs (which are triggered because
> there's no EPT) contribute to the easiness of the reproduction? Purely
> from from looking at the code of your patch, the issue should also
> happen with other exceptions, KVM just doesn't inject them that
> often.
I haven't seen the same race with
any other exception, like with GP.
I suspect there is no vmexit
for GP, so it will just be injected in
hardware. I think only PF makes
the problem because of the shadow
page tables, as you pointed before.
While I haven't made a specific
test-case to try GP, I am quite sure
it would have been observed long
ago because GPs in our usage
scenarios are much more frequent
than PFs. But the race was never
observed with GP.

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

[Jim Mattson]

On Sun, Jun 27, 2021 at 4:38 PM Stas Sergeev <stsp2@yandex.ru> wrote:
>
> 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".
>
> 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@yandex.ru>
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Sean Christopherson <seanjc@google.com>
> CC: Vitaly Kuznetsov <vkuznets@redhat.com>
> CC: Wanpeng Li <wanpengli@tencent.com>
> CC: Jim Mattson <jmattson@google.com>
> CC: Joerg Roedel <joro@8bytes.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: x86@kernel.org
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: kvm@vger.kernel.org
> ---
>  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);
> --
> 2.32.0
>

This doesn't work. Kvm has no facilities for converting an injected
exception back into a pending exception. In particular, if the
exception has side effects, such as a #PF which sets CR2, those side
effects have already taken place. Once kvm sets the VM-entry
interruption-information field, the next VM-entry must deliver that
exception. You could arrange to back it out, but you would also have
to back out the changes to CR2 (for #PF) or DR6 (for #DB).

Cancel_injection *should* leave the exception in the 'injected' state,
and KVM_SET_REGS *should not* clear an injected exception. (I don't
think it's right to clear a pending exception either, if that
exception happens to be a trap, but that's a different discussion).

It seems to me that the crux of the problem here is that
run->ready_for_interrupt_injection returns true when it should return
false. That's probably where you should focus your efforts.

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

[Jim Mattson]

On Mon, Jun 28, 2021 at 4:27 AM stsp <stsp2@yandex.ru> wrote:
>
> 28.06.2021 13:56, Vitaly Kuznetsov пишет:
> > stsp <stsp2@yandex.ru> writes:
> >
> >> Yes, cancel_injection is supposed to
> >> be always broken indeed. But there
> >> are a few more things to it.
> >> Namely:
> >> - Other CPUs do not seem to exhibit
> >> that path. My guess here is that they
> >> just handle the exception in hardware,
> >> without returning to KVM for that. I
> >> am not sure why Core2 vmexits per
> >> each page fault. Is it incapable of
> >> handling the PF in hardware, or maybe
> >> some other bug is around?
> > Wild guess: no EPT support and running on shadow pages?
>
> That's something you should tell
> me, and not the other way around. :)
> I am just working with kvm as a user.
>
Yes, with shadow paging, kvm intercepts all guest page faults. You
should be able to replicate this behavior on modern CPUs by adding
"ept=N" to the kvm_intel module parameters.

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

[stsp]

28.06.2021 19:39, Jim Mattson пишет:
> Yes, with shadow paging, kvm intercepts all guest page faults. You
> should be able to replicate this behavior on modern CPUs by adding
> "ept=N" to the kvm_intel module parameters.

Yes, that works as a reproducer
on a more modern CPUs.
Thanks!

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

[stsp]

28.06.2021 19:19, Jim Mattson пишет:
> This doesn't work. Kvm has no facilities for converting an injected
> exception back into a pending exception.

What is the purpose of the
cancel_injection then?


>   In particular, if the
> exception has side effects, such as a #PF which sets CR2, those side
> effects have already taken place. Once kvm sets the VM-entry
> interruption-information field, the next VM-entry must deliver that
> exception. You could arrange to back it out, but you would also have
> to back out the changes to CR2 (for #PF) or DR6 (for #DB).
>
> Cancel_injection *should* leave the exception in the 'injected' state,

But it removes it from VMCS, no?
I thought "injected=true" means
"injected to VMCS". What is the
difference between "injected" and
"pending" if both may or may not
mean "already in VMCS"?


> and KVM_SET_REGS *should not* clear an injected exception. (I don't
> think it's right to clear a pending exception either, if that
> exception happens to be a trap, but that's a different discussion).
>
> It seems to me that the crux of the problem here is that
> run->ready_for_interrupt_injection returns true when it should return
> false. That's probably where you should focus your efforts.

I tried that already, and showed
the results to you. :) Alas, you didn't
reply to those.
But why do you suggest the cpu-specific
approach? All other CPUs exit to user-space
only when the exception is _really_ injected,
i.e. CS/EIP points to the IDT handler.
I don't see why it should be non-atomic
just for one CPU. Shouldn't that be atomic
for all CPUs?

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

[Jim Mattson]

On Mon, Jun 28, 2021 at 10:06 AM stsp <stsp2@yandex.ru> wrote:
>
> 28.06.2021 19:19, Jim Mattson пишет:
> > This doesn't work. Kvm has no facilities for converting an injected
> > exception back into a pending exception.
>
> What is the purpose of the
> cancel_injection then?

I believe cancel_injection exists for serializing the vCPU state. If
the vCPU is saved and restored, then you don't want to lose the
'injected' event that is sitting in the VMCS.

>
> >   In particular, if the
> > exception has side effects, such as a #PF which sets CR2, those side
> > effects have already taken place. Once kvm sets the VM-entry
> > interruption-information field, the next VM-entry must deliver that
> > exception. You could arrange to back it out, but you would also have
> > to back out the changes to CR2 (for #PF) or DR6 (for #DB).
> >
> > Cancel_injection *should* leave the exception in the 'injected' state,
>
> But it removes it from VMCS, no?
> I thought "injected=true" means
> "injected to VMCS". What is the
> difference between "injected" and
> "pending" if both may or may not
> mean "already in VMCS"?

Pending events have not yet been subject to interception by L1 (if L2
is active). Their side effects have not yet been applied. When a
pending event is processed, if L2 is active, kvm checks L1's exception
bitmap to see if the event should cause an emulated VM-exit from L2 to
L1. If not, the pending event transitions to an injected event and the
side effects are applied. (At this point, the event is essentially
committed.) The event is then written to the VM-entry
interruption-information field to take advantage of hardware
assistance for vectoring through the IDT.

Perhaps 'committed' would have been a better term than 'injected.'

> > and KVM_SET_REGS *should not* clear an injected exception. (I don't
> > think it's right to clear a pending exception either, if that
> > exception happens to be a trap, but that's a different discussion).
> >
> > It seems to me that the crux of the problem here is that
> > run->ready_for_interrupt_injection returns true when it should return
> > false. That's probably where you should focus your efforts.
>
> I tried that already, and showed
> the results to you. :) Alas, you didn't
> reply to those.

I haven't had the time. I was hoping that someone else on the kvm list
would help you.

> But why do you suggest the cpu-specific
> approach? All other CPUs exit to user-space
> only when the exception is _really_ injected,
> i.e. CS/EIP points to the IDT handler.
> I don't see why it should be non-atomic
> just for one CPU. Shouldn't that be atomic
> for all CPUs?

This isn't CPU-specific. Even when using EPT, you can potentially end
up in this state after an EPT violation during IDT vectoring.

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

[stsp]

28.06.2021 19:19, Jim Mattson пишет:
> It seems to me that the crux of the problem here is that
> run->ready_for_interrupt_injection returns true when it should return
> false. That's probably where you should focus your efforts.

OK, it was occasionally found that
it actually worked that way in the
past. This patch:
https://www.lkml.org/lkml/2020/12/1/324
from Paolo removes the
!kvm_event_needs_reinjection(vcpu)
check.

Paolo, maybe you can comment?


> This isn't CPU-specific. Even when using EPT, you can potentially end
> up in this state after an EPT violation during IDT vectoring.

Well, in that case you will at
least return the proper status
about the EPT violation.
But for EINTR this is definitely
going to be CPU-specific.
And a rather nasty one: running
a ring3 guest with CPL=0 and IF
always set, and having to check
for ready_to_injection upon EINTR
on just one CPU, is very unexpected.

So I won't be claiming that Paolo's
patch is incorrect. Maybe someone
can think of the way to just not
get such scenario on EINTR?

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

[Jim Mattson]

On Mon, Jun 28, 2021 at 3:23 PM stsp <stsp2@yandex.ru> wrote:
>
> 28.06.2021 19:19, Jim Mattson пишет:
> > It seems to me that the crux of the problem here is that
> > run->ready_for_interrupt_injection returns true when it should return
> > false. That's probably where you should focus your efforts.
>
> OK, it was occasionally found that
> it actually worked that way in the
> past. This patch:
> https://www.lkml.org/lkml/2020/12/1/324
> from Paolo removes the
> !kvm_event_needs_reinjection(vcpu)
> check.
>
> Paolo, maybe you can comment?
>
>
> > This isn't CPU-specific. Even when using EPT, you can potentially end
> > up in this state after an EPT violation during IDT vectoring.
>
> Well, in that case you will at
> least return the proper status
> about the EPT violation.
> But for EINTR this is definitely
> going to be CPU-specific.

No; it's not. After fixing up the EPT violation, when the interrupted
IDT vectoring operation is reinjected via the VM-entry
interruption-information field, you can still get kicked out of the
vcpu run loop by an alarm, and you're in exactly the same boat.

> And a rather nasty one: running
> a ring3 guest with CPL=0 and IF
> always set, and having to check
> for ready_to_injection upon EINTR
> on just one CPU, is very unexpected.

You should *always* be checking ready_for_interrupt_injection before
injecting an interrupt, regardless of CPU. That's the API.

> So I won't be claiming that Paolo's
> patch is incorrect. Maybe someone
> can think of the way to just not
> get such scenario on EINTR?

How long are you willing to defer the EINTR? What if you're running at
the target of a live migration, and the guest's IDT page needs to be
demand-fetched over the network? It may be quite some time before you
can actually deliver that exception.

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

[stsp]

29.06.2021 01:35, Jim Mattson пишет:
> How long are you willing to defer the EINTR? What if you're running at
> the target of a live migration, and the guest's IDT page needs to be
> demand-fetched over the network? It may be quite some time before you
> can actually deliver that exception.

OK, understood, thank you.
I wonder how many other non-atomic
things are there in KVM, but probably
that doesn't matter much, if
run->ready_for_injection covers them
all at once...

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

[Stas Sergeev]

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".

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 clearing of pending exception to
__set_sregs() the same way it is in __set_regs(). See patch
b4f14abd9 that added it to __set_regs().

How to trigger the buggy scenario (that is, without this patch):
- Make sure you have the old CPU where shadow page tables
are used. Core2 family should be fine for the task. In this
case, all PF exceptions produce the exit to monitor.
- You need the _TIF_SIGPENDING flag set at the right moment
to get kvm_vcpu_exit_request() to return true when the PF
exception was just injected. In that case the cancel_injection
path is executed.
- You need the "unlucky" user-space that executes KVM_SET_REGS
at the right moment. This leads to KVM_SET_REGS not clearing
the exception, but instead corrupting its context.

v2 changes:
- do not add WARN_ON_ONCE() to __set_regs(). As explained by
Vitaly Kuznetsov, it can be user-triggerable.
- clear pending exception also in __set_sregs().
- update description with the bug-triggering scenario.

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Sean Christopherson <seanjc@google.com>
CC: Vitaly Kuznetsov <vkuznets@redhat.com>
CC: Jim Mattson <jmattson@google.com>
CC: Joerg Roedel <joro@8bytes.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: Jan Kiszka <jan.kiszka@siemens.com>
CC: x86@kernel.org
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: kvm@vger.kernel.org
---
 arch/x86/kvm/x86.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e0f4a46649d7..d1026e9216e4 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:
@@ -10077,6 +10081,8 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
 		pr_debug("Set back pending irq %d\n", pending_vec);
 	}
 
+	vcpu->arch.exception.pending = false;
+
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 
 	ret = 0;
-- 
2.32.0