| Message ID | 20210430123822.13825-37-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand |
> > +static void snp_handle_guest_request(struct vcpu_svm *svm, struct ghcb *ghcb, > + gpa_t req_gpa, gpa_t resp_gpa) > +{ > + struct sev_data_snp_guest_request data = {}; > + struct kvm_vcpu *vcpu = &svm->vcpu; > + struct kvm *kvm = vcpu->kvm; > + kvm_pfn_t req_pfn, resp_pfn; > + struct kvm_sev_info *sev; > + int rc, err = 0; > + > + if (!sev_snp_guest(vcpu->kvm)) { > + rc = -ENODEV; > + goto e_fail; > + } > + > + sev = &to_kvm_svm(kvm)->sev_info; > + > + if (!__ratelimit(&sev->snp_guest_msg_rs)) { > + pr_info_ratelimited("svm: too many guest message requests\n"); > + rc = -EAGAIN; > + goto e_fail; > + } > + > + if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, PAGE_SIZE)) { > + pr_err("svm: guest request (%#llx) or response (%#llx) is not page aligned\n", > + req_gpa, resp_gpa); > + goto e_term; > + } > + > + req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa)); > + if (is_error_noslot_pfn(req_pfn)) { > + pr_err("svm: guest request invalid gpa=%#llx\n", req_gpa); > + goto e_term; > + } > + > + resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa)); > + if (is_error_noslot_pfn(resp_pfn)) { > + pr_err("svm: guest response invalid gpa=%#llx\n", resp_gpa); > + goto e_term; > + } > + > + data.gctx_paddr = __psp_pa(sev->snp_context); > + data.req_paddr = __sme_set(req_pfn << PAGE_SHIFT); > + data.res_paddr = __psp_pa(sev->snp_resp_page); > + > + mutex_lock(&kvm->lock); > + > + rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err); > + if (rc) { > + mutex_unlock(&kvm->lock); > + > + /* If we have a firmware error code then use it. */ > + if (err) > + rc = err; > + > + goto e_fail; > + } > + > + /* Copy the response after the firmware returns success. */ > + rc = kvm_write_guest(kvm, resp_gpa, sev->snp_resp_page, PAGE_SIZE); > + > + mutex_unlock(&kvm->lock); > + > +e_fail: > + ghcb_set_sw_exit_info_2(ghcb, rc); > + return; > + > +e_term: > + ghcb_set_sw_exit_info_1(ghcb, 1); > + ghcb_set_sw_exit_info_2(ghcb, > + X86_TRAP_GP | > + SVM_EVTINJ_TYPE_EXEPT | > + SVM_EVTINJ_VALID); > +} I am probably missing something in the spec but I don't see any references to #GP in the '4.1.7 SNP Guest Request' section. Why is this different from e_fail?
On 5/10/21 1:57 PM, Peter Gonda wrote: >> +static void snp_handle_guest_request(struct vcpu_svm *svm, struct ghcb *ghcb, >> + gpa_t req_gpa, gpa_t resp_gpa) >> +{ >> + struct sev_data_snp_guest_request data = {}; >> + struct kvm_vcpu *vcpu = &svm->vcpu; >> + struct kvm *kvm = vcpu->kvm; >> + kvm_pfn_t req_pfn, resp_pfn; >> + struct kvm_sev_info *sev; >> + int rc, err = 0; >> + >> + if (!sev_snp_guest(vcpu->kvm)) { >> + rc = -ENODEV; >> + goto e_fail; >> + } >> + >> + sev = &to_kvm_svm(kvm)->sev_info; >> + >> + if (!__ratelimit(&sev->snp_guest_msg_rs)) { >> + pr_info_ratelimited("svm: too many guest message requests\n"); >> + rc = -EAGAIN; >> + goto e_fail; >> + } >> + >> + if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, PAGE_SIZE)) { >> + pr_err("svm: guest request (%#llx) or response (%#llx) is not page aligned\n", >> + req_gpa, resp_gpa); >> + goto e_term; >> + } >> + >> + req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa)); >> + if (is_error_noslot_pfn(req_pfn)) { >> + pr_err("svm: guest request invalid gpa=%#llx\n", req_gpa); >> + goto e_term; >> + } >> + >> + resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa)); >> + if (is_error_noslot_pfn(resp_pfn)) { >> + pr_err("svm: guest response invalid gpa=%#llx\n", resp_gpa); >> + goto e_term; >> + } >> + >> + data.gctx_paddr = __psp_pa(sev->snp_context); >> + data.req_paddr = __sme_set(req_pfn << PAGE_SHIFT); >> + data.res_paddr = __psp_pa(sev->snp_resp_page); >> + >> + mutex_lock(&kvm->lock); >> + >> + rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err); >> + if (rc) { >> + mutex_unlock(&kvm->lock); >> + >> + /* If we have a firmware error code then use it. */ >> + if (err) >> + rc = err; >> + >> + goto e_fail; >> + } >> + >> + /* Copy the response after the firmware returns success. */ >> + rc = kvm_write_guest(kvm, resp_gpa, sev->snp_resp_page, PAGE_SIZE); >> + >> + mutex_unlock(&kvm->lock); >> + >> +e_fail: >> + ghcb_set_sw_exit_info_2(ghcb, rc); >> + return; >> + >> +e_term: >> + ghcb_set_sw_exit_info_1(ghcb, 1); >> + ghcb_set_sw_exit_info_2(ghcb, >> + X86_TRAP_GP | >> + SVM_EVTINJ_TYPE_EXEPT | >> + SVM_EVTINJ_VALID); >> +} > I am probably missing something in the spec but I don't see any > references to #GP in the '4.1.7 SNP Guest Request' section. Why is > this different from e_fail? The spec does not say to inject the #GP, I chose this because guest is not adhering to the spec and there was a not a good error code in the GHCB spec to communicate this condition. Per the spec, both the request and response page must be a valid GPA. If we detect that guest is not following the spec then its a guest BUG. IIRC, other places in the KVM does something similar when guest is trying invalid operation. -Brijesh
On Mon, May 10, 2021, Brijesh Singh wrote: > > On 5/10/21 1:57 PM, Peter Gonda wrote: > >> +e_fail: > >> + ghcb_set_sw_exit_info_2(ghcb, rc); > >> + return; > >> + > >> +e_term: > >> + ghcb_set_sw_exit_info_1(ghcb, 1); > >> + ghcb_set_sw_exit_info_2(ghcb, > >> + X86_TRAP_GP | > >> + SVM_EVTINJ_TYPE_EXEPT | > >> + SVM_EVTINJ_VALID); > >> +} > > I am probably missing something in the spec but I don't see any > > references to #GP in the '4.1.7 SNP Guest Request' section. Why is > > this different from e_fail? > > The spec does not say to inject the #GP, I chose this because guest is > not adhering to the spec and there was a not a good error code in the > GHCB spec to communicate this condition. Per the spec, both the request > and response page must be a valid GPA. If we detect that guest is not > following the spec then its a guest BUG. IIRC, other places in the KVM > does something similar when guest is trying invalid operation. The GHCB spec should be updated to define an error code, even if it's a blanket statement for all VMGEXITs that fail to adhere to the spec. Arbitrarily choosing an error code and/or exception number makes the information useless to the guest because the guest can't take specific action for those failures. E.g. if there is a return code specifically for GHCB spec violation, then the guest can panic, WARN, etc... knowing that it done messed up. "Injecting" an exception is particularly bad, because if the guest kernel takes that request literally and emulates a #GP, then we can end up in a situation where someone files a bug report because VMGEXIT is hitting a #GP and confusion ensues.
On 5/10/21 4:17 PM, Sean Christopherson wrote: > On Mon, May 10, 2021, Brijesh Singh wrote: >> On 5/10/21 1:57 PM, Peter Gonda wrote: >>>> +e_fail: >>>> + ghcb_set_sw_exit_info_2(ghcb, rc); >>>> + return; >>>> + >>>> +e_term: >>>> + ghcb_set_sw_exit_info_1(ghcb, 1); >>>> + ghcb_set_sw_exit_info_2(ghcb, >>>> + X86_TRAP_GP | >>>> + SVM_EVTINJ_TYPE_EXEPT | >>>> + SVM_EVTINJ_VALID); >>>> +} >>> I am probably missing something in the spec but I don't see any >>> references to #GP in the '4.1.7 SNP Guest Request' section. Why is >>> this different from e_fail? >> The spec does not say to inject the #GP, I chose this because guest is >> not adhering to the spec and there was a not a good error code in the >> GHCB spec to communicate this condition. Per the spec, both the request >> and response page must be a valid GPA. If we detect that guest is not >> following the spec then its a guest BUG. IIRC, other places in the KVM >> does something similar when guest is trying invalid operation. > The GHCB spec should be updated to define an error code, even if it's a blanket > statement for all VMGEXITs that fail to adhere to the spec. Arbitrarily choosing > an error code and/or exception number makes the information useless to the guest > because the guest can't take specific action for those failures. E.g. if there > is a return code specifically for GHCB spec violation, then the guest can panic, > WARN, etc... knowing that it done messed up. The GHCB is finalized and released so I don't think it will be covered in v2. But I will go ahead and file the report so that it is considered in the next updates. Having said that, I do see that for other commands (e.g, HV door bell page) the spec spell out that if guest provides an invalid GPA then inject a #GP. I guess we need to move that statement and apply to all the commands. Until then I am fine with not injecting #GP to not divert from the spec. > "Injecting" an exception is particularly bad, because if the guest kernel takes > that request literally and emulates a #GP, then we can end up in a situation > where someone files a bug report because VMGEXIT is hitting a #GP and confusion > ensues.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 8e4783e105b9..da4158da644b 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -18,6 +18,7 @@ #include <linux/processor.h> #include <linux/trace_events.h> #include <linux/sev.h> +#include <linux/kvm_host.h> #include <asm/fpu/internal.h> #include <asm/trapnr.h> @@ -1517,6 +1518,7 @@ static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp) static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp) { + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; struct sev_data_snp_gctx_create data = {}; void *context; int rc; @@ -1526,14 +1528,24 @@ static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp) if (!context) return NULL; - data.gctx_paddr = __psp_pa(context); - rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error); - if (rc) { + /* Allocate a firmware buffer used during the guest command handling. */ + sev->snp_resp_page = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT); + if (!sev->snp_resp_page) { snp_free_firmware_page(context); return NULL; } + data.gctx_paddr = __psp_pa(context); + rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error); + if (rc) + goto e_free; + return context; + +e_free: + snp_free_firmware_page(context); + snp_free_firmware_page(sev->snp_resp_page); + return NULL; } static int snp_bind_asid(struct kvm *kvm, int *error) @@ -1601,6 +1613,9 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) if (rc) goto e_free_context; + /* Used for rate limiting SNP guest message request, use the default settings */ + ratelimit_default_init(&sev->snp_guest_msg_rs); + return 0; e_free_context: @@ -2197,6 +2212,9 @@ static int snp_decommission_context(struct kvm *kvm) snp_free_firmware_page(sev->snp_context); sev->snp_context = NULL; + /* Free the response page. */ + snp_free_firmware_page(sev->snp_resp_page); + return 0; } @@ -2642,6 +2660,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm) case SVM_VMGEXIT_UNSUPPORTED_EVENT: case SVM_VMGEXIT_HYPERVISOR_FEATURES: case SVM_VMGEXIT_SNP_PAGE_STATE_CHANGE: + case SVM_VMGEXIT_SNP_GUEST_REQUEST: break; default: goto vmgexit_err; @@ -2996,6 +3015,81 @@ static unsigned long snp_handle_page_state_change(struct vcpu_svm *svm, struct g return rc; } +static void snp_handle_guest_request(struct vcpu_svm *svm, struct ghcb *ghcb, + gpa_t req_gpa, gpa_t resp_gpa) +{ + struct sev_data_snp_guest_request data = {}; + struct kvm_vcpu *vcpu = &svm->vcpu; + struct kvm *kvm = vcpu->kvm; + kvm_pfn_t req_pfn, resp_pfn; + struct kvm_sev_info *sev; + int rc, err = 0; + + if (!sev_snp_guest(vcpu->kvm)) { + rc = -ENODEV; + goto e_fail; + } + + sev = &to_kvm_svm(kvm)->sev_info; + + if (!__ratelimit(&sev->snp_guest_msg_rs)) { + pr_info_ratelimited("svm: too many guest message requests\n"); + rc = -EAGAIN; + goto e_fail; + } + + if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, PAGE_SIZE)) { + pr_err("svm: guest request (%#llx) or response (%#llx) is not page aligned\n", + req_gpa, resp_gpa); + goto e_term; + } + + req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa)); + if (is_error_noslot_pfn(req_pfn)) { + pr_err("svm: guest request invalid gpa=%#llx\n", req_gpa); + goto e_term; + } + + resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa)); + if (is_error_noslot_pfn(resp_pfn)) { + pr_err("svm: guest response invalid gpa=%#llx\n", resp_gpa); + goto e_term; + } + + data.gctx_paddr = __psp_pa(sev->snp_context); + data.req_paddr = __sme_set(req_pfn << PAGE_SHIFT); + data.res_paddr = __psp_pa(sev->snp_resp_page); + + mutex_lock(&kvm->lock); + + rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err); + if (rc) { + mutex_unlock(&kvm->lock); + + /* If we have a firmware error code then use it. */ + if (err) + rc = err; + + goto e_fail; + } + + /* Copy the response after the firmware returns success. */ + rc = kvm_write_guest(kvm, resp_gpa, sev->snp_resp_page, PAGE_SIZE); + + mutex_unlock(&kvm->lock); + +e_fail: + ghcb_set_sw_exit_info_2(ghcb, rc); + return; + +e_term: + ghcb_set_sw_exit_info_1(ghcb, 1); + ghcb_set_sw_exit_info_2(ghcb, + X86_TRAP_GP | + SVM_EVTINJ_TYPE_EXEPT | + SVM_EVTINJ_VALID); +} + static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm) { struct vmcb_control_area *control = &svm->vmcb->control; @@ -3245,6 +3339,12 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) ghcb_set_sw_exit_info_2(ghcb, rc); break; } + case SVM_VMGEXIT_SNP_GUEST_REQUEST: { + snp_handle_guest_request(svm, ghcb, control->exit_info_1, control->exit_info_2); + + ret = 1; + break; + } case SVM_VMGEXIT_UNSUPPORTED_EVENT: vcpu_unimpl(vcpu, "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n", diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 011374e6b2b2..ecd466721c23 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -18,6 +18,7 @@ #include <linux/kvm_types.h> #include <linux/kvm_host.h> #include <linux/bits.h> +#include <linux/ratelimit.h> #include <asm/svm.h> #include <asm/sev-common.h> @@ -68,6 +69,8 @@ struct kvm_sev_info { struct kvm *enc_context_owner; /* Owner of copied encryption context */ struct misc_cg *misc_cg; /* For misc cgroup accounting */ void *snp_context; /* SNP guest context page */ + void *snp_resp_page; /* SNP guest response page */ + struct ratelimit_state snp_guest_msg_rs; /* Rate limit the SNP guest message */ }; struct kvm_svm {
Version 2 of GHCB specification added the support SNP Guest Request Message NAE event. The event allows for an SEV-SNP guest to make request to the SEV-SNP firmware through hypervisor using the SNP_GUEST_REQUEST API define in the SEV-SNP firmware specification. The SNP_GUEST_REQUEST requires two unique pages, one page for the request and one page for the response. The response page need to be in the firmware state. The GHCB specification says that both the pages need to be in the hypervisor state but before executing the SEV-SNP command the response page need to be in the firmware state. In order to minimize the page state transition during the command handling, pre-allocate a firmware page on guest creation. Use the pre-allocated firmware page to complete the command execution and copy the result in the guest response page. Ratelimit the handling of SNP_GUEST_REQUEST NAE to avoid the possibility of a guest creating a denial of service attack aginst the SNP firmware. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/kvm/svm/sev.c | 106 +++++++++++++++++++++++++++++++++++++++-- arch/x86/kvm/svm/svm.h | 3 ++ 2 files changed, 106 insertions(+), 3 deletions(-)