* Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address
2020-04-17 16:38 ` [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address Paolo Bonzini
@ 2020-04-21 19:56 ` Sasha Levin
2020-07-08 8:17 ` Wanpeng Li
2021-06-08 4:39 ` Salvatore Bonaccorso
2 siblings, 0 replies; 8+ messages in thread
From: Sasha Levin @ 2020-04-21 19:56 UTC (permalink / raw)
To: Sasha Levin, Paolo Bonzini, linux-kernel, kvm
Cc: Sean Christopherson, stable, stable
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address
2020-04-17 16:38 ` [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address Paolo Bonzini
2020-04-21 19:56 ` Sasha Levin
@ 2020-07-08 8:17 ` Wanpeng Li
2020-07-08 8:38 ` Paolo Bonzini
2021-06-08 4:39 ` Salvatore Bonaccorso
2 siblings, 1 reply; 8+ messages in thread
From: Wanpeng Li @ 2020-07-08 8:17 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: LKML, kvm, Sean Christopherson, # v3 . 10+
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)
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address
2020-07-08 8:17 ` Wanpeng Li
@ 2020-07-08 8:38 ` Paolo Bonzini
2020-07-08 9:08 ` Wanpeng Li
0 siblings, 1 reply; 8+ messages in thread
From: Paolo Bonzini @ 2020-07-08 8:38 UTC (permalink / raw)
To: Wanpeng Li; +Cc: LKML, kvm, Sean Christopherson, # v3 . 10+
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address
2020-07-08 8:38 ` Paolo Bonzini
@ 2020-07-08 9:08 ` Wanpeng Li
2020-07-08 11:10 ` Paolo Bonzini
0 siblings, 1 reply; 8+ messages in thread
From: Wanpeng Li @ 2020-07-08 9:08 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: LKML, kvm, Sean Christopherson, # v3 . 10+
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address
2020-07-08 9:08 ` Wanpeng Li
@ 2020-07-08 11:10 ` Paolo Bonzini
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2020-07-08 11:10 UTC (permalink / raw)
To: Wanpeng Li; +Cc: LKML, kvm, Sean Christopherson, # v3 . 10+
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address
2020-04-17 16:38 ` [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address Paolo Bonzini
2020-04-21 19:56 ` Sasha Levin
2020-07-08 8:17 ` Wanpeng Li
@ 2021-06-08 4:39 ` Salvatore Bonaccorso
2021-06-08 7:17 ` Paolo Bonzini
2 siblings, 1 reply; 8+ messages in thread
From: Salvatore Bonaccorso @ 2021-06-08 4:39 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, Sean Christopherson, stable, Wanpeng Li
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] KVM: SVM: avoid infinite loop on NPF from bad address
2021-06-08 4:39 ` Salvatore Bonaccorso
@ 2021-06-08 7:17 ` Paolo Bonzini
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2021-06-08 7:17 UTC (permalink / raw)
To: Salvatore Bonaccorso
Cc: linux-kernel, kvm, Sean Christopherson, stable, Wanpeng Li
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
^ permalink raw reply [flat|nested] 8+ messages in thread