[PATCH 0/2] KVM: fix set_memory_region_test on AMD

[PATCH 0/2] KVM: fix set_memory_region_test on AMD

[Paolo Bonzini]

The set_memory_region_test selftest is failing on AMD machines, this
series fixes two bugs in it.

Paolo Bonzini (2):
  KVM: SVM: avoid infinite loop on NPF from bad address
  selftests: kvm/set_memory_region_test: do not check RIP if the guest
    shuts down

 arch/x86/kvm/svm/svm.c                              |  7 +++++++
 .../testing/selftests/kvm/set_memory_region_test.c  | 13 +++++++++----
 virt/kvm/kvm_main.c                                 |  1 +
 3 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.18.2

[PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Paolo Bonzini]

When a nested page fault is taken from an address that does not have
a memslot associated to it, kvm_mmu_do_page_fault returns RET_PF_EMULATE
(via mmu_set_spte) and kvm_mmu_page_fault then invokes svm_need_emulation_on_page_fault.

The default answer there is to return false, but in this case this just
causes the page fault to be retried ad libitum.  Since this is not a
fast path, and the only other case where it is taken is an erratum,
just stick a kvm_vcpu_gfn_to_memslot check in there to detect the
common case where the erratum is not happening.

This fixes an infinite loop in the new set_memory_region_test.

Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)")
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/svm.c | 7 +++++++
 virt/kvm/kvm_main.c    | 1 +
 2 files changed, 8 insertions(+)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a91e397d6750..c86f7278509b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3837,6 +3837,13 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu)
 	bool smap = cr4 & X86_CR4_SMAP;
 	bool is_user = svm_get_cpl(vcpu) == 3;
 
+	/*
+	 * If RIP is invalid, go ahead with emulation which will cause an
+	 * internal error exit.
+	 */
+	if (!kvm_vcpu_gfn_to_memslot(vcpu, kvm_rip_read(vcpu) >> PAGE_SHIFT))
+		return true;
+
 	/*
 	 * Detect and workaround Errata 1096 Fam_17h_00_0Fh.
 	 *
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e2f60e313c87..e7436d054305 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1602,6 +1602,7 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn
 {
 	return __gfn_to_memslot(kvm_vcpu_memslots(vcpu), gfn);
 }
+EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_memslot);
 
 bool kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn)
 {
-- 
2.18.2

[PATCH 2/2] selftests: kvm/set_memory_region_test: do not check RIP if the guest shuts down

[Paolo Bonzini]

On AMD, the state of the VMCB is undefined after a shutdown VMEXIT.  KVM
takes a very conservative approach to that and resets the guest altogether
when that happens.  This causes the set_memory_region_test to fail
because the RIP is 0xfff0 (the reset vector).  Restrict the RIP test
to KVM_EXIT_INTERNAL_ERROR in order to fix this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .../testing/selftests/kvm/set_memory_region_test.c  | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index 260e638826dc..b3ece55a2da6 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -287,10 +287,15 @@ static void test_delete_memory_region(void)
 
 	vcpu_regs_get(vm, VCPU_ID, &regs);
 
-	TEST_ASSERT(regs.rip >= final_rip_start &&
-		    regs.rip < final_rip_end,
-		    "Bad rip, expected 0x%lx - 0x%lx, got 0x%llx\n",
-		    final_rip_start, final_rip_end, regs.rip);
+	/*
+	 * On AMD, after KVM_EXIT_SHUTDOWN the VMCB has been reinitialized already,
+	 * so the instruction pointer would point to the reset vector.
+	 */
+	if (run->exit_reason == KVM_EXIT_INTERNAL_ERROR)
+		TEST_ASSERT(regs.rip >= final_rip_start &&
+			    regs.rip < final_rip_end,
+			    "Bad rip, expected 0x%lx - 0x%lx, got 0x%llx\n",
+			    final_rip_start, final_rip_end, regs.rip);
 
 	kvm_vm_free(vm);
 }
-- 
2.18.2

Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)").

The bot has tested the following trees: v5.6.5, v5.5.18, v5.4.33.

v5.6.5: Build OK!
v5.5.18: Failed to apply! Possible dependencies:
    Unable to calculate

v5.4.33: Failed to apply! Possible dependencies:
    Unable to calculate


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Wanpeng Li]

On Sat, 18 Apr 2020 at 00:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> When a nested page fault is taken from an address that does not have
> a memslot associated to it, kvm_mmu_do_page_fault returns RET_PF_EMULATE
> (via mmu_set_spte) and kvm_mmu_page_fault then invokes svm_need_emulation_on_page_fault.
>
> The default answer there is to return false, but in this case this just
> causes the page fault to be retried ad libitum.  Since this is not a
> fast path, and the only other case where it is taken is an erratum,
> just stick a kvm_vcpu_gfn_to_memslot check in there to detect the
> common case where the erratum is not happening.
>
> This fixes an infinite loop in the new set_memory_region_test.
>
> Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/svm.c | 7 +++++++
>  virt/kvm/kvm_main.c    | 1 +
>  2 files changed, 8 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index a91e397d6750..c86f7278509b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3837,6 +3837,13 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu)
>         bool smap = cr4 & X86_CR4_SMAP;
>         bool is_user = svm_get_cpl(vcpu) == 3;
>
> +       /*
> +        * If RIP is invalid, go ahead with emulation which will cause an
> +        * internal error exit.
> +        */
> +       if (!kvm_vcpu_gfn_to_memslot(vcpu, kvm_rip_read(vcpu) >> PAGE_SHIFT))
> +               return true;
> +
>         /*
>          * Detect and workaround Errata 1096 Fam_17h_00_0Fh.
>          *
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e2f60e313c87..e7436d054305 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1602,6 +1602,7 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn
>  {
>         return __gfn_to_memslot(kvm_vcpu_memslots(vcpu), gfn);
>  }
> +EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_memslot);

This commit incurs the linux guest fails to boot once add --overcommit
cpu-pm=on or not intercept hlt instruction, any thoughts?

<...>-35787 [038] ....  2825.959082: kvm_exit: vcpu 1 reason npf rip
0xfd11d info 100000014 fd000
<...>-35788 [037] ....  2825.959082: kvm_exit: vcpu 2 reason npf rip
0xfd11d info 100000014 fd000
<...>-35789 [036] ....  2825.959082: kvm_exit: vcpu 3 reason npf rip
0xfd11d info 100000014 fd000
<...>-35788 [037] ....  2825.959082: kvm_page_fault: address fd000 error_code 14
<...>-35789 [036] ....  2825.959082: kvm_page_fault: address fd000 error_code 14
<...>-35787 [038] ....  2825.959083: kvm_page_fault: address fd000 error_code 14
<...>-35788 [037] ....  2825.959086: kvm_emulate_insn: 0:fd11d: (prot32)
<...>-35788 [037] ....  2825.959086: kvm_emulate_insn: 0:fd11d: (prot32) failed
<...>-35789 [036] ....  2825.959087: kvm_emulate_insn: 0:fd11d: (prot32)
<...>-35789 [036] ....  2825.959087: kvm_emulate_insn: 0:fd11d: (prot32) failed
<...>-35788 [037] ....  2825.959087: kvm_fpu: unload
<...>-35787 [038] ....  2825.959087: kvm_emulate_insn: 0:fd11d: (prot32)
<...>-35787 [038] ....  2825.959087: kvm_emulate_insn: 0:fd11d: (prot32) failed
<...>-35789 [036] ....  2825.959087: kvm_fpu: unload
<...>-35787 [038] ....  2825.959088: kvm_fpu: unload
<...>-35788 [037] ....  2825.959088: kvm_userspace_exit: reason
KVM_EXIT_INTERNAL_ERROR (17)
<...>-35789 [036] ....  2825.959089: kvm_userspace_exit: reason
KVM_EXIT_INTERNAL_ERROR (17)
<...>-35787 [038] ....  2825.959089: kvm_userspace_exit: reason
KVM_EXIT_INTERNAL_ERROR (17)

Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Paolo Bonzini]

On 08/07/20 10:17, Wanpeng Li wrote:
> On Sat, 18 Apr 2020 at 00:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> When a nested page fault is taken from an address that does not have
>> a memslot associated to it, kvm_mmu_do_page_fault returns RET_PF_EMULATE
>> (via mmu_set_spte) and kvm_mmu_page_fault then invokes svm_need_emulation_on_page_fault.
>>
>> The default answer there is to return false, but in this case this just
>> causes the page fault to be retried ad libitum.  Since this is not a
>> fast path, and the only other case where it is taken is an erratum,
>> just stick a kvm_vcpu_gfn_to_memslot check in there to detect the
>> common case where the erratum is not happening.
>>
>> This fixes an infinite loop in the new set_memory_region_test.
>>
>> Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  arch/x86/kvm/svm/svm.c | 7 +++++++
>>  virt/kvm/kvm_main.c    | 1 +
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index a91e397d6750..c86f7278509b 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3837,6 +3837,13 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu)
>>         bool smap = cr4 & X86_CR4_SMAP;
>>         bool is_user = svm_get_cpl(vcpu) == 3;
>>
>> +       /*
>> +        * If RIP is invalid, go ahead with emulation which will cause an
>> +        * internal error exit.
>> +        */
>> +       if (!kvm_vcpu_gfn_to_memslot(vcpu, kvm_rip_read(vcpu) >> PAGE_SHIFT))
>> +               return true;
>> +
>>         /*
>>          * Detect and workaround Errata 1096 Fam_17h_00_0Fh.
>>          *
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index e2f60e313c87..e7436d054305 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -1602,6 +1602,7 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn
>>  {
>>         return __gfn_to_memslot(kvm_vcpu_memslots(vcpu), gfn);
>>  }
>> +EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_memslot);
> 
> This commit incurs the linux guest fails to boot once add --overcommit
> cpu-pm=on or not intercept hlt instruction, any thoughts?

Can you write a selftest?

Paolo

Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Wanpeng Li]

On Wed, 8 Jul 2020 at 16:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 08/07/20 10:17, Wanpeng Li wrote:
> > On Sat, 18 Apr 2020 at 00:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> When a nested page fault is taken from an address that does not have
> >> a memslot associated to it, kvm_mmu_do_page_fault returns RET_PF_EMULATE
> >> (via mmu_set_spte) and kvm_mmu_page_fault then invokes svm_need_emulation_on_page_fault.
> >>
> >> The default answer there is to return false, but in this case this just
> >> causes the page fault to be retried ad libitum.  Since this is not a
> >> fast path, and the only other case where it is taken is an erratum,
> >> just stick a kvm_vcpu_gfn_to_memslot check in there to detect the
> >> common case where the erratum is not happening.
> >>
> >> This fixes an infinite loop in the new set_memory_region_test.
> >>
> >> Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  arch/x86/kvm/svm/svm.c | 7 +++++++
> >>  virt/kvm/kvm_main.c    | 1 +
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> >> index a91e397d6750..c86f7278509b 100644
> >> --- a/arch/x86/kvm/svm/svm.c
> >> +++ b/arch/x86/kvm/svm/svm.c
> >> @@ -3837,6 +3837,13 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu)
> >>         bool smap = cr4 & X86_CR4_SMAP;
> >>         bool is_user = svm_get_cpl(vcpu) == 3;
> >>
> >> +       /*
> >> +        * If RIP is invalid, go ahead with emulation which will cause an
> >> +        * internal error exit.
> >> +        */
> >> +       if (!kvm_vcpu_gfn_to_memslot(vcpu, kvm_rip_read(vcpu) >> PAGE_SHIFT))
> >> +               return true;
> >> +
> >>         /*
> >>          * Detect and workaround Errata 1096 Fam_17h_00_0Fh.
> >>          *
> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> index e2f60e313c87..e7436d054305 100644
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -1602,6 +1602,7 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn
> >>  {
> >>         return __gfn_to_memslot(kvm_vcpu_memslots(vcpu), gfn);
> >>  }
> >> +EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_memslot);
> >
> > This commit incurs the linux guest fails to boot once add --overcommit
> > cpu-pm=on or not intercept hlt instruction, any thoughts?
>
> Can you write a selftest?

Actually I don't know what's happening here(why not intercept hlt
instruction has associated with this commit), otherwise, it has
already been fixed. :)

    Wanpeng

Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Paolo Bonzini]

On 08/07/20 11:08, Wanpeng Li wrote:
>>>> +EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_memslot);
>>> This commit incurs the linux guest fails to boot once add --overcommit
>>> cpu-pm=on or not intercept hlt instruction, any thoughts?
>> Can you write a selftest?
> Actually I don't know what's happening here(why not intercept hlt
> instruction has associated with this commit), otherwise, it has
> already been fixed. :)

I don't understand, what has been fixed and where?

Paolo

Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Salvatore Bonaccorso]

Hi Paolo,

On Fri, Apr 17, 2020 at 12:38:42PM -0400, Paolo Bonzini wrote:
> When a nested page fault is taken from an address that does not have
> a memslot associated to it, kvm_mmu_do_page_fault returns RET_PF_EMULATE
> (via mmu_set_spte) and kvm_mmu_page_fault then invokes svm_need_emulation_on_page_fault.
> 
> The default answer there is to return false, but in this case this just
> causes the page fault to be retried ad libitum.  Since this is not a
> fast path, and the only other case where it is taken is an erratum,
> just stick a kvm_vcpu_gfn_to_memslot check in there to detect the
> common case where the erratum is not happening.
> 
> This fixes an infinite loop in the new set_memory_region_test.
> 
> Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/svm.c | 7 +++++++
>  virt/kvm/kvm_main.c    | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index a91e397d6750..c86f7278509b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3837,6 +3837,13 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu)
>  	bool smap = cr4 & X86_CR4_SMAP;
>  	bool is_user = svm_get_cpl(vcpu) == 3;
>  
> +	/*
> +	 * If RIP is invalid, go ahead with emulation which will cause an
> +	 * internal error exit.
> +	 */
> +	if (!kvm_vcpu_gfn_to_memslot(vcpu, kvm_rip_read(vcpu) >> PAGE_SHIFT))
> +		return true;
> +
>  	/*
>  	 * Detect and workaround Errata 1096 Fam_17h_00_0Fh.
>  	 *
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e2f60e313c87..e7436d054305 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1602,6 +1602,7 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn
>  {
>  	return __gfn_to_memslot(kvm_vcpu_memslots(vcpu), gfn);
>  }
> +EXPORT_SYMBOL_GPL(kvm_vcpu_gfn_to_memslot);
>  
>  bool kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn)
>  {
> -- 
> 2.18.2

I noticed that this patch, whilst beeing CC'ed for stable, appers to
not have been applied to stable branches back then. There was first a
mail from Sasha's bot that patches do not apply and Wanpeng Li.

Did this simply felt through the cracks here or is it not worth
backporting to older series? At least
https://bugzilla.redhat.com/show_bug.cgi?id=1947982#c3 seem to
indicate it might not be worth of if there is risk for regression if I
understand Wanpeng Li. Is this right?

Regards,
Salvatore

Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Paolo Bonzini]

On 08/06/21 06:39, Salvatore Bonaccorso wrote:
> 
> Did this simply felt through the cracks here or is it not worth
> backporting to older series? At least
> https://bugzilla.redhat.com/show_bug.cgi?id=1947982#c3  seem to
> indicate it might not be worth of if there is risk for regression if I
> understand Wanpeng Li. Is this right?

It's not particularly interesting, because the loop can be broken with 
just Ctrl-C (or any signal for that matter) and the guest was 
misbehaving anyway.  You can read from that bugzilla link my opinion on 
this "vulnerability": if you run a VM for somebody and they want to 
waste your CPU time, they can just run a while(1) loop.

It's a bug and it is caught by the kvm-unit-tests, so I marked it for 
stable at the time because it can be useful to run kvm-unit-tests on 
stable kernels and hanging is a bit impolite (the test harness has a 
timeout, but of course tests that hang have the risk missing other 
regressions).

I will review gladly a backport, but if it is just because of that CVE 
report, documenting that the vulnerability is bogus would be time spent 
better that doing and testing the backport.

Paolo

Query about calling kvm_vcpu_gfn_to_memslot() with a GVA (Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Liam Merwick]

On Fri, Apr 17, 2020 at 12:38:42PM -0400, Paolo Bonzini wrote:
 > When a nested page fault is taken from an address that does not have
 > a memslot associated to it, kvm_mmu_do_page_fault returns RET_PF_EMULATE
 > (via mmu_set_spte) and kvm_mmu_page_fault then invokes 
svm_need_emulation_on_page_fault.
 >
 > The default answer there is to return false, but in this case this just
 > causes the page fault to be retried ad libitum.  Since this is not a
 > fast path, and the only other case where it is taken is an erratum,
 > just stick a kvm_vcpu_gfn_to_memslot check in there to detect the
 > common case where the erratum is not happening.
 >
 > This fixes an infinite loop in the new set_memory_region_test.
 >
 > Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len 
maybe zero on SMAP violation)")
 > Cc: stable@vger.kernel.org
 > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
 > ---
 >  arch/x86/kvm/svm/svm.c | 7 +++++++
 >  virt/kvm/kvm_main.c    | 1 +
 >  2 files changed, 8 insertions(+)
 >
 > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
 > index a91e397d6750..c86f7278509b 100644
 > --- a/arch/x86/kvm/svm/svm.c
 > +++ b/arch/x86/kvm/svm/svm.c
 > @@ -3837,6 +3837,13 @@ static bool 
svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu)
 >  	bool smap = cr4 & X86_CR4_SMAP;
 >  	bool is_user = svm_get_cpl(vcpu) == 3;
 >
 > +	/*
 > +	 * If RIP is invalid, go ahead with emulation which will cause an
 > +	 * internal error exit.
 > +	 */
 > +	if (!kvm_vcpu_gfn_to_memslot(vcpu, kvm_rip_read(vcpu) >> PAGE_SHIFT))

When looking into an SEV issue it was noted that the second arg to
kvm_vcpu_gfn_to_memslot() is a gfn_t but kvm_rip_read() will return 
guest RIP which is a guest virtual address and memslots hold guest 
physical addresses. How is KVM supposed to translate it to a memslot
and indicate if the guest RIP is valid?

Regards,
Liam

Re: Query about calling kvm_vcpu_gfn_to_memslot() with a GVA (Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Sean Christopherson]

On Thu, Jan 13, 2022, Liam Merwick wrote:
> On Fri, Apr 17, 2020 at 12:38:42PM -0400, Paolo Bonzini wrote:
> > When a nested page fault is taken from an address that does not have
> > a memslot associated to it, kvm_mmu_do_page_fault returns RET_PF_EMULATE
> > (via mmu_set_spte) and kvm_mmu_page_fault then invokes
> svm_need_emulation_on_page_fault.
> >
> > The default answer there is to return false, but in this case this just
> > causes the page fault to be retried ad libitum.  Since this is not a
> > fast path, and the only other case where it is taken is an erratum,
> > just stick a kvm_vcpu_gfn_to_memslot check in there to detect the
> > common case where the erratum is not happening.
> >
> > This fixes an infinite loop in the new set_memory_region_test.
> >
> > Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe
> zero on SMAP violation)")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  arch/x86/kvm/svm/svm.c | 7 +++++++
> >  virt/kvm/kvm_main.c    | 1 +
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index a91e397d6750..c86f7278509b 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -3837,6 +3837,13 @@ static bool svm_need_emulation_on_page_fault(struct
> kvm_vcpu *vcpu)
> >  	bool smap = cr4 & X86_CR4_SMAP;
> >  	bool is_user = svm_get_cpl(vcpu) == 3;
> >
> > +	/*
> > +	 * If RIP is invalid, go ahead with emulation which will cause an
> > +	 * internal error exit.
> > +	 */
> > +	if (!kvm_vcpu_gfn_to_memslot(vcpu, kvm_rip_read(vcpu) >> PAGE_SHIFT))
> 
> When looking into an SEV issue it was noted that the second arg to
> kvm_vcpu_gfn_to_memslot() is a gfn_t but kvm_rip_read() will return guest
> RIP which is a guest virtual address and memslots hold guest physical
> addresses. How is KVM supposed to translate it to a memslot
> and indicate if the guest RIP is valid?

Ugh, magic?  That code is complete garbage.  It worked to fix the selftest issue
because the selftest identity maps the relevant guest code.

The entire idea is a hack.  If KVM gets into an infinite loop because the guest
is attempting to fetch from MMIO, then the #NPF/#PF should have the FETCH bit set
in the error code.  I.e. I believe the below change should fix the original issue,
at which point we can revert the above.  I'll test today and hopefully get a patch
sent out.

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c3d9006478a4..e1d2a46e06bf 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1995,6 +1995,17 @@ static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
        vmcb_mark_dirty(svm->vmcb, VMCB_DR);
 }

+static char *svm_get_pf_insn_bytes(struct vcpu_svm *svm)
+{
+       if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
+               return NULL;
+
+       if (svm->vmcb->control.exit_info_1 & PFERR_FETCH_MASK)
+               return NULL;
+
+       return svm->vmcb->control.insn_bytes;
+}
+
 static int pf_interception(struct kvm_vcpu *vcpu)
 {
        struct vcpu_svm *svm = to_svm(vcpu);
@@ -2003,9 +2014,8 @@ static int pf_interception(struct kvm_vcpu *vcpu)
        u64 error_code = svm->vmcb->control.exit_info_1;

        return kvm_handle_page_fault(vcpu, error_code, fault_address,
-                       static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
-                       svm->vmcb->control.insn_bytes : NULL,
-                       svm->vmcb->control.insn_len);
+                                    svm_get_pf_insn_bytes(svm),
+                                    svm->vmcb->control.insn_len);
 }

 static int npf_interception(struct kvm_vcpu *vcpu)
@@ -2017,9 +2027,8 @@ static int npf_interception(struct kvm_vcpu *vcpu)

        trace_kvm_page_fault(fault_address, error_code);
        return kvm_mmu_page_fault(vcpu, fault_address, error_code,
-                       static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
-                       svm->vmcb->control.insn_bytes : NULL,
-                       svm->vmcb->control.insn_len);
+                                 svm_get_pf_insn_bytes(svm),
+                                 svm->vmcb->control.insn_len);
 }

 static int db_interception(struct kvm_vcpu *vcpu)

Re: Query about calling kvm_vcpu_gfn_to_memslot() with a GVA (Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Liam Merwick]

On 13/01/2022 16:57, Sean Christopherson wrote:
> On Thu, Jan 13, 2022, Liam Merwick wrote:
>> On Fri, Apr 17, 2020 at 12:38:42PM -0400, Paolo Bonzini wrote:
>>> When a nested page fault is taken from an address that does not have
>>> a memslot associated to it, kvm_mmu_do_page_fault returns RET_PF_EMULATE
>>> (via mmu_set_spte) and kvm_mmu_page_fault then invokes
>> svm_need_emulation_on_page_fault.
>>>
>>> The default answer there is to return false, but in this case this just
>>> causes the page fault to be retried ad libitum.  Since this is not a
>>> fast path, and the only other case where it is taken is an erratum,
>>> just stick a kvm_vcpu_gfn_to_memslot check in there to detect the
>>> common case where the erratum is not happening.
>>>
>>> This fixes an infinite loop in the new set_memory_region_test.
>>>
>>> Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe
>> zero on SMAP violation)")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   arch/x86/kvm/svm/svm.c | 7 +++++++
>>>   virt/kvm/kvm_main.c    | 1 +
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index a91e397d6750..c86f7278509b 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -3837,6 +3837,13 @@ static bool svm_need_emulation_on_page_fault(struct
>> kvm_vcpu *vcpu)
>>>   	bool smap = cr4 & X86_CR4_SMAP;
>>>   	bool is_user = svm_get_cpl(vcpu) == 3;
>>>
>>> +	/*
>>> +	 * If RIP is invalid, go ahead with emulation which will cause an
>>> +	 * internal error exit.
>>> +	 */
>>> +	if (!kvm_vcpu_gfn_to_memslot(vcpu, kvm_rip_read(vcpu) >> PAGE_SHIFT))
>>
>> When looking into an SEV issue it was noted that the second arg to
>> kvm_vcpu_gfn_to_memslot() is a gfn_t but kvm_rip_read() will return guest
>> RIP which is a guest virtual address and memslots hold guest physical
>> addresses. How is KVM supposed to translate it to a memslot
>> and indicate if the guest RIP is valid?
> 
> Ugh, magic?  That code is complete garbage.  It worked to fix the selftest issue
> because the selftest identity maps the relevant guest code.
> 
> The entire idea is a hack.  If KVM gets into an infinite loop because the guest
> is attempting to fetch from MMIO, then the #NPF/#PF should have the FETCH bit set
> in the error code.  I.e. I believe the below change should fix the original issue,
> at which point we can revert the above.  I'll test today and hopefully get a patch
> sent out.

Thanks Sean.

I have been running with this patch along with reverting commit
e72436bc3a52 ("KVM: SVM: avoid infinite loop on NPF from bad address")
with over 150 hours runtime on multiple machines and it resolves an SEV
guest crash I was encountering where if there were no decode assist 
bytes available, it then continued on and hit the invalid RIP check.

Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
Tested-by: Liam Merwick <liam.merwick@oracle.com>

> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index c3d9006478a4..e1d2a46e06bf 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1995,6 +1995,17 @@ static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
>          vmcb_mark_dirty(svm->vmcb, VMCB_DR);
>   }
> 
> +static char *svm_get_pf_insn_bytes(struct vcpu_svm *svm)
> +{
> +       if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
> +               return NULL;
> +
> +       if (svm->vmcb->control.exit_info_1 & PFERR_FETCH_MASK)
> +               return NULL;
> +
> +       return svm->vmcb->control.insn_bytes;
> +}
> +
>   static int pf_interception(struct kvm_vcpu *vcpu)
>   {
>          struct vcpu_svm *svm = to_svm(vcpu);
> @@ -2003,9 +2014,8 @@ static int pf_interception(struct kvm_vcpu *vcpu)
>          u64 error_code = svm->vmcb->control.exit_info_1;
> 
>          return kvm_handle_page_fault(vcpu, error_code, fault_address,
> -                       static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> -                       svm->vmcb->control.insn_bytes : NULL,
> -                       svm->vmcb->control.insn_len);
> +                                    svm_get_pf_insn_bytes(svm),
> +                                    svm->vmcb->control.insn_len);
>   }
> 
>   static int npf_interception(struct kvm_vcpu *vcpu)
> @@ -2017,9 +2027,8 @@ static int npf_interception(struct kvm_vcpu *vcpu)
> 
>          trace_kvm_page_fault(fault_address, error_code);
>          return kvm_mmu_page_fault(vcpu, fault_address, error_code,
> -                       static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> -                       svm->vmcb->control.insn_bytes : NULL,
> -                       svm->vmcb->control.insn_len);
> +                                 svm_get_pf_insn_bytes(svm),
> +                                 svm->vmcb->control.insn_len);
>   }
> 
>   static int db_interception(struct kvm_vcpu *vcpu)

Re: Query about calling kvm_vcpu_gfn_to_memslot() with a GVA (Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address

[Sean Christopherson]

On Mon, Jan 17, 2022, Liam Merwick wrote:
> On 13/01/2022 16:57, Sean Christopherson wrote:
> > On Thu, Jan 13, 2022, Liam Merwick wrote:
> > > When looking into an SEV issue it was noted that the second arg to
> > > kvm_vcpu_gfn_to_memslot() is a gfn_t but kvm_rip_read() will return guest
> > > RIP which is a guest virtual address and memslots hold guest physical
> > > addresses. How is KVM supposed to translate it to a memslot
> > > and indicate if the guest RIP is valid?
> > 
> > Ugh, magic?  That code is complete garbage.  It worked to fix the selftest issue
> > because the selftest identity maps the relevant guest code.
> > 
> > The entire idea is a hack.  If KVM gets into an infinite loop because the guest
> > is attempting to fetch from MMIO, then the #NPF/#PF should have the FETCH bit set
> > in the error code.  I.e. I believe the below change should fix the original issue,
> > at which point we can revert the above.  I'll test today and hopefully get a patch
> > sent out.
> 
> Thanks Sean.
> 
> I have been running with this patch along with reverting commit
> e72436bc3a52 ("KVM: SVM: avoid infinite loop on NPF from bad address")
> with over 150 hours runtime on multiple machines and it resolves an SEV
> guest crash I was encountering where if there were no decode assist bytes
> available, it then continued on and hit the invalid RIP check.

Nice!  Thanks for the update.  The below patch doesn't fully remedy KVM's woes,
and it's not the best place to handle PFERR_FETCH_MASK for other reasons, so what
I'll officially post (hopefully soon) will be different, but the basic gist will
be the same.

> Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
> Tested-by: Liam Merwick <liam.merwick@oracle.com>
> 
> > 
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index c3d9006478a4..e1d2a46e06bf 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -1995,6 +1995,17 @@ static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
> >          vmcb_mark_dirty(svm->vmcb, VMCB_DR);
> >   }
> > 
> > +static char *svm_get_pf_insn_bytes(struct vcpu_svm *svm)
> > +{
> > +       if (!static_cpu_has(X86_FEATURE_DECODEASSISTS))
> > +               return NULL;
> > +
> > +       if (svm->vmcb->control.exit_info_1 & PFERR_FETCH_MASK)
> > +               return NULL;
> > +
> > +       return svm->vmcb->control.insn_bytes;
> > +}
> > +
> >   static int pf_interception(struct kvm_vcpu *vcpu)
> >   {
> >          struct vcpu_svm *svm = to_svm(vcpu);
> > @@ -2003,9 +2014,8 @@ static int pf_interception(struct kvm_vcpu *vcpu)
> >          u64 error_code = svm->vmcb->control.exit_info_1;
> > 
> >          return kvm_handle_page_fault(vcpu, error_code, fault_address,
> > -                       static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> > -                       svm->vmcb->control.insn_bytes : NULL,
> > -                       svm->vmcb->control.insn_len);
> > +                                    svm_get_pf_insn_bytes(svm),
> > +                                    svm->vmcb->control.insn_len);
> >   }
> > 
> >   static int npf_interception(struct kvm_vcpu *vcpu)
> > @@ -2017,9 +2027,8 @@ static int npf_interception(struct kvm_vcpu *vcpu)
> > 
> >          trace_kvm_page_fault(fault_address, error_code);
> >          return kvm_mmu_page_fault(vcpu, fault_address, error_code,
> > -                       static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> > -                       svm->vmcb->control.insn_bytes : NULL,
> > -                       svm->vmcb->control.insn_len);
> > +                                 svm_get_pf_insn_bytes(svm),
> > +                                 svm->vmcb->control.insn_len);
> >   }
> > 
> >   static int db_interception(struct kvm_vcpu *vcpu)
>