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

[Salvatore Bonaccorso <carnil@debian.org>]

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