| Message ID | 20190226170201.24246-1-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] KVM: SVM: Workaround errata#1096 (insn_len maybe zero on SMAP violation) | expand |
On Tue, Feb 26, 2019 at 9:02 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: > > Errata#1096: > > On a nested data page fault when CR.SMAP=1 and the guest data read > generates a SMAP violation, GuestInstrBytes field of the VMCB on a > VMEXIT will incorrectly return 0h instead the correct guest > instruction bytes . > > Recommend Workaround: > > To determine what instruction the guest was executing the hypervisor > will have to decode the instruction at the instruction pointer. > > The recommended workaround can not be implemented for the SEV > guest because guest memory is encrypted with the guest specific key, > and instruction decoder will not be able to decode the instruction > bytes. If we hit this errata in the SEV guest then log the message > and request a guest shutdown. > > Reported-by: Venkatesh Srinivas <venkateshs@google.com> > Cc: Jim Mattson <jmattson@google.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Joerg Roedel <joro@8bytes.org> > Cc: "Radim Krčmář" <rkrcmar@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > > Change since v2: > * rename the callback emulate_instruction_possible->need_emulation_on_page_fault This function still seems poorly named. You already know that you *need* emulation by the time you call it, don't you?
On 2/26/19 11:12 AM, Jim Mattson wrote: > On Tue, Feb 26, 2019 at 9:02 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: >> >> Errata#1096: >> >> On a nested data page fault when CR.SMAP=1 and the guest data read >> generates a SMAP violation, GuestInstrBytes field of the VMCB on a >> VMEXIT will incorrectly return 0h instead the correct guest >> instruction bytes . >> >> Recommend Workaround: >> >> To determine what instruction the guest was executing the hypervisor >> will have to decode the instruction at the instruction pointer. >> >> The recommended workaround can not be implemented for the SEV >> guest because guest memory is encrypted with the guest specific key, >> and instruction decoder will not be able to decode the instruction >> bytes. If we hit this errata in the SEV guest then log the message >> and request a guest shutdown. >> >> Reported-by: Venkatesh Srinivas <venkateshs@google.com> >> Cc: Jim Mattson <jmattson@google.com> >> Cc: Tom Lendacky <thomas.lendacky@amd.com> >> Cc: Borislav Petkov <bp@alien8.de> >> Cc: Joerg Roedel <joro@8bytes.org> >> Cc: "Radim Krčmář" <rkrcmar@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> >> Change since v2: >> * rename the callback emulate_instruction_possible->need_emulation_on_page_fault > > This function still seems poorly named. You already know that you > *need* emulation by the time you call it, don't you? > We know that we are going to require emulation to handle this #PF. How about can_emulate_on_page_fault(..) ? Any other suggestions ? -Brijesh
On Tue, Feb 26, 2019 at 10:17 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: > > > > On 2/26/19 11:12 AM, Jim Mattson wrote: > > On Tue, Feb 26, 2019 at 9:02 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: > >> > >> Errata#1096: > >> > >> On a nested data page fault when CR.SMAP=1 and the guest data read > >> generates a SMAP violation, GuestInstrBytes field of the VMCB on a > >> VMEXIT will incorrectly return 0h instead the correct guest > >> instruction bytes . > >> > >> Recommend Workaround: > >> > >> To determine what instruction the guest was executing the hypervisor > >> will have to decode the instruction at the instruction pointer. > >> > >> The recommended workaround can not be implemented for the SEV > >> guest because guest memory is encrypted with the guest specific key, > >> and instruction decoder will not be able to decode the instruction > >> bytes. If we hit this errata in the SEV guest then log the message > >> and request a guest shutdown. > >> > >> Reported-by: Venkatesh Srinivas <venkateshs@google.com> > >> Cc: Jim Mattson <jmattson@google.com> > >> Cc: Tom Lendacky <thomas.lendacky@amd.com> > >> Cc: Borislav Petkov <bp@alien8.de> > >> Cc: Joerg Roedel <joro@8bytes.org> > >> Cc: "Radim Krčmář" <rkrcmar@redhat.com> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > >> --- > >> > >> Change since v2: > >> * rename the callback emulate_instruction_possible->need_emulation_on_page_fault > > > > This function still seems poorly named. You already know that you > > *need* emulation by the time you call it, don't you? > > > > > We know that we are going to require emulation to handle this #PF. > > How about can_emulate_on_page_fault(..) ? Any other suggestions ? Isn't "can_emulate_on_page_fault()" exactly the same as "!sev_guest()"? The function in question also returns false when CPL != 3 or CR4.SMAP is clear. I think it's difficult to name this function because the function is essentially answering two unrelated questions: 1) Did we encounter erratum 1096? 2) Can we emulate an instruction to make forward progress? I would suggest that this be broken up into two separate functions: one which determines whether or not we've encountered erratum 1096 and another which determines whether or not SEV is enabled. I suspect that a function to answer the SEV question on its own is going to prove itself quite useful over time.
On 2/26/19 1:04 PM, Jim Mattson wrote: > On Tue, Feb 26, 2019 at 10:17 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: >> >> >> >> On 2/26/19 11:12 AM, Jim Mattson wrote: >>> On Tue, Feb 26, 2019 at 9:02 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: >>>> >>>> Errata#1096: >>>> >>>> On a nested data page fault when CR.SMAP=1 and the guest data read >>>> generates a SMAP violation, GuestInstrBytes field of the VMCB on a >>>> VMEXIT will incorrectly return 0h instead the correct guest >>>> instruction bytes . >>>> >>>> Recommend Workaround: >>>> >>>> To determine what instruction the guest was executing the hypervisor >>>> will have to decode the instruction at the instruction pointer. >>>> >>>> The recommended workaround can not be implemented for the SEV >>>> guest because guest memory is encrypted with the guest specific key, >>>> and instruction decoder will not be able to decode the instruction >>>> bytes. If we hit this errata in the SEV guest then log the message >>>> and request a guest shutdown. >>>> >>>> Reported-by: Venkatesh Srinivas <venkateshs@google.com> >>>> Cc: Jim Mattson <jmattson@google.com> >>>> Cc: Tom Lendacky <thomas.lendacky@amd.com> >>>> Cc: Borislav Petkov <bp@alien8.de> >>>> Cc: Joerg Roedel <joro@8bytes.org> >>>> Cc: "Radim Krčmář" <rkrcmar@redhat.com> >>>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >>>> --- >>>> >>>> Change since v2: >>>> * rename the callback emulate_instruction_possible->need_emulation_on_page_fault >>> >>> This function still seems poorly named. You already know that you >>> *need* emulation by the time you call it, don't you? >>> >> >> >> We know that we are going to require emulation to handle this #PF. >> >> How about can_emulate_on_page_fault(..) ? Any other suggestions ? > > Isn't "can_emulate_on_page_fault()" exactly the same as > "!sev_guest()"? It may seem like that, but in common handlers whenever possible I am trying to avoid if(sev_guest() /* do this */ else /* do that */. If insn_len is zero then retry the instruction whether SEV is enabled or not. The function in question also returns false when CPL > != 3 or CR4.SMAP is clear. I think it's difficult to name this > function because the function is essentially answering two unrelated > questions: > > 1) Did we encounter erratum 1096? > 2) Can we emulate an instruction to make forward progress? > > I would suggest that this be broken up into two separate functions: > one which determines whether or not we've encountered erratum 1096 and > another which determines whether or not SEV is enabled. I suspect that > a function to answer the SEV question on its own is going to prove > itself quite useful over time. > Well, I have been trying not to expose SVM specific functions to the common layer. If we go with this approach then a generic page fault handler will look like this: kvm_mmu_page_fault(...) { .... .... if (kvm_x86->svm_erratum_1096(..)) .... if (kvm_x86->svm_sev_guest(...)) ... } Please correct me if I misunderstood you.
On Tue, Feb 26, 2019 at 11:50 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: > > > > On 2/26/19 1:04 PM, Jim Mattson wrote: > > On Tue, Feb 26, 2019 at 10:17 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: > >> > >> > >> > >> On 2/26/19 11:12 AM, Jim Mattson wrote: > >>> On Tue, Feb 26, 2019 at 9:02 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: > >>>> > >>>> Errata#1096: > >>>> > >>>> On a nested data page fault when CR.SMAP=1 and the guest data read > >>>> generates a SMAP violation, GuestInstrBytes field of the VMCB on a > >>>> VMEXIT will incorrectly return 0h instead the correct guest > >>>> instruction bytes . > >>>> > >>>> Recommend Workaround: > >>>> > >>>> To determine what instruction the guest was executing the hypervisor > >>>> will have to decode the instruction at the instruction pointer. > >>>> > >>>> The recommended workaround can not be implemented for the SEV > >>>> guest because guest memory is encrypted with the guest specific key, > >>>> and instruction decoder will not be able to decode the instruction > >>>> bytes. If we hit this errata in the SEV guest then log the message > >>>> and request a guest shutdown. > >>>> > >>>> Reported-by: Venkatesh Srinivas <venkateshs@google.com> > >>>> Cc: Jim Mattson <jmattson@google.com> > >>>> Cc: Tom Lendacky <thomas.lendacky@amd.com> > >>>> Cc: Borislav Petkov <bp@alien8.de> > >>>> Cc: Joerg Roedel <joro@8bytes.org> > >>>> Cc: "Radim Krčmář" <rkrcmar@redhat.com> > >>>> Cc: Paolo Bonzini <pbonzini@redhat.com> > >>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > >>>> --- > >>>> > >>>> Change since v2: > >>>> * rename the callback emulate_instruction_possible->need_emulation_on_page_fault > >>> > >>> This function still seems poorly named. You already know that you > >>> *need* emulation by the time you call it, don't you? > >>> > >> > >> > >> We know that we are going to require emulation to handle this #PF. > >> > >> How about can_emulate_on_page_fault(..) ? Any other suggestions ? > > > > Isn't "can_emulate_on_page_fault()" exactly the same as > > "!sev_guest()"? > > > It may seem like that, but in common handlers whenever possible I am > trying to avoid if(sev_guest() /* do this */ else /* do that */. > > If insn_len is zero then retry the instruction whether SEV is enabled or > not. > > > The function in question also returns false when CPL > > != 3 or CR4.SMAP is clear. I think it's difficult to name this > > function because the function is essentially answering two unrelated > > questions: > > > > 1) Did we encounter erratum 1096? > > 2) Can we emulate an instruction to make forward progress? > > > > I would suggest that this be broken up into two separate functions: > > one which determines whether or not we've encountered erratum 1096 and > > another which determines whether or not SEV is enabled. I suspect that > > a function to answer the SEV question on its own is going to prove > > itself quite useful over time. > > > > > Well, I have been trying not to expose SVM specific functions to the > common layer. If we go with this approach then a generic page fault > handler will look like this: > > kvm_mmu_page_fault(...) > { > .... > .... > if (kvm_x86->svm_erratum_1096(..)) I think the real question here is whether or not we will go into an infinite loop if we simply try to re-enter the guest and re-execute the instruction. You don't necessarily have to reference the erratum. > .... > if (kvm_x86->svm_sev_guest(...)) For this one, you can always use your previous name, "can_emulate_on_page_fault()," and negate the return value. > ... > > } > > Please correct me if I misunderstood you.
On 2/26/19 2:21 PM, Jim Mattson wrote: > On Tue, Feb 26, 2019 at 11:50 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: >> >> >> On 2/26/19 1:04 PM, Jim Mattson wrote: >>> On Tue, Feb 26, 2019 at 10:17 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: >>>> >>>> >>>> On 2/26/19 11:12 AM, Jim Mattson wrote: >>>>> On Tue, Feb 26, 2019 at 9:02 AM Singh, Brijesh <brijesh.singh@amd.com> wrote: >>>>>> Errata#1096: >>>>>> >>>>>> On a nested data page fault when CR.SMAP=1 and the guest data read >>>>>> generates a SMAP violation, GuestInstrBytes field of the VMCB on a >>>>>> VMEXIT will incorrectly return 0h instead the correct guest >>>>>> instruction bytes . >>>>>> >>>>>> Recommend Workaround: >>>>>> >>>>>> To determine what instruction the guest was executing the hypervisor >>>>>> will have to decode the instruction at the instruction pointer. >>>>>> >>>>>> The recommended workaround can not be implemented for the SEV >>>>>> guest because guest memory is encrypted with the guest specific key, >>>>>> and instruction decoder will not be able to decode the instruction >>>>>> bytes. If we hit this errata in the SEV guest then log the message >>>>>> and request a guest shutdown. >>>>>> >>>>>> Reported-by: Venkatesh Srinivas <venkateshs@google.com> >>>>>> Cc: Jim Mattson <jmattson@google.com> >>>>>> Cc: Tom Lendacky <thomas.lendacky@amd.com> >>>>>> Cc: Borislav Petkov <bp@alien8.de> >>>>>> Cc: Joerg Roedel <joro@8bytes.org> >>>>>> Cc: "Radim Krčmář" <rkrcmar@redhat.com> >>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >>>>>> --- >>>>>> >>>>>> Change since v2: >>>>>> * rename the callback emulate_instruction_possible->need_emulation_on_page_fault >>>>> This function still seems poorly named. You already know that you >>>>> *need* emulation by the time you call it, don't you? >>>>> >>>> >>>> We know that we are going to require emulation to handle this #PF. >>>> >>>> How about can_emulate_on_page_fault(..) ? Any other suggestions ? >>> Isn't "can_emulate_on_page_fault()" exactly the same as >>> "!sev_guest()"? >> >> It may seem like that, but in common handlers whenever possible I am >> trying to avoid if(sev_guest() /* do this */ else /* do that */. >> >> If insn_len is zero then retry the instruction whether SEV is enabled or >> not. >> >> >> The function in question also returns false when CPL >>> != 3 or CR4.SMAP is clear. I think it's difficult to name this >>> function because the function is essentially answering two unrelated >>> questions: >>> >>> 1) Did we encounter erratum 1096? >>> 2) Can we emulate an instruction to make forward progress? >>> >>> I would suggest that this be broken up into two separate functions: >>> one which determines whether or not we've encountered erratum 1096 and >>> another which determines whether or not SEV is enabled. I suspect that >>> a function to answer the SEV question on its own is going to prove >>> itself quite useful over time. >>> >> >> Well, I have been trying not to expose SVM specific functions to the >> common layer. If we go with this approach then a generic page fault >> handler will look like this: >> >> kvm_mmu_page_fault(...) >> { >> .... >> .... >> if (kvm_x86->svm_erratum_1096(..)) > I think the real question here is whether or not we will go into an > infinite loop if we simply try to re-enter the guest and re-execute > the instruction. You don't necessarily have to reference the erratum. > We will get into infinite loop for both the SEV and non SEV case. In case of SEV, we will need to log the message and request the SHUTDOWN whereas in non SEV we will need to workaround it. The check should be something like if (!insn_len) { if (erratum_1096()) { if (sev_guest()) /* log the message, request SHUTDOWN and re-enter the guest */ else /* goto read_the_instruction_bytes */ } /* re-enter the guest and re-execute the instruction */ } The need_emulate_on_page_fault() was taking care of the above checks. IMHO, a one callback in svm.c is better than modifying the generic page_fault handler to handle the erratum.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 180373360e34..f913d21dafce 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1196,6 +1196,8 @@ struct kvm_x86_ops { int (*nested_enable_evmcs)(struct kvm_vcpu *vcpu, uint16_t *vmcs_version); uint16_t (*nested_get_evmcs_version)(struct kvm_vcpu *vcpu); + + bool (*need_emulation_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 f2d1d230d5b8..68672bd756b5 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -5386,14 +5386,13 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u64 error_code, emulation_type = EMULTYPE_ALLOW_RETRY; emulate: /* - * On AMD platforms, under certain conditions insn_len may be zero on #NPF. - * This can happen if a guest gets a page-fault on data access but the HW - * table walker is not able to read the instruction page (e.g instruction - * page is not present in memory). In those cases we simply restart the - * guest. + * On AMD platform, under certain conditions insn_len may be zero on + * #NPF. Lets check if its safe to emulate the instruction. */ - if (unlikely(insn && !insn_len)) - return 1; + if (unlikely(insn && !insn_len)) { + if (!kvm_x86_ops->need_emulation_on_page_fault(vcpu)) + return 1; + } er = x86_emulate_instruction(vcpu, cr2, emulation_type, insn, insn_len); diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index f13a3a24d360..4cc4d556e852 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -7098,6 +7098,44 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, return -ENODEV; } +static bool 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); + + /* + * Detect and workaround Errata 1096 Fam_17h_00_0Fh + * + * In non SEV guest, hypervisor will be able to read the guest + * memory to decode the instruction pointer when insn_len is zero + * so we return true to indicate that decoding is possible. + * + * But in the SEV guest, the guest memory is encrypted with the + * guest specific key and hypervisor will not be able to decode the + * 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 (!sev_guest(vcpu->kvm)) + return true; + + pr_err_ratelimited("KVM: Guest triggered AMD Erratum 1096\n"); + kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu); + } + + /* + * Under certain conditions insn_len may be zero on #NPF. This can + * happen if a guest gets a page-fault on data access but the HW table + * walker is not able to read the instruction page (e.g instruction + * page is not present in memory). In those cases we return false + * indicating the emulation is not possible and request to retry the + * execution. + */ + return false; +} + static struct kvm_x86_ops svm_x86_ops __ro_after_init = { .cpu_has_kvm_support = has_svm, .disabled_by_bios = is_disabled, @@ -7231,6 +7269,8 @@ 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 = need_emulation_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 30a6bcd735ec..25940feddec5 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7527,6 +7527,11 @@ static int enable_smi_window(struct kvm_vcpu *vcpu) return 0; } +static bool need_emulation_on_page_fault(struct kvm_vcpu *vcpu) +{ + return false; +} + static __init int hardware_setup(void) { unsigned long host_bndcfgs; @@ -7829,6 +7834,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 = need_emulation_on_page_fault, }; static void vmx_cleanup_l1d_flush(void)
Errata#1096: On a nested data page fault when CR.SMAP=1 and the guest data read generates a SMAP violation, GuestInstrBytes field of the VMCB on a VMEXIT will incorrectly return 0h instead the correct guest instruction bytes . Recommend Workaround: To determine what instruction the guest was executing the hypervisor will have to decode the instruction at the instruction pointer. The recommended workaround can not be implemented for the SEV guest because guest memory is encrypted with the guest specific key, and instruction decoder will not be able to decode the instruction bytes. If we hit this errata in the SEV guest then log the message and request a guest shutdown. Reported-by: Venkatesh Srinivas <venkateshs@google.com> Cc: Jim Mattson <jmattson@google.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Joerg Roedel <joro@8bytes.org> Cc: "Radim Krčmář" <rkrcmar@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- Change since v2: * rename the callback emulate_instruction_possible->need_emulation_on_page_fault Changes since v1: * request to shutdown the guest instead of injecting #GP * use the pr_err_ratelimited to supress the host dmesg arch/x86/include/asm/kvm_host.h | 2 ++ arch/x86/kvm/mmu.c | 13 +++++------ arch/x86/kvm/svm.c | 40 +++++++++++++++++++++++++++++++++ arch/x86/kvm/vmx/vmx.c | 6 +++++ 4 files changed, 54 insertions(+), 7 deletions(-)