From: Liam Merwick <liam.merwick@oracle.com>
To: Sean Christopherson <seanjc@google.com>
Cc: pbonzini@redhat.com, kvm@vger.kernel.org,
"Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
Liam Merwick <liam.merwick@oracle.com>
Subject: 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
Date: Mon, 17 Jan 2022 17:09:02 +0000 [thread overview]
Message-ID: <fcf4c5c8-aa13-11bf-ec6d-1775b3bd9cd2@oracle.com> (raw)
In-Reply-To: <YeBZ+QcXUIQ7/fD2@google.com>
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)
next prev parent reply other threads:[~2022-01-17 17:11 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-17 16:38 [PATCH 0/2] KVM: fix set_memory_region_test on AMD Paolo Bonzini
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
2020-07-08 9:08 ` Wanpeng Li
2020-07-08 11:10 ` Paolo Bonzini
2021-06-08 4:39 ` Salvatore Bonaccorso
2021-06-08 7:17 ` Paolo Bonzini
2022-01-13 16:27 ` Query about calling kvm_vcpu_gfn_to_memslot() with a GVA (Re: " Liam Merwick
2022-01-13 16:57 ` Sean Christopherson
2022-01-17 17:09 ` Liam Merwick [this message]
2022-01-18 18:46 ` Sean Christopherson
2020-04-17 16:38 ` [PATCH 2/2] selftests: kvm/set_memory_region_test: do not check RIP if the guest shuts down Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fcf4c5c8-aa13-11bf-ec6d-1775b3bd9cd2@oracle.com \
--to=liam.merwick@oracle.com \
--cc=boris.ostrovsky@oracle.com \
--cc=kvm@vger.kernel.org \
--cc=maciej.szmigiero@oracle.com \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).