* KVM: SVM: Fix workaround for AMD Errata 1096 @ 2019-07-15 20:30 Liran Alon 2019-07-15 20:30 ` [PATCH 1/2] " Liran Alon 2019-07-15 20:30 ` [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() Liran Alon 0 siblings, 2 replies; 34+ messages in thread From: Liran Alon @ 2019-07-15 20:30 UTC (permalink / raw) To: pbonzini, rkrcmar, kvm; +Cc: brijesh.singh Hi, This simple patch series aims to fix a bug in how KVM handles AMD Errata 1096. 1st patch is the fix. The bug was that vCPU state is checked incorrectly for being able to possible raise a SMAP violation which is required to encounter AMD Errata 1096. 2nd patch is a simple rename to make code more readable. Disclaimer: I don't have the proper physical AMD machine to verify the fix. I would appreciate if someone who have access to such AMD machine will test it and reply. Thanks, -Liran ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-15 20:30 KVM: SVM: Fix workaround for AMD Errata 1096 Liran Alon @ 2019-07-15 20:30 ` Liran Alon 2019-07-16 15:48 ` Singh, Brijesh 2019-07-15 20:30 ` [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() Liran Alon 1 sibling, 1 reply; 34+ messages in thread From: Liran Alon @ 2019-07-15 20:30 UTC (permalink / raw) To: pbonzini, rkrcmar, kvm; +Cc: brijesh.singh, Liran Alon, Boris Ostrovsky According to AMD Errata 1096: "On a nested data page fault when CR4.SMAP = 1 and the guest data read generates a SMAP violation, the GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction bytes." As stated above, errata is encountered when guest read generates a SMAP violation. i.e. vCPU runs with CPL<3 and CR4.SMAP=1. However, code have mistakenly checked if CPL==3 and CR4.SMAP==0. To avoid future confusion and improve code readbility, comment errata details in code and not just in commit message. Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)") Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- arch/x86/kvm/svm.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 735b8c01895e..79023a41f7a7 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -7123,10 +7123,19 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) bool is_user, smap; is_user = svm_get_cpl(vcpu) == 3; - smap = !kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); + smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); /* - * Detect and workaround Errata 1096 Fam_17h_00_0Fh + * Detect and workaround Errata 1096 Fam_17h_00_0Fh. + * + * Errata: + * On a nested page fault when CR4.SMAP=1 and the guest data read generates + * a SMAP violation, GuestIntrBytes field of the VMCB on a VMEXIT will + * incorrectly return 0 instead the correct guest instruction bytes. + * + * Workaround: + * To determine what instruction the guest was executing, the hypervisor + * will have to decode the instruction at the instruction pointer. * * In non SEV guest, hypervisor will be able to read the guest * memory to decode the instruction pointer when insn_len is zero @@ -7137,11 +7146,11 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) * instruction pointer so we will not able to workaround it. Lets * print the error and request to kill the guest. */ - if (is_user && smap) { + if (!is_user && smap) { if (!sev_guest(vcpu->kvm)) return true; - pr_err_ratelimited("KVM: Guest triggered AMD Erratum 1096\n"); + pr_err_ratelimited("KVM: SEV Guest triggered AMD Erratum 1096\n"); kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-15 20:30 ` [PATCH 1/2] " Liran Alon @ 2019-07-16 15:48 ` Singh, Brijesh 2019-07-16 15:56 ` Liran Alon 0 siblings, 1 reply; 34+ messages in thread From: Singh, Brijesh @ 2019-07-16 15:48 UTC (permalink / raw) To: Liran Alon, pbonzini, rkrcmar, kvm; +Cc: Singh, Brijesh, Boris Ostrovsky On 7/15/19 3:30 PM, Liran Alon wrote: > According to AMD Errata 1096: > "On a nested data page fault when CR4.SMAP = 1 and the guest data read generates a SMAP violation, the > GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction > bytes." > > As stated above, errata is encountered when guest read generates a SMAP violation. i.e. vCPU runs > with CPL<3 and CR4.SMAP=1. However, code have mistakenly checked if CPL==3 and CR4.SMAP==0. > The SMAP violation will occur from CPL3 so CPL==3 is a valid check. See [1] for complete discussion https://patchwork.kernel.org/patch/10808075/#22479271 However, code mistakenly checked CR4.SMAP==0, it should be CR4.SMAP==1 > To avoid future confusion and improve code readbility, comment errata details in code and not > just in commit message. > > Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)") > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/kvm/svm.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 735b8c01895e..79023a41f7a7 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -7123,10 +7123,19 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) > bool is_user, smap; > > is_user = svm_get_cpl(vcpu) == 3; > - smap = !kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); > + smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); > Ah good catch. thank > /* > - * Detect and workaround Errata 1096 Fam_17h_00_0Fh > + * Detect and workaround Errata 1096 Fam_17h_00_0Fh. > + * > + * Errata: > + * On a nested page fault when CR4.SMAP=1 and the guest data read generates > + * a SMAP violation, GuestIntrBytes field of the VMCB on a VMEXIT will > + * incorrectly return 0 instead the correct guest instruction bytes. > + * > + * Workaround: > + * To determine what instruction the guest was executing, the hypervisor > + * will have to decode the instruction at the instruction pointer. > * > * In non SEV guest, hypervisor will be able to read the guest > * memory to decode the instruction pointer when insn_len is zero > @@ -7137,11 +7146,11 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) > * instruction pointer so we will not able to workaround it. Lets > * print the error and request to kill the guest. > */ > - if (is_user && smap) { > + if (!is_user && smap) { > if (!sev_guest(vcpu->kvm)) > return true; > > - pr_err_ratelimited("KVM: Guest triggered AMD Erratum 1096\n"); > + pr_err_ratelimited("KVM: SEV Guest triggered AMD Erratum 1096\n"); > kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); > } > > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 15:48 ` Singh, Brijesh @ 2019-07-16 15:56 ` Liran Alon 2019-07-16 16:07 ` Liran Alon 2019-07-16 16:10 ` Singh, Brijesh 0 siblings, 2 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 15:56 UTC (permalink / raw) To: Singh, Brijesh; +Cc: pbonzini, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 18:48, Singh, Brijesh <brijesh.singh@amd.com> wrote: > > On 7/15/19 3:30 PM, Liran Alon wrote: >> According to AMD Errata 1096: >> "On a nested data page fault when CR4.SMAP = 1 and the guest data read generates a SMAP violation, the >> GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction >> bytes." >> >> As stated above, errata is encountered when guest read generates a SMAP violation. i.e. vCPU runs >> with CPL<3 and CR4.SMAP=1. However, code have mistakenly checked if CPL==3 and CR4.SMAP==0. >> > > The SMAP violation will occur from CPL3 so CPL==3 is a valid check. > > See [1] for complete discussion > > https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_10808075_-2322479271&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=RAt8t8nBaCxUPy5OTDkO0n8BMQ5l9oSfLMiL0TLTu6c&s=Nkwe8rTJhygBCIPz27LXrylptjnWyMwB-nJaiowWpWc&e= I still don’t understand. SMAP is a mechanism which is meant to protect a CPU running in CPL<3 from mistakenly referencing data controllable by CPL==3. Therefore, SMAP violation should be raised when CPL<3 and data referenced is mapped in page-tables with PTE with U/S bit set to 1. (i.e. User accessible). Thus, we should check if CPL<3 and CR4.SMAP==1. -Liran > > However, code mistakenly checked CR4.SMAP==0, it should be CR4.SMAP==1 > >> To avoid future confusion and improve code readbility, comment errata details in code and not >> just in commit message. >> >> Fixes: 05d5a4863525 ("KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation)") >> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> >> --- >> arch/x86/kvm/svm.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index 735b8c01895e..79023a41f7a7 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -7123,10 +7123,19 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) >> bool is_user, smap; >> >> is_user = svm_get_cpl(vcpu) == 3; >> - smap = !kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); >> + smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); >> > > Ah good catch. thank > > >> /* >> - * Detect and workaround Errata 1096 Fam_17h_00_0Fh >> + * Detect and workaround Errata 1096 Fam_17h_00_0Fh. >> + * >> + * Errata: >> + * On a nested page fault when CR4.SMAP=1 and the guest data read generates >> + * a SMAP violation, GuestIntrBytes field of the VMCB on a VMEXIT will >> + * incorrectly return 0 instead the correct guest instruction bytes. >> + * >> + * Workaround: >> + * To determine what instruction the guest was executing, the hypervisor >> + * will have to decode the instruction at the instruction pointer. >> * >> * In non SEV guest, hypervisor will be able to read the guest >> * memory to decode the instruction pointer when insn_len is zero >> @@ -7137,11 +7146,11 @@ static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) >> * instruction pointer so we will not able to workaround it. Lets >> * print the error and request to kill the guest. >> */ >> - if (is_user && smap) { >> + if (!is_user && smap) { >> if (!sev_guest(vcpu->kvm)) >> return true; >> >> - pr_err_ratelimited("KVM: Guest triggered AMD Erratum 1096\n"); >> + pr_err_ratelimited("KVM: SEV Guest triggered AMD Erratum 1096\n"); >> kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); >> } >> >> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 15:56 ` Liran Alon @ 2019-07-16 16:07 ` Liran Alon 2019-07-16 16:10 ` Singh, Brijesh 1 sibling, 0 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 16:07 UTC (permalink / raw) To: Singh, Brijesh; +Cc: pbonzini, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 18:56, Liran Alon <liran.alon@oracle.com> wrote: > > > >> On 16 Jul 2019, at 18:48, Singh, Brijesh <brijesh.singh@amd.com> wrote: >> >> On 7/15/19 3:30 PM, Liran Alon wrote: >>> According to AMD Errata 1096: >>> "On a nested data page fault when CR4.SMAP = 1 and the guest data read generates a SMAP violation, the >>> GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction >>> bytes." >>> >>> As stated above, errata is encountered when guest read generates a SMAP violation. i.e. vCPU runs >>> with CPL<3 and CR4.SMAP=1. However, code have mistakenly checked if CPL==3 and CR4.SMAP==0. >>> >> >> The SMAP violation will occur from CPL3 so CPL==3 is a valid check. >> >> See [1] for complete discussion >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_10808075_-2322479271&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=RAt8t8nBaCxUPy5OTDkO0n8BMQ5l9oSfLMiL0TLTu6c&s=Nkwe8rTJhygBCIPz27LXrylptjnWyMwB-nJaiowWpWc&e= > > I still don’t understand. SMAP is a mechanism which is meant to protect a CPU running in CPL<3 from mistakenly referencing data controllable by CPL==3. > Therefore, SMAP violation should be raised when CPL<3 and data referenced is mapped in page-tables with PTE with U/S bit set to 1. (i.e. User accessible). > > Thus, we should check if CPL<3 and CR4.SMAP==1. > > -Liran > To clarify, I would assume that to simulate this Errata we should perform the following: 1) Guest maps code in page-tables as user-accessible (i.e. PTE with U/S bit set to 1). 2) Guest executes this code with CPL<3 (even though mapped as user-accessible which is a security vulnerability in itself…) which access data that is not mapped or marked as reserved in NPT and therefore cause #NPF. 3) Physical CPU DecodeAssist feature attempts to fill-in guest instruction bytes. So it reads as data the guest instructions while CPU is currently with CPL<3, CR4.SMAP=1 and code is mapped as user-accessible. Therefore, this fill-in raise a SMAP violation which cause #NPF to be raised to KVM with 0 instruction bytes. Am I mistaken in my analysis? -Liran ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 15:56 ` Liran Alon 2019-07-16 16:07 ` Liran Alon @ 2019-07-16 16:10 ` Singh, Brijesh 2019-07-16 16:20 ` Liran Alon 1 sibling, 1 reply; 34+ messages in thread From: Singh, Brijesh @ 2019-07-16 16:10 UTC (permalink / raw) To: Liran Alon; +Cc: Singh, Brijesh, pbonzini, rkrcmar, kvm, Boris Ostrovsky On 7/16/19 10:56 AM, Liran Alon wrote: > > >> On 16 Jul 2019, at 18:48, Singh, Brijesh <brijesh.singh@amd.com> wrote: >> >> On 7/15/19 3:30 PM, Liran Alon wrote: >>> According to AMD Errata 1096: >>> "On a nested data page fault when CR4.SMAP = 1 and the guest data read generates a SMAP violation, the >>> GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction >>> bytes." >>> >>> As stated above, errata is encountered when guest read generates a SMAP violation. i.e. vCPU runs >>> with CPL<3 and CR4.SMAP=1. However, code have mistakenly checked if CPL==3 and CR4.SMAP==0. >>> >> >> The SMAP violation will occur from CPL3 so CPL==3 is a valid check. >> >> See [1] for complete discussion >> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_10808075_-2322479271&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=RAt8t8nBaCxUPy5OTDkO0n8BMQ5l9oSfLMiL0TLTu6c&s=Nkwe8rTJhygBCIPz27LXrylptjnWyMwB-nJaiowWpWc&e= > > I still don’t understand. SMAP is a mechanism which is meant to protect a CPU running in CPL<3 from mistakenly referencing data controllable by CPL==3. > Therefore, SMAP violation should be raised when CPL<3 and data referenced is mapped in page-tables with PTE with U/S bit set to 1. (i.e. User accessible). > > Thus, we should check if CPL<3 and CR4.SMAP==1. > In this particular case we are dealing with NPF and not SMAP fault per say. What typically has happened here is: - user space does the MMIO access which causes a fault - hardware processes this as a VMEXIT - during processing, hardware attempts to read the instruction bytes to provide decode assist. This is typically done by data read request from the RIP that the guest was at. While doing so, we may hit SMAP fault because internally CPU is doing a data read from the RIP to get those instruction bytes. Since it hit the SMAP fault hence it was not able to decode the instruction to provide the insn_len. So we are first checking if it was a fault caused from CPL==3 and SMAP is enabled. If so, we are hitting this errata and it can be workaround. -Brijesh ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 16:10 ` Singh, Brijesh @ 2019-07-16 16:20 ` Liran Alon 2019-07-16 16:41 ` Sean Christopherson 2019-07-16 18:05 ` Singh, Brijesh 0 siblings, 2 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 16:20 UTC (permalink / raw) To: Singh, Brijesh; +Cc: pbonzini, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 19:10, Singh, Brijesh <brijesh.singh@amd.com> wrote: > > > > On 7/16/19 10:56 AM, Liran Alon wrote: >> >> >>> On 16 Jul 2019, at 18:48, Singh, Brijesh <brijesh.singh@amd.com> wrote: >>> >>> On 7/15/19 3:30 PM, Liran Alon wrote: >>>> According to AMD Errata 1096: >>>> "On a nested data page fault when CR4.SMAP = 1 and the guest data read generates a SMAP violation, the >>>> GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction >>>> bytes." >>>> >>>> As stated above, errata is encountered when guest read generates a SMAP violation. i.e. vCPU runs >>>> with CPL<3 and CR4.SMAP=1. However, code have mistakenly checked if CPL==3 and CR4.SMAP==0. >>>> >>> >>> The SMAP violation will occur from CPL3 so CPL==3 is a valid check. >>> >>> See [1] for complete discussion >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_10808075_-2322479271&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=RAt8t8nBaCxUPy5OTDkO0n8BMQ5l9oSfLMiL0TLTu6c&s=Nkwe8rTJhygBCIPz27LXrylptjnWyMwB-nJaiowWpWc&e= >> >> I still don’t understand. SMAP is a mechanism which is meant to protect a CPU running in CPL<3 from mistakenly referencing data controllable by CPL==3. >> Therefore, SMAP violation should be raised when CPL<3 and data referenced is mapped in page-tables with PTE with U/S bit set to 1. (i.e. User accessible). >> >> Thus, we should check if CPL<3 and CR4.SMAP==1. >> > > In this particular case we are dealing with NPF and not SMAP fault per > say. > > What typically has happened here is: > > - user space does the MMIO access which causes a fault > - hardware processes this as a VMEXIT > - during processing, hardware attempts to read the instruction bytes to > provide decode assist. This is typically done by data read request from > the RIP that the guest was at. While doing so, we may hit SMAP fault How can a SMAP fault occur when CPL==3? One of the conditions for SMAP is that CPL<3. I think the confusion is that I believe a code mapped as user-accessible in page-tables but runs with CPL<3 should be the one which does the MMIO. Rather then code running in CPL==3. The sequence of events I imagine to trigger the Errata is as follows: 1) Guest maps code in page-tables as user-accessible (i.e. PTE with U/S bit set to 1). 2) Guest executes this code with CPL<3 (even though mapped as user-accessible which is a security vulnerability in itself…) which access data that is not mapped or marked as reserved in NPT and therefore cause #NPF. 3) Physical CPU DecodeAssist feature attempts to fill-in guest instruction bytes. So it reads as data the guest instructions while CPU is currently with CPL<3, CR4.SMAP=1 and code is mapped as user-accessible. Therefore, this fill-in raise a SMAP violation which cause #NPF to be raised to KVM with 0 instruction bytes. BTW, this also means that in order to trigger this, CR4.SMEP should be set to 0. As otherwise, instruction couldn’t have been executed to raise #NPF in the first place. Maybe we can add this as another condition to recognise the Errata? -Liran > because internally CPU is doing a data read from the RIP to get those > instruction bytes. Since it hit the SMAP fault hence it was not able > to decode the instruction to provide the insn_len. So we are first > checking if it was a fault caused from CPL==3 and SMAP is enabled. > If so, we are hitting this errata and it can be workaround. > > -Brijesh > > > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 16:20 ` Liran Alon @ 2019-07-16 16:41 ` Sean Christopherson 2019-07-16 16:56 ` Liran Alon 2019-07-16 18:05 ` Singh, Brijesh 1 sibling, 1 reply; 34+ messages in thread From: Sean Christopherson @ 2019-07-16 16:41 UTC (permalink / raw) To: Liran Alon; +Cc: Singh, Brijesh, pbonzini, rkrcmar, kvm, Boris Ostrovsky On Tue, Jul 16, 2019 at 07:20:42PM +0300, Liran Alon wrote: > How can a SMAP fault occur when CPL==3? One of the conditions for SMAP is > that CPL<3. The CPU is effectively at CPL0 when it does the decode assist, e.g.: 1. CPL3 guest hits reserved bit NPT fault (MMIO access) 2. CPU transitions to CPL0 on VM-Exit 3. CPU performs data access on **%rip**, encounters SMAP violation 4. CPU squashes SMAP violation, sets VMCB.insn_len=0 5. CPU delivers VM-Exit to software for original NPT fault The original NPT fault is due to a reserved bit (or not present) entry for a MMIO GPA, *not* the GPA corresponding to %rip. The fault on the decode assist is never delivered to software, it simply results in having invalid info in the VMCB's insn_bytes and insn_len fields. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 16:41 ` Sean Christopherson @ 2019-07-16 16:56 ` Liran Alon 2019-07-16 17:27 ` Sean Christopherson 2019-07-16 17:27 ` Paolo Bonzini 0 siblings, 2 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 16:56 UTC (permalink / raw) To: Sean Christopherson Cc: Singh, Brijesh, pbonzini, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 19:41, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Jul 16, 2019 at 07:20:42PM +0300, Liran Alon wrote: >> How can a SMAP fault occur when CPL==3? One of the conditions for SMAP is >> that CPL<3. > > The CPU is effectively at CPL0 when it does the decode assist, e.g.: > > 1. CPL3 guest hits reserved bit NPT fault (MMIO access) > 2. CPU transitions to CPL0 on VM-Exit > 3. CPU performs data access on **%rip**, encounters SMAP violation > 4. CPU squashes SMAP violation, sets VMCB.insn_len=0 > 5. CPU delivers VM-Exit to software for original NPT fault > > The original NPT fault is due to a reserved bit (or not present) entry for > a MMIO GPA, *not* the GPA corresponding to %rip. The fault on the decode > assist is never delivered to software, it simply results in having invalid > info in the VMCB's insn_bytes and insn_len fields. If the CPU performs the VMExit transition of state before doing the data read for DecodeAssist, then I agree that CPL will be 0 on data-access regardless of vCPU CPL. But this also means that SMAP violation should be raised based on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks. Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this Errata. It’s only important that guest page-tables maps the guest RIP as user-accessible. i.e. U/S bit in PTE set to 1. So I’m still left a bit confused rather the correctness of KVM code handling this Errata. -Liran ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 16:56 ` Liran Alon @ 2019-07-16 17:27 ` Sean Christopherson 2019-07-16 17:27 ` Paolo Bonzini 1 sibling, 0 replies; 34+ messages in thread From: Sean Christopherson @ 2019-07-16 17:27 UTC (permalink / raw) To: Liran Alon; +Cc: Singh, Brijesh, pbonzini, rkrcmar, kvm, Boris Ostrovsky On Tue, Jul 16, 2019 at 07:56:31PM +0300, Liran Alon wrote: > > > On 16 Jul 2019, at 19:41, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > > > On Tue, Jul 16, 2019 at 07:20:42PM +0300, Liran Alon wrote: > >> How can a SMAP fault occur when CPL==3? One of the conditions for SMAP is > >> that CPL<3. > > > > The CPU is effectively at CPL0 when it does the decode assist, e.g.: > > > > 1. CPL3 guest hits reserved bit NPT fault (MMIO access) > > 2. CPU transitions to CPL0 on VM-Exit > > 3. CPU performs data access on **%rip**, encounters SMAP violation > > 4. CPU squashes SMAP violation, sets VMCB.insn_len=0 > > 5. CPU delivers VM-Exit to software for original NPT fault > > > > The original NPT fault is due to a reserved bit (or not present) entry for > > a MMIO GPA, *not* the GPA corresponding to %rip. The fault on the decode > > assist is never delivered to software, it simply results in having invalid > > info in the VMCB's insn_bytes and insn_len fields. > > If the CPU performs the VMExit transition of state before doing the data read > for DecodeAssist, then I agree that CPL will be 0 on data-access regardless > of vCPU CPL. But this also means that SMAP violation should be raised based > on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks. > Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this > Errata. Doh, #2 above is likely wrong. My *guess* is that the DecodeAssist walk starts with %rip, i.e. does a NPT walk using the guest context, and that the access is always an implicit system (CPL0) access. I'll get out of the way and let Brijesh answer :-) ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 16:56 ` Liran Alon 2019-07-16 17:27 ` Sean Christopherson @ 2019-07-16 17:27 ` Paolo Bonzini 2019-07-16 17:35 ` Liran Alon 1 sibling, 1 reply; 34+ messages in thread From: Paolo Bonzini @ 2019-07-16 17:27 UTC (permalink / raw) To: Liran Alon, Sean Christopherson Cc: Singh, Brijesh, rkrcmar, kvm, Boris Ostrovsky On 16/07/19 18:56, Liran Alon wrote: > If the CPU performs the VMExit transition of state before doing the data read for DecodeAssist, > then I agree that CPL will be 0 on data-access regardless of vCPU CPL. But this also means that SMAP > violation should be raised based on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks. > > Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this Errata. Under the conditions in the code, if CPL were <3 then the SMAP fault would have been sent to the guest. But I agree that if we need to change it to check host CR4, then the CPL of the guest should not be checked. Paolo > It’s only important that guest page-tables maps the guest RIP as user-accessible. i.e. U/S bit in PTE set to 1. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 17:27 ` Paolo Bonzini @ 2019-07-16 17:35 ` Liran Alon 2019-07-16 19:28 ` Singh, Brijesh 0 siblings, 1 reply; 34+ messages in thread From: Liran Alon @ 2019-07-16 17:35 UTC (permalink / raw) To: Paolo Bonzini Cc: Sean Christopherson, Singh, Brijesh, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 20:27, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 16/07/19 18:56, Liran Alon wrote: >> If the CPU performs the VMExit transition of state before doing the data read for DecodeAssist, >> then I agree that CPL will be 0 on data-access regardless of vCPU CPL. But this also means that SMAP >> violation should be raised based on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks. >> >> Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this Errata. > > Under the conditions in the code, if CPL were <3 then the SMAP fault > would have been sent to the guest. > But I agree that if we need to > change it to check host CR4, then the CPL of the guest should not be > checked. Yep. Well it all depends on how AMD CPU actually works. We need some clarification from AMD but for sure the current code in KVM is not only wrong, but probably have never been tested. :P Looking for further clarifications from AMD before submitting v2… -Liran > > Paolo > >> It’s only important that guest page-tables maps the guest RIP as user-accessible. i.e. U/S bit in PTE set to 1. > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 17:35 ` Liran Alon @ 2019-07-16 19:28 ` Singh, Brijesh 2019-07-16 19:34 ` Liran Alon 0 siblings, 1 reply; 34+ messages in thread From: Singh, Brijesh @ 2019-07-16 19:28 UTC (permalink / raw) To: Liran Alon, Paolo Bonzini Cc: Singh, Brijesh, Sean Christopherson, rkrcmar, kvm, Boris Ostrovsky On 7/16/19 12:35 PM, Liran Alon wrote: > > >> On 16 Jul 2019, at 20:27, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 16/07/19 18:56, Liran Alon wrote: >>> If the CPU performs the VMExit transition of state before doing the data read for DecodeAssist, >>> then I agree that CPL will be 0 on data-access regardless of vCPU CPL. But this also means that SMAP >>> violation should be raised based on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks. >>> >>> Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this Errata. >> >> Under the conditions in the code, if CPL were <3 then the SMAP fault >> would have been sent to the guest. >> But I agree that if we need to >> change it to check host CR4, then the CPL of the guest should not be >> checked. > > Yep. > Well it all depends on how AMD CPU actually works. > We need some clarification from AMD but for sure the current code in KVM is not only wrong, but probably have never been tested. :P > > Looking for further clarifications from AMD before submitting v2… > When this errata is hit, the CPU will be at CPL3. From hardware point-of-view the below sequence happens: 1. CPL3 guest hits reserved bit NPT fault (MMIO access) 2. Microcode uses special opcode which attempts to read data using the CPL0 privileges. The microcode read CS:RIP, when it hits SMAP fault, it gives up and returns no instruction bytes. (Note: vCPU is still at CPL3) 3. CPU causes #VMEXIT for original fault address. The SMAP fault occurred while we are still in guest context. It will be nice to have code test example to triggers this errata. > -Liran > >> >> Paolo >> >>> It’s only important that guest page-tables maps the guest RIP as user-accessible. i.e. U/S bit in PTE set to 1. >> > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 19:28 ` Singh, Brijesh @ 2019-07-16 19:34 ` Liran Alon 2019-07-16 19:39 ` Paolo Bonzini ` (2 more replies) 0 siblings, 3 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 19:34 UTC (permalink / raw) To: Singh, Brijesh Cc: Paolo Bonzini, Sean Christopherson, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 22:28, Singh, Brijesh <brijesh.singh@amd.com> wrote: > > > > On 7/16/19 12:35 PM, Liran Alon wrote: >> >> >>> On 16 Jul 2019, at 20:27, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> On 16/07/19 18:56, Liran Alon wrote: >>>> If the CPU performs the VMExit transition of state before doing the data read for DecodeAssist, >>>> then I agree that CPL will be 0 on data-access regardless of vCPU CPL. But this also means that SMAP >>>> violation should be raised based on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks. >>>> >>>> Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this Errata. >>> >>> Under the conditions in the code, if CPL were <3 then the SMAP fault >>> would have been sent to the guest. >>> But I agree that if we need to >>> change it to check host CR4, then the CPL of the guest should not be >>> checked. >> >> Yep. >> Well it all depends on how AMD CPU actually works. >> We need some clarification from AMD but for sure the current code in KVM is not only wrong, but probably have never been tested. :P >> >> Looking for further clarifications from AMD before submitting v2… >> > > When this errata is hit, the CPU will be at CPL3. From hardware > point-of-view the below sequence happens: > > 1. CPL3 guest hits reserved bit NPT fault (MMIO access) Why CPU needs to be at CPL3? The requirement for SMAP should be that this page is user-accessible in guest page-tables. Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0. > > 2. Microcode uses special opcode which attempts to read data using the > CPL0 privileges. The microcode read CS:RIP, when it hits SMAP fault, > it gives up and returns no instruction bytes. > > (Note: vCPU is still at CPL3) So at this point guest vCPU CR4.SMAP is what matters right? Not host CR4.SMAP. > > 3. CPU causes #VMEXIT for original fault address. > > The SMAP fault occurred while we are still in guest context. It will be > nice to have code test example to triggers this errata. I can write such code in kvm-unit-tests for you to run on relevant hardware if you have such a machine present. I don’t have relevant machine with me and therefore I wrote a disclaimer I couldn’t test it in cover letter. So to sum-up what KVM needs to do: 1) Check guest vCPU CR4.SMAP is set to 1. (As I fixed in this commit). 2) Remove the check for CPL==3. If we really want to be pedantic, we can parse guest page-tables to see if PTE have U/S bit set to 1. What do you think? -Liran > >> -Liran >> >>> >>> Paolo >>> >>>> It’s only important that guest page-tables maps the guest RIP as user-accessible. i.e. U/S bit in PTE set to 1. >>> >> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 19:34 ` Liran Alon @ 2019-07-16 19:39 ` Paolo Bonzini 2019-07-16 19:45 ` Sean Christopherson 2019-07-16 19:47 ` Liran Alon 2019-07-16 19:41 ` Sean Christopherson 2019-07-16 20:02 ` Singh, Brijesh 2 siblings, 2 replies; 34+ messages in thread From: Paolo Bonzini @ 2019-07-16 19:39 UTC (permalink / raw) To: Liran Alon, Singh, Brijesh Cc: Sean Christopherson, rkrcmar, kvm, Boris Ostrovsky On 16/07/19 21:34, Liran Alon wrote: >> When this errata is hit, the CPU will be at CPL3. From hardware >> point-of-view the below sequence happens: >> >> 1. CPL3 guest hits reserved bit NPT fault (MMIO access) > Why CPU needs to be at CPL3? > The requirement for SMAP should be that this page is user-accessible in guest page-tables. > Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0. > If you are not at CPL3, you'd get a SMAP NPF, not a RSVD NPF. Paolo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 19:39 ` Paolo Bonzini @ 2019-07-16 19:45 ` Sean Christopherson 2019-07-16 19:50 ` Liran Alon 2019-07-16 19:47 ` Liran Alon 1 sibling, 1 reply; 34+ messages in thread From: Sean Christopherson @ 2019-07-16 19:45 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Liran Alon, Singh, Brijesh, rkrcmar, kvm, Boris Ostrovsky On Tue, Jul 16, 2019 at 09:39:48PM +0200, Paolo Bonzini wrote: > On 16/07/19 21:34, Liran Alon wrote: > >> When this errata is hit, the CPU will be at CPL3. From hardware > >> point-of-view the below sequence happens: > >> > >> 1. CPL3 guest hits reserved bit NPT fault (MMIO access) > > Why CPU needs to be at CPL3? > > The requirement for SMAP should be that this page is user-accessible in guest page-tables. > > Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0. > > > > If you are not at CPL3, you'd get a SMAP NPF, not a RSVD NPF. I think Liran is right. When software is executing, the %rip access is a code fetch (SMEP), but the ucode assist is a data access (SMAP). This likely has only been observed in a CPL3 scenario because no sane OS exercises the case of the kernel executing from a user page with SMAP=1 and SMEP=0. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 19:45 ` Sean Christopherson @ 2019-07-16 19:50 ` Liran Alon 0 siblings, 0 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 19:50 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, Singh, Brijesh, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 22:45, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Jul 16, 2019 at 09:39:48PM +0200, Paolo Bonzini wrote: >> On 16/07/19 21:34, Liran Alon wrote: >>>> When this errata is hit, the CPU will be at CPL3. From hardware >>>> point-of-view the below sequence happens: >>>> >>>> 1. CPL3 guest hits reserved bit NPT fault (MMIO access) >>> Why CPU needs to be at CPL3? >>> The requirement for SMAP should be that this page is user-accessible in guest page-tables. >>> Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0. >>> >> >> If you are not at CPL3, you'd get a SMAP NPF, not a RSVD NPF. > > I think Liran is right. When software is executing, the %rip access is > a code fetch (SMEP), but the ucode assist is a data access (SMAP). > > This likely has only been observed in a CPL3 scenario because no sane OS > exercises the case of the kernel executing from a user page with SMAP=1 > and SMEP=0. True. I’m trying to be pedantic and accurate here. :) I think we should just remove the vCPU CPL check and remain only with the CR4.SMAP check. Don’t you agree that having a #NPF that returns 0 instruction bytes with DecodeAssist enabled and CR4.SMAP=1 is sufficient for finger-printing this Errata? With which other use-case it’s expected to collide? -Liran ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 19:39 ` Paolo Bonzini 2019-07-16 19:45 ` Sean Christopherson @ 2019-07-16 19:47 ` Liran Alon 1 sibling, 0 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 19:47 UTC (permalink / raw) To: Paolo Bonzini Cc: Singh, Brijesh, Sean Christopherson, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 22:39, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 16/07/19 21:34, Liran Alon wrote: >>> When this errata is hit, the CPU will be at CPL3. From hardware >>> point-of-view the below sequence happens: >>> >>> 1. CPL3 guest hits reserved bit NPT fault (MMIO access) >> Why CPU needs to be at CPL3? >> The requirement for SMAP should be that this page is user-accessible in guest page-tables. >> Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0. >> > > If you are not at CPL3, you'd get a SMAP NPF, not a RSVD NPF. If CR4.SMEP=0, guest vCPU can execute a user-accessible page in guest page-tables with CPL<3. This instruction will successfully execute and can cause by the data it references any type of #NPF. Including RSVD #NPF. When hardware DecodeAssist microcode will attempt to read guest RIP though, it will get a SMAP violation because data read is done by microcode with CPL<3 and is accessing user-accessible page. Therefore, I still don’t think that guest vCPU CPL matters at all. Only whether code page is mapped in guest page-tables as user-accessible or not. -Liran > > Paolo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 19:34 ` Liran Alon 2019-07-16 19:39 ` Paolo Bonzini @ 2019-07-16 19:41 ` Sean Christopherson 2019-07-16 19:52 ` Liran Alon 2019-07-16 20:02 ` Singh, Brijesh 2 siblings, 1 reply; 34+ messages in thread From: Sean Christopherson @ 2019-07-16 19:41 UTC (permalink / raw) To: Liran Alon; +Cc: Singh, Brijesh, Paolo Bonzini, rkrcmar, kvm, Boris Ostrovsky On Tue, Jul 16, 2019 at 10:34:08PM +0300, Liran Alon wrote: > If we really want to be pedantic, we can parse guest page-tables to see if PTE > have U/S bit set to 1. What do you think? Performance aside, walking the guest page tables would fall apart if a different vCPU modified the guest's page tables. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 19:41 ` Sean Christopherson @ 2019-07-16 19:52 ` Liran Alon 0 siblings, 0 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 19:52 UTC (permalink / raw) To: Sean Christopherson Cc: Singh, Brijesh, Paolo Bonzini, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 22:41, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Jul 16, 2019 at 10:34:08PM +0300, Liran Alon wrote: >> If we really want to be pedantic, we can parse guest page-tables to see if PTE >> have U/S bit set to 1. What do you think? > > Performance aside, walking the guest page tables would fall apart if a > different vCPU modified the guest's page tables. True. :) So let’s just stick with checking only CR4.SMAP=1 then. -Liran ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 19:34 ` Liran Alon 2019-07-16 19:39 ` Paolo Bonzini 2019-07-16 19:41 ` Sean Christopherson @ 2019-07-16 20:02 ` Singh, Brijesh 2019-07-16 20:07 ` Sean Christopherson 2019-07-16 20:09 ` Liran Alon 2 siblings, 2 replies; 34+ messages in thread From: Singh, Brijesh @ 2019-07-16 20:02 UTC (permalink / raw) To: Liran Alon Cc: Singh, Brijesh, Paolo Bonzini, Sean Christopherson, rkrcmar, kvm, Boris Ostrovsky On 7/16/19 2:34 PM, Liran Alon wrote: > > >> On 16 Jul 2019, at 22:28, Singh, Brijesh <brijesh.singh@amd.com> wrote: >> >> >> >> On 7/16/19 12:35 PM, Liran Alon wrote: >>> >>> >>>> On 16 Jul 2019, at 20:27, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> >>>> On 16/07/19 18:56, Liran Alon wrote: >>>>> If the CPU performs the VMExit transition of state before doing the data read for DecodeAssist, >>>>> then I agree that CPL will be 0 on data-access regardless of vCPU CPL. But this also means that SMAP >>>>> violation should be raised based on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks. >>>>> >>>>> Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this Errata. >>>> >>>> Under the conditions in the code, if CPL were <3 then the SMAP fault >>>> would have been sent to the guest. >>>> But I agree that if we need to >>>> change it to check host CR4, then the CPL of the guest should not be >>>> checked. >>> >>> Yep. >>> Well it all depends on how AMD CPU actually works. >>> We need some clarification from AMD but for sure the current code in KVM is not only wrong, but probably have never been tested. :P >>> >>> Looking for further clarifications from AMD before submitting v2… >>> >> >> When this errata is hit, the CPU will be at CPL3. From hardware >> point-of-view the below sequence happens: >> >> 1. CPL3 guest hits reserved bit NPT fault (MMIO access) > > Why CPU needs to be at CPL3? > The requirement for SMAP should be that this page is user-accessible in guest page-tables. > Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0. > We are discussing reserved NPF so we need to be at CPL3. >> >> 2. Microcode uses special opcode which attempts to read data using the >> CPL0 privileges. The microcode read CS:RIP, when it hits SMAP fault, >> it gives up and returns no instruction bytes. >> >> (Note: vCPU is still at CPL3) > > So at this point guest vCPU CR4.SMAP is what matters right? Not host CR4.SMAP. > Yes, its guest vCPU SMAP. >> >> 3. CPU causes #VMEXIT for original fault address. >> >> The SMAP fault occurred while we are still in guest context. It will be >> nice to have code test example to triggers this errata. > > I can write such code in kvm-unit-tests for you to run on relevant hardware if you have such a machine present. > I don’t have relevant machine with me and therefore I wrote a disclaimer I couldn’t test it in cover letter. > I have required hardware and should be able to run some test for you. > So to sum-up what KVM needs to do: > 1) Check guest vCPU CR4.SMAP is set to 1. (As I fixed in this commit). > 2) Remove the check for CPL==3. If we really want to be pedantic, we can parse guest page-tables to see if PTE have U/S bit set to 1. Remember in the case of SEV guest, the page-tables are encrypted with the guest specific key and we will not be able to walk it to inspect the U/S bit. We want to detect the errata and if its SEV guest then we can't do much, for non-SEV fallback to instruction decode which will walk the guest page-tables to fetch the instruction bytes. > What do you think? > > -Liran > >> >>> -Liran >>> >>>> >>>> Paolo >>>> >>>>> It’s only important that guest page-tables maps the guest RIP as user-accessible. i.e. U/S bit in PTE set to 1. >>>> >>> > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 20:02 ` Singh, Brijesh @ 2019-07-16 20:07 ` Sean Christopherson 2019-07-16 20:13 ` Paolo Bonzini 2019-07-16 20:09 ` Liran Alon 1 sibling, 1 reply; 34+ messages in thread From: Sean Christopherson @ 2019-07-16 20:07 UTC (permalink / raw) To: Singh, Brijesh; +Cc: Liran Alon, Paolo Bonzini, rkrcmar, kvm, Boris Ostrovsky On Tue, Jul 16, 2019 at 08:02:43PM +0000, Singh, Brijesh wrote: > > On 7/16/19 2:34 PM, Liran Alon wrote: > > Why CPU needs to be at CPL3? > > The requirement for SMAP should be that this page is user-accessible in guest page-tables. > > Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0. > > > > We are discussing reserved NPF so we need to be at CPL3. For my own education, why does reserved NPF require CPL3? I.e. what happens if a reserved bit is encountered at CPL<3 (or do they simply not exist)? ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 20:07 ` Sean Christopherson @ 2019-07-16 20:13 ` Paolo Bonzini 0 siblings, 0 replies; 34+ messages in thread From: Paolo Bonzini @ 2019-07-16 20:13 UTC (permalink / raw) To: Sean Christopherson, Singh, Brijesh Cc: Liran Alon, rkrcmar, kvm, Boris Ostrovsky On 16/07/19 22:07, Sean Christopherson wrote: >> We are discussing reserved NPF so we need to be at CPL3. > For my own education, why does reserved NPF require CPL3? I.e. what > happens if a reserved bit is encountered at CPL<3 (or do they simply not > exist)? Better: what happens if a reserved bit is encountered at CPL<3 *with CR4.SMEP=0*? Paolo ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 20:02 ` Singh, Brijesh 2019-07-16 20:07 ` Sean Christopherson @ 2019-07-16 20:09 ` Liran Alon 2019-07-16 20:27 ` Singh, Brijesh 1 sibling, 1 reply; 34+ messages in thread From: Liran Alon @ 2019-07-16 20:09 UTC (permalink / raw) To: Singh, Brijesh Cc: Paolo Bonzini, Sean Christopherson, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 23:02, Singh, Brijesh <brijesh.singh@amd.com> wrote: > > > > On 7/16/19 2:34 PM, Liran Alon wrote: >> >> >>> On 16 Jul 2019, at 22:28, Singh, Brijesh <brijesh.singh@amd.com> wrote: >>> >>> >>> >>> On 7/16/19 12:35 PM, Liran Alon wrote: >>>> >>>> >>>>> On 16 Jul 2019, at 20:27, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>> >>>>> On 16/07/19 18:56, Liran Alon wrote: >>>>>> If the CPU performs the VMExit transition of state before doing the data read for DecodeAssist, >>>>>> then I agree that CPL will be 0 on data-access regardless of vCPU CPL. But this also means that SMAP >>>>>> violation should be raised based on host CR4.SMAP value and not vCPU CR4.SMAP value as KVM code checks. >>>>>> >>>>>> Furthermore, vCPU CPL of guest doesn’t need to be 3 in order to trigger this Errata. >>>>> >>>>> Under the conditions in the code, if CPL were <3 then the SMAP fault >>>>> would have been sent to the guest. >>>>> But I agree that if we need to >>>>> change it to check host CR4, then the CPL of the guest should not be >>>>> checked. >>>> >>>> Yep. >>>> Well it all depends on how AMD CPU actually works. >>>> We need some clarification from AMD but for sure the current code in KVM is not only wrong, but probably have never been tested. :P >>>> >>>> Looking for further clarifications from AMD before submitting v2… >>>> >>> >>> When this errata is hit, the CPU will be at CPL3. From hardware >>> point-of-view the below sequence happens: >>> >>> 1. CPL3 guest hits reserved bit NPT fault (MMIO access) >> >> Why CPU needs to be at CPL3? >> The requirement for SMAP should be that this page is user-accessible in guest page-tables. >> Think on a case where guest have CR4.SMAP=1 and CR4.SMEP=0. >> > > We are discussing reserved NPF so we need to be at CPL3. I don’t see the connection between a reserved #NPF and the need to be at CPL3. A vCPU can execute at CPL<3 a page that is mapped user-accessible in guest page-tables in case CR4.SMEP=0 and then instruction will execute successfully and can dereference a page that is mapped in NPT using an entry with a reserved bit set. Thus, reserved #NPF will be raised while vCPU is at CPL<3 and DecodeAssist microcode will still raise SMAP violation as CR4.SMAP=1 and microcode perform data-fetch with CPL<3. This leading exactly to Errata condition as far as I understand. > >>> >>> 2. Microcode uses special opcode which attempts to read data using the >>> CPL0 privileges. The microcode read CS:RIP, when it hits SMAP fault, >>> it gives up and returns no instruction bytes. >>> >>> (Note: vCPU is still at CPL3) >> >> So at this point guest vCPU CR4.SMAP is what matters right? Not host CR4.SMAP. >> > > Yes, its guest vCPU SMAP. > >>> >>> 3. CPU causes #VMEXIT for original fault address. >>> >>> The SMAP fault occurred while we are still in guest context. It will be >>> nice to have code test example to triggers this errata. >> >> I can write such code in kvm-unit-tests for you to run on relevant hardware if you have such a machine present. >> I don’t have relevant machine with me and therefore I wrote a disclaimer I couldn’t test it in cover letter. >> > > I have required hardware and should be able to run some test for you. Ok. Will write such kvm-unit-test and Cc you. > >> So to sum-up what KVM needs to do: >> 1) Check guest vCPU CR4.SMAP is set to 1. (As I fixed in this commit). >> 2) Remove the check for CPL==3. If we really want to be pedantic, we can parse guest page-tables to see if PTE have U/S bit set to 1. > > > Remember in the case of SEV guest, the page-tables are encrypted with > the guest specific key and we will not be able to walk it to inspect > the U/S bit. We want to detect the errata and if its SEV guest then > we can't do much, for non-SEV fallback to instruction decode which > will walk the guest page-tables to fetch the instruction bytes. > > >> What do you think? >> >> -Liran >> >>> >>>> -Liran >>>> >>>>> >>>>> Paolo >>>>> >>>>>> It’s only important that guest page-tables maps the guest RIP as user-accessible. i.e. U/S bit in PTE set to 1. >>>>> >>>> >> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 20:09 ` Liran Alon @ 2019-07-16 20:27 ` Singh, Brijesh 2019-07-16 20:54 ` Sean Christopherson 0 siblings, 1 reply; 34+ messages in thread From: Singh, Brijesh @ 2019-07-16 20:27 UTC (permalink / raw) To: Liran Alon Cc: Singh, Brijesh, Paolo Bonzini, Sean Christopherson, rkrcmar, kvm, Boris Ostrovsky On 7/16/19 3:09 PM, Liran Alon wrote: ... >> >> We are discussing reserved NPF so we need to be at CPL3. > > I don’t see the connection between a reserved #NPF and the need to be at CPL3. > A vCPU can execute at CPL<3 a page that is mapped user-accessible in guest page-tables in case CR4.SMEP=0 > and then instruction will execute successfully and can dereference a page that is mapped in NPT using an entry with a reserved bit set. > Thus, reserved #NPF will be raised while vCPU is at CPL<3 and DecodeAssist microcode will still raise SMAP violation > as CR4.SMAP=1 and microcode perform data-fetch with CPL<3. This leading exactly to Errata condition as far as I understand. > Yes, vCPU at CPL<3 can raise the SMAP violation. When SMEP is disabled, the guest kernel never should be executing from code in user-mode pages, that'd be insecure. So I am not sure if kernel code can cause this errata. Having said so, I'm OK to remove the CPL check if it makes code more readable. -Brijesh ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 20:27 ` Singh, Brijesh @ 2019-07-16 20:54 ` Sean Christopherson 2019-07-16 21:53 ` Liran Alon 0 siblings, 1 reply; 34+ messages in thread From: Sean Christopherson @ 2019-07-16 20:54 UTC (permalink / raw) To: Singh, Brijesh; +Cc: Liran Alon, Paolo Bonzini, rkrcmar, kvm, Boris Ostrovsky On Tue, Jul 16, 2019 at 08:27:12PM +0000, Singh, Brijesh wrote: > > On 7/16/19 3:09 PM, Liran Alon wrote: > >> > >> We are discussing reserved NPF so we need to be at CPL3. > > > > I don’t see the connection between a reserved #NPF and the need to be at > > CPL3. A vCPU can execute at CPL<3 a page that is mapped user-accessible in > > guest page-tables in case CR4.SMEP=0 and then instruction will execute > > successfully and can dereference a page that is mapped in NPT using an > > entry with a reserved bit set. Thus, reserved #NPF will be raised while > > vCPU is at CPL<3 and DecodeAssist microcode will still raise SMAP violation > > as CR4.SMAP=1 and microcode perform data-fetch with CPL<3. This leading > > exactly to Errata condition as far as I understand. > > > > Yes, vCPU at CPL<3 can raise the SMAP violation. When SMEP is disabled, > the guest kernel never should be executing from code in user-mode pages, > that'd be insecure. So I am not sure if kernel code can cause this > errata. From KVM's perspective, it's not a question of what is *likely* to happen so much as it's a question of what *can* happen. Architecturally there is nothing that prevents CPL<3 code from encountering the SMAP fault. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 20:54 ` Sean Christopherson @ 2019-07-16 21:53 ` Liran Alon 0 siblings, 0 replies; 34+ messages in thread From: Liran Alon @ 2019-07-16 21:53 UTC (permalink / raw) To: Sean Christopherson Cc: Singh, Brijesh, Paolo Bonzini, rkrcmar, kvm, Boris Ostrovsky > On 16 Jul 2019, at 23:54, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Tue, Jul 16, 2019 at 08:27:12PM +0000, Singh, Brijesh wrote: >> >> On 7/16/19 3:09 PM, Liran Alon wrote: >>>> >>>> We are discussing reserved NPF so we need to be at CPL3. >>> >>> I don’t see the connection between a reserved #NPF and the need to be at >>> CPL3. A vCPU can execute at CPL<3 a page that is mapped user-accessible in >>> guest page-tables in case CR4.SMEP=0 and then instruction will execute >>> successfully and can dereference a page that is mapped in NPT using an >>> entry with a reserved bit set. Thus, reserved #NPF will be raised while >>> vCPU is at CPL<3 and DecodeAssist microcode will still raise SMAP violation >>> as CR4.SMAP=1 and microcode perform data-fetch with CPL<3. This leading >>> exactly to Errata condition as far as I understand. >>> >> >> Yes, vCPU at CPL<3 can raise the SMAP violation. When SMEP is disabled, >> the guest kernel never should be executing from code in user-mode pages, >> that'd be insecure. So I am not sure if kernel code can cause this >> errata. > > From KVM's perspective, it's not a question of what is *likely* to happen > so much as it's a question of what *can* happen. Architecturally there is > nothing that prevents CPL<3 code from encountering the SMAP fault. Exactly. :) I will submit a v2 patch which also clarifies the details we understood in this email thread. Thanks, -Liran ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 16:20 ` Liran Alon 2019-07-16 16:41 ` Sean Christopherson @ 2019-07-16 18:05 ` Singh, Brijesh 2019-07-16 18:06 ` Singh, Brijesh 1 sibling, 1 reply; 34+ messages in thread From: Singh, Brijesh @ 2019-07-16 18:05 UTC (permalink / raw) To: Liran Alon; +Cc: Singh, Brijesh, pbonzini, rkrcmar, kvm, Boris Ostrovsky Here is a thread.. but more recent is available https://marc.info/?t=156322283300001&r=1&w=2 Paolo, Sean and others have also replied to it which you can see on marc.info. -Brijesh On 7/16/19 11:20 AM, Liran Alon wrote: > > >> On 16 Jul 2019, at 19:10, Singh, Brijesh <brijesh.singh@amd.com> wrote: >> >> >> >> On 7/16/19 10:56 AM, Liran Alon wrote: >>> >>> >>>> On 16 Jul 2019, at 18:48, Singh, Brijesh <brijesh.singh@amd.com> wrote: >>>> >>>> On 7/15/19 3:30 PM, Liran Alon wrote: >>>>> According to AMD Errata 1096: >>>>> "On a nested data page fault when CR4.SMAP = 1 and the guest data read generates a SMAP violation, the >>>>> GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction >>>>> bytes." >>>>> >>>>> As stated above, errata is encountered when guest read generates a SMAP violation. i.e. vCPU runs >>>>> with CPL<3 and CR4.SMAP=1. However, code have mistakenly checked if CPL==3 and CR4.SMAP==0. >>>>> >>>> >>>> The SMAP violation will occur from CPL3 so CPL==3 is a valid check. >>>> >>>> See [1] for complete discussion >>>> >>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_10808075_-2322479271&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=RAt8t8nBaCxUPy5OTDkO0n8BMQ5l9oSfLMiL0TLTu6c&s=Nkwe8rTJhygBCIPz27LXrylptjnWyMwB-nJaiowWpWc&e= >>> >>> I still don’t understand. SMAP is a mechanism which is meant to protect a CPU running in CPL<3 from mistakenly referencing data controllable by CPL==3. >>> Therefore, SMAP violation should be raised when CPL<3 and data referenced is mapped in page-tables with PTE with U/S bit set to 1. (i.e. User accessible). >>> >>> Thus, we should check if CPL<3 and CR4.SMAP==1. >>> >> >> In this particular case we are dealing with NPF and not SMAP fault per >> say. >> >> What typically has happened here is: >> >> - user space does the MMIO access which causes a fault >> - hardware processes this as a VMEXIT >> - during processing, hardware attempts to read the instruction bytes to >> provide decode assist. This is typically done by data read request from >> the RIP that the guest was at. While doing so, we may hit SMAP fault > > How can a SMAP fault occur when CPL==3? One of the conditions for SMAP is that CPL<3. > > I think the confusion is that I believe a code mapped as user-accessible in page-tables but runs with CPL<3 > should be the one which does the MMIO. Rather then code running in CPL==3. > > The sequence of events I imagine to trigger the Errata is as follows: > 1) Guest maps code in page-tables as user-accessible (i.e. PTE with U/S bit set to 1). > 2) Guest executes this code with CPL<3 (even though mapped as user-accessible which is a security vulnerability in itself…) which access data that is not mapped or marked as reserved in NPT and therefore cause #NPF. > 3) Physical CPU DecodeAssist feature attempts to fill-in guest instruction bytes. So it reads as data the guest instructions while CPU is currently with CPL<3, CR4.SMAP=1 and code is mapped as user-accessible. Therefore, this fill-in raise a SMAP violation which cause #NPF to be raised to KVM with 0 instruction bytes. > > BTW, this also means that in order to trigger this, CR4.SMEP should be set to 0. As otherwise, instruction couldn’t have been executed to raise #NPF in the first place. Maybe we can add this as another condition to recognise the Errata? > > -Liran > >> because internally CPU is doing a data read from the RIP to get those >> instruction bytes. Since it hit the SMAP fault hence it was not able >> to decode the instruction to provide the insn_len. So we are first >> checking if it was a fault caused from CPL==3 and SMAP is enabled. >> If so, we are hitting this errata and it can be workaround. >> >> -Brijesh >> >> >> > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 1/2] KVM: SVM: Fix workaround for AMD Errata 1096 2019-07-16 18:05 ` Singh, Brijesh @ 2019-07-16 18:06 ` Singh, Brijesh 0 siblings, 0 replies; 34+ messages in thread From: Singh, Brijesh @ 2019-07-16 18:06 UTC (permalink / raw) To: Liran Alon; +Cc: Singh, Brijesh, pbonzini, rkrcmar, kvm, Boris Ostrovsky Ooops sorry, I was fwding thread HW folks to correct my understanding. I will update you with result. thanks On 7/16/19 1:05 PM, Singh, Brijesh wrote: > Here is a thread.. but more recent is available > > https://marc.info/?t=156322283300001&r=1&w=2 > > Paolo, Sean and others have also replied to it which you can see on > marc.info. > > -Brijesh > > On 7/16/19 11:20 AM, Liran Alon wrote: >> >> >>> On 16 Jul 2019, at 19:10, Singh, Brijesh <brijesh.singh@amd.com> wrote: >>> >>> >>> >>> On 7/16/19 10:56 AM, Liran Alon wrote: >>>> >>>> >>>>> On 16 Jul 2019, at 18:48, Singh, Brijesh <brijesh.singh@amd.com> wrote: >>>>> >>>>> On 7/15/19 3:30 PM, Liran Alon wrote: >>>>>> According to AMD Errata 1096: >>>>>> "On a nested data page fault when CR4.SMAP = 1 and the guest data read generates a SMAP violation, the >>>>>> GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction >>>>>> bytes." >>>>>> >>>>>> As stated above, errata is encountered when guest read generates a SMAP violation. i.e. vCPU runs >>>>>> with CPL<3 and CR4.SMAP=1. However, code have mistakenly checked if CPL==3 and CR4.SMAP==0. >>>>>> >>>>> >>>>> The SMAP violation will occur from CPL3 so CPL==3 is a valid check. >>>>> >>>>> See [1] for complete discussion >>>>> >>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_10808075_-2322479271&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=RAt8t8nBaCxUPy5OTDkO0n8BMQ5l9oSfLMiL0TLTu6c&s=Nkwe8rTJhygBCIPz27LXrylptjnWyMwB-nJaiowWpWc&e= >>>> >>>> I still don’t understand. SMAP is a mechanism which is meant to protect a CPU running in CPL<3 from mistakenly referencing data controllable by CPL==3. >>>> Therefore, SMAP violation should be raised when CPL<3 and data referenced is mapped in page-tables with PTE with U/S bit set to 1. (i.e. User accessible). >>>> >>>> Thus, we should check if CPL<3 and CR4.SMAP==1. >>>> >>> >>> In this particular case we are dealing with NPF and not SMAP fault per >>> say. >>> >>> What typically has happened here is: >>> >>> - user space does the MMIO access which causes a fault >>> - hardware processes this as a VMEXIT >>> - during processing, hardware attempts to read the instruction bytes to >>> provide decode assist. This is typically done by data read request from >>> the RIP that the guest was at. While doing so, we may hit SMAP fault >> >> How can a SMAP fault occur when CPL==3? One of the conditions for SMAP is that CPL<3. >> >> I think the confusion is that I believe a code mapped as user-accessible in page-tables but runs with CPL<3 >> should be the one which does the MMIO. Rather then code running in CPL==3. >> >> The sequence of events I imagine to trigger the Errata is as follows: >> 1) Guest maps code in page-tables as user-accessible (i.e. PTE with U/S bit set to 1). >> 2) Guest executes this code with CPL<3 (even though mapped as user-accessible which is a security vulnerability in itself…) which access data that is not mapped or marked as reserved in NPT and therefore cause #NPF. >> 3) Physical CPU DecodeAssist feature attempts to fill-in guest instruction bytes. So it reads as data the guest instructions while CPU is currently with CPL<3, CR4.SMAP=1 and code is mapped as user-accessible. Therefore, this fill-in raise a SMAP violation which cause #NPF to be raised to KVM with 0 instruction bytes. >> >> BTW, this also means that in order to trigger this, CR4.SMEP should be set to 0. As otherwise, instruction couldn’t have been executed to raise #NPF in the first place. Maybe we can add this as another condition to recognise the Errata? >> >> -Liran >> >>> because internally CPU is doing a data read from the RIP to get those >>> instruction bytes. Since it hit the SMAP fault hence it was not able >>> to decode the instruction to provide the insn_len. So we are first >>> checking if it was a fault caused from CPL==3 and SMAP is enabled. >>> If so, we are hitting this errata and it can be workaround. >>> >>> -Brijesh >>> >>> >>> >> ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() 2019-07-15 20:30 KVM: SVM: Fix workaround for AMD Errata 1096 Liran Alon 2019-07-15 20:30 ` [PATCH 1/2] " Liran Alon @ 2019-07-15 20:30 ` Liran Alon 2019-07-16 15:48 ` Sean Christopherson 1 sibling, 1 reply; 34+ messages in thread From: Liran Alon @ 2019-07-15 20:30 UTC (permalink / raw) To: pbonzini, rkrcmar, kvm; +Cc: brijesh.singh, Liran Alon, Boris Ostrovsky I think this name is more appropriate to what the x86_ops method does. In addition, modify VMX callback to return true as #PF handler can proceed to emulation in this case. This didn't result in a bug only because the callback is called when DecodeAssist is supported which is currently supported only on SVM. Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- arch/x86/include/asm/kvm_host.h | 3 ++- arch/x86/kvm/mmu.c | 2 +- arch/x86/kvm/svm.c | 4 ++-- arch/x86/kvm/vmx/vmx.c | 6 +++--- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 450d69a1e6fa..536fd56f777d 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1201,7 +1201,8 @@ struct kvm_x86_ops { uint16_t *vmcs_version); uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu); - bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu); + /* Returns true if #PF handler can proceed to emulation */ + bool (*handle_no_insn_on_page_fault)(struct kvm_vcpu *vcpu); }; struct kvm_arch_async_pf { diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 1e9ba81accba..889de3ccf655 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -5423,7 +5423,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code, * guest, with the exception of AMD Erratum 1096 which is unrecoverable. */ if (unlikely(insn && !insn_len)) { - if (!kvm_x86_ops->need_emulation_on_page_fault(vcpu)) + if (!kvm_x86_ops->handle_no_insn_on_page_fault(vcpu)) return 1; } diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 79023a41f7a7..ab89bb0de8df 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -7118,7 +7118,7 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, return -ENODEV; } -static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) +static bool svm_handle_no_insn_on_page_fault(struct kvm_vcpu *vcpu) { bool is_user, smap; @@ -7291,7 +7291,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = { .nested_enable_evmcs = nested_enable_evmcs, .nested_get_evmcs_version = nested_get_evmcs_version, - .need_emulation_on_page_fault = svm_need_emulation_on_page_fault, + .handle_no_insn_on_page_fault = svm_handle_no_insn_on_page_fault, }; static int __init svm_init(void) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index f64bcbb03906..088fc6d943e9 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7419,9 +7419,9 @@ static int enable_smi_window(struct kvm_vcpu *vcpu) return 0; } -static bool vmx_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) +static bool vmx_handle_no_insn_on_page_fault(struct kvm_vcpu *vcpu) { - return 0; + return true; } static __init int hardware_setup(void) @@ -7726,7 +7726,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { .set_nested_state = NULL, .get_vmcs12_pages = NULL, .nested_enable_evmcs = NULL, - .need_emulation_on_page_fault = vmx_need_emulation_on_page_fault, + .handle_no_insn_on_page_fault = vmx_handle_no_insn_on_page_fault, }; static void vmx_cleanup_l1d_flush(void) -- 2.20.1 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() 2019-07-15 20:30 ` [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() Liran Alon @ 2019-07-16 15:48 ` Sean Christopherson 2019-07-16 16:01 ` Liran Alon 0 siblings, 1 reply; 34+ messages in thread From: Sean Christopherson @ 2019-07-16 15:48 UTC (permalink / raw) To: Liran Alon; +Cc: pbonzini, rkrcmar, kvm, brijesh.singh, Boris Ostrovsky On Mon, Jul 15, 2019 at 11:30:43PM +0300, Liran Alon wrote: > I think this name is more appropriate to what the x86_ops method does. > In addition, modify VMX callback to return true as #PF handler can > proceed to emulation in this case. This didn't result in a bug > only because the callback is called when DecodeAssist is supported > which is currently supported only on SVM. > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/include/asm/kvm_host.h | 3 ++- > arch/x86/kvm/mmu.c | 2 +- > arch/x86/kvm/svm.c | 4 ++-- > arch/x86/kvm/vmx/vmx.c | 6 +++--- > 4 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 450d69a1e6fa..536fd56f777d 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1201,7 +1201,8 @@ struct kvm_x86_ops { > uint16_t *vmcs_version); > uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu); > > - bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu); > + /* Returns true if #PF handler can proceed to emulation */ > + bool (*handle_no_insn_on_page_fault)(struct kvm_vcpu *vcpu); The problem with this name is that it requires a comment to explain the boolean return value. The VMX implementation particular would be inscrutuable. "no insn" is also a misnomer, as the AMD quirk has an insn, it's the insn_len that's missing. What about something like force_emulation_on_zero_len_insn()? > }; > > struct kvm_arch_async_pf { > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 1e9ba81accba..889de3ccf655 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -5423,7 +5423,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code, > * guest, with the exception of AMD Erratum 1096 which is unrecoverable. > */ > if (unlikely(insn && !insn_len)) { > - if (!kvm_x86_ops->need_emulation_on_page_fault(vcpu)) > + if (!kvm_x86_ops->handle_no_insn_on_page_fault(vcpu)) > return 1; > } > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 79023a41f7a7..ab89bb0de8df 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -7118,7 +7118,7 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, > return -ENODEV; > } > > -static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) > +static bool svm_handle_no_insn_on_page_fault(struct kvm_vcpu *vcpu) > { > bool is_user, smap; > > @@ -7291,7 +7291,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = { > .nested_enable_evmcs = nested_enable_evmcs, > .nested_get_evmcs_version = nested_get_evmcs_version, > > - .need_emulation_on_page_fault = svm_need_emulation_on_page_fault, > + .handle_no_insn_on_page_fault = svm_handle_no_insn_on_page_fault, > }; > > static int __init svm_init(void) > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index f64bcbb03906..088fc6d943e9 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -7419,9 +7419,9 @@ static int enable_smi_window(struct kvm_vcpu *vcpu) > return 0; > } > > -static bool vmx_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) > +static bool vmx_handle_no_insn_on_page_fault(struct kvm_vcpu *vcpu) > { > - return 0; > + return true; Any functional change here should be done in a different patch. Given that we should never reach this point on VMX, a WARN and triple fault request seems in order. WARN_ON_ONCE(1); kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); return false; > } > > static __init int hardware_setup(void) > @@ -7726,7 +7726,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { > .set_nested_state = NULL, > .get_vmcs12_pages = NULL, > .nested_enable_evmcs = NULL, > - .need_emulation_on_page_fault = vmx_need_emulation_on_page_fault, > + .handle_no_insn_on_page_fault = vmx_handle_no_insn_on_page_fault, > }; > > static void vmx_cleanup_l1d_flush(void) > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() 2019-07-16 15:48 ` Sean Christopherson @ 2019-07-16 16:01 ` Liran Alon 2019-07-16 16:10 ` Sean Christopherson 0 siblings, 1 reply; 34+ messages in thread From: Liran Alon @ 2019-07-16 16:01 UTC (permalink / raw) To: Sean Christopherson Cc: pbonzini, rkrcmar, kvm, brijesh.singh, Boris Ostrovsky > On 16 Jul 2019, at 18:48, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Mon, Jul 15, 2019 at 11:30:43PM +0300, Liran Alon wrote: >> I think this name is more appropriate to what the x86_ops method does. >> In addition, modify VMX callback to return true as #PF handler can >> proceed to emulation in this case. This didn't result in a bug >> only because the callback is called when DecodeAssist is supported >> which is currently supported only on SVM. >> >> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> >> --- >> arch/x86/include/asm/kvm_host.h | 3 ++- >> arch/x86/kvm/mmu.c | 2 +- >> arch/x86/kvm/svm.c | 4 ++-- >> arch/x86/kvm/vmx/vmx.c | 6 +++--- >> 4 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> index 450d69a1e6fa..536fd56f777d 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -1201,7 +1201,8 @@ struct kvm_x86_ops { >> uint16_t *vmcs_version); >> uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu); >> >> - bool (*need_emulation_on_page_fault)(struct kvm_vcpu *vcpu); >> + /* Returns true if #PF handler can proceed to emulation */ >> + bool (*handle_no_insn_on_page_fault)(struct kvm_vcpu *vcpu); > > The problem with this name is that it requires a comment to explain the > boolean return value. The VMX implementation particular would be > inscrutuable. True. > > "no insn" is also a misnomer, as the AMD quirk has an insn, it's the > insn_len that's missing. This could in theory also happen for VMX if it ever implements DecodeAssist style feature. So this name is still kinda makes sense in the generic x86 level. > > What about something like force_emulation_on_zero_len_insn()? I have no objection to such name besides the fact that it seems to state that the callback have read-only boolean semantic. Which is not true as the SVM implementation could also for example, trigger a triple-fault and change vCPU state. This is why I renamed to handle_*... > >> }; >> >> struct kvm_arch_async_pf { >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c >> index 1e9ba81accba..889de3ccf655 100644 >> --- a/arch/x86/kvm/mmu.c >> +++ b/arch/x86/kvm/mmu.c >> @@ -5423,7 +5423,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code, >> * guest, with the exception of AMD Erratum 1096 which is unrecoverable. >> */ >> if (unlikely(insn && !insn_len)) { >> - if (!kvm_x86_ops->need_emulation_on_page_fault(vcpu)) >> + if (!kvm_x86_ops->handle_no_insn_on_page_fault(vcpu)) >> return 1; >> } >> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index 79023a41f7a7..ab89bb0de8df 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -7118,7 +7118,7 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, >> return -ENODEV; >> } >> >> -static bool svm_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) >> +static bool svm_handle_no_insn_on_page_fault(struct kvm_vcpu *vcpu) >> { >> bool is_user, smap; >> >> @@ -7291,7 +7291,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = { >> .nested_enable_evmcs = nested_enable_evmcs, >> .nested_get_evmcs_version = nested_get_evmcs_version, >> >> - .need_emulation_on_page_fault = svm_need_emulation_on_page_fault, >> + .handle_no_insn_on_page_fault = svm_handle_no_insn_on_page_fault, >> }; >> >> static int __init svm_init(void) >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index f64bcbb03906..088fc6d943e9 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -7419,9 +7419,9 @@ static int enable_smi_window(struct kvm_vcpu *vcpu) >> return 0; >> } >> >> -static bool vmx_need_emulation_on_page_fault(struct kvm_vcpu *vcpu) >> +static bool vmx_handle_no_insn_on_page_fault(struct kvm_vcpu *vcpu) >> { >> - return 0; >> + return true; > > Any functional change here should be done in a different patch. I originally done so and don’t regretted as it depends on what is the semantic definition of the boolean return value. That’s why I preferred to do so in same patch. But I don’t have strong objection for separating it out to a different patch. > > Given that we should never reach this point on VMX, a WARN and triple > fault request seems in order. > > WARN_ON_ONCE(1); I agree we should add here such a WARN_ON(). Makes sense. > > kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); > return false; I don’t think we should triple-fault and return “false”. As from a semantic perspective, we should return true. But this commit is getting really philosophical :) Maybe let’s hear Paolo’s preference first before doing any change. -Liran > >> } >> >> static __init int hardware_setup(void) >> @@ -7726,7 +7726,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = { >> .set_nested_state = NULL, >> .get_vmcs12_pages = NULL, >> .nested_enable_evmcs = NULL, >> - .need_emulation_on_page_fault = vmx_need_emulation_on_page_fault, >> + .handle_no_insn_on_page_fault = vmx_handle_no_insn_on_page_fault, >> }; >> >> static void vmx_cleanup_l1d_flush(void) >> -- >> 2.20.1 >> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() 2019-07-16 16:01 ` Liran Alon @ 2019-07-16 16:10 ` Sean Christopherson 2019-07-16 19:33 ` Singh, Brijesh 0 siblings, 1 reply; 34+ messages in thread From: Sean Christopherson @ 2019-07-16 16:10 UTC (permalink / raw) To: Liran Alon; +Cc: pbonzini, rkrcmar, kvm, brijesh.singh, Boris Ostrovsky On Tue, Jul 16, 2019 at 07:01:30PM +0300, Liran Alon wrote: > > > On 16 Jul 2019, at 18:48, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); > > return false; > > I don’t think we should triple-fault and return “false”. As from a semantic > perspective, we should return true. Fair enough, I guess it's no different than the warn-and-continue logic used in the unreachable VM-Exit handlers. > But this commit is getting really philosophical :) Maybe let’s hear Paolo’s > preference first before doing any change. Hence my recommendation to put the function change in a separate patch :-) ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() 2019-07-16 16:10 ` Sean Christopherson @ 2019-07-16 19:33 ` Singh, Brijesh 0 siblings, 0 replies; 34+ messages in thread From: Singh, Brijesh @ 2019-07-16 19:33 UTC (permalink / raw) To: Sean Christopherson, Liran Alon Cc: Singh, Brijesh, pbonzini, rkrcmar, kvm, Boris Ostrovsky On 7/16/19 11:10 AM, Sean Christopherson wrote: > On Tue, Jul 16, 2019 at 07:01:30PM +0300, Liran Alon wrote: >> >>> On 16 Jul 2019, at 18:48, Sean Christopherson <sean.j.christopherson@intel.com> wrote: >>> kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); >>> return false; >> >> I don’t think we should triple-fault and return “false”. As from a semantic >> perspective, we should return true. > > Fair enough, I guess it's no different than the warn-and-continue logic > used in the unreachable VM-Exit handlers. > >> But this commit is getting really philosophical :) Maybe let’s hear Paolo’s >> preference first before doing any change. > > Hence my recommendation to put the function change in a separate patch :-) > Well, during the initial patch we had some discussion about the function names. At that time we all felt the name was okay. I don't have any strong preference. Lets go with whatever everyone agrees to :) -Brijesh ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2019-07-16 21:54 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-15 20:30 KVM: SVM: Fix workaround for AMD Errata 1096 Liran Alon 2019-07-15 20:30 ` [PATCH 1/2] " Liran Alon 2019-07-16 15:48 ` Singh, Brijesh 2019-07-16 15:56 ` Liran Alon 2019-07-16 16:07 ` Liran Alon 2019-07-16 16:10 ` Singh, Brijesh 2019-07-16 16:20 ` Liran Alon 2019-07-16 16:41 ` Sean Christopherson 2019-07-16 16:56 ` Liran Alon 2019-07-16 17:27 ` Sean Christopherson 2019-07-16 17:27 ` Paolo Bonzini 2019-07-16 17:35 ` Liran Alon 2019-07-16 19:28 ` Singh, Brijesh 2019-07-16 19:34 ` Liran Alon 2019-07-16 19:39 ` Paolo Bonzini 2019-07-16 19:45 ` Sean Christopherson 2019-07-16 19:50 ` Liran Alon 2019-07-16 19:47 ` Liran Alon 2019-07-16 19:41 ` Sean Christopherson 2019-07-16 19:52 ` Liran Alon 2019-07-16 20:02 ` Singh, Brijesh 2019-07-16 20:07 ` Sean Christopherson 2019-07-16 20:13 ` Paolo Bonzini 2019-07-16 20:09 ` Liran Alon 2019-07-16 20:27 ` Singh, Brijesh 2019-07-16 20:54 ` Sean Christopherson 2019-07-16 21:53 ` Liran Alon 2019-07-16 18:05 ` Singh, Brijesh 2019-07-16 18:06 ` Singh, Brijesh 2019-07-15 20:30 ` [PATCH 2/2] KVM: x86: Rename need_emulation_on_page_fault() to handle_no_insn_on_page_fault() Liran Alon 2019-07-16 15:48 ` Sean Christopherson 2019-07-16 16:01 ` Liran Alon 2019-07-16 16:10 ` Sean Christopherson 2019-07-16 19:33 ` Singh, Brijesh
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).