From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BD22AC43334 for ; Fri, 24 Jun 2022 16:25:36 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 34DF18E0233; Fri, 24 Jun 2022 12:25:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2D6A68E020E; Fri, 24 Jun 2022 12:25:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1291C8E0233; Fri, 24 Jun 2022 12:25:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id F01C58E020E for ; Fri, 24 Jun 2022 12:25:35 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay12.hostedemail.com (Postfix) with ESMTP id BA10E120A2C for ; Fri, 24 Jun 2022 16:25:35 +0000 (UTC) X-FDA: 79613655030.26.80EC021 Received: from mail-lf1-f53.google.com (mail-lf1-f53.google.com [209.85.167.53]) by imf15.hostedemail.com (Postfix) with ESMTP id 46A02A009D for ; Fri, 24 Jun 2022 16:25:35 +0000 (UTC) Received: by mail-lf1-f53.google.com with SMTP id g4so5318071lfv.9 for ; Fri, 24 Jun 2022 09:25:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=7sfkYHgtbrbvRGaQ8qGJyoZw24OZdcFRjeTJQ0mC8q4=; b=Bpuvm7GZi0KpPV9CEvl2ZIxQGNNzpQfrEgbiAWnDFyUyNTloQ1P9ixD/SgxfYKlOVE F0CDg/mQmug/gIW536C/uzDniUPU/hl0pXQAKy/zb6ff+vbuxbXoler9iXN1xTB8dU9r 3GHeKa4WME/VmHifB9h/fTx60FyJvwXKIYocKDXGDGgmIuA5A2IjqldJtNFkVk7hTLAo Q+1y8n1IsKQxljxIUGPtLjYPvMetq7k8Ip3i8Vl7fFR+QdOV/RJhGszjxs4/Rh5DDNVI jnaS4w973Y3Trei9qw+FNNZgSZUFfQyEYfHFsSsQRZtahn0tEo8tLSL/l1NQ1DGImPq6 vTCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=7sfkYHgtbrbvRGaQ8qGJyoZw24OZdcFRjeTJQ0mC8q4=; b=1NKE7fMjxNoOUzfscQagrFAxbW3qNwJR8hm1nXGojZa6tq1iIgz7YzHiEwHkE16GTw wWBoxV5PfoZNLV5xfzn0hm8OOwX1EuyWBIwdQKDVUrYhPkZ6zW7JPJPE4cyMxa/Do29W qaDTVWEJv10e8eKzyFbmsgS9KjOlFAwyrHA30cQpf2EZZANj5euBcf7qm4CfiX3msoJm 1iV1U8gshGeyQ0kKwS7KsFi5yhiiuviyrEfaDvCSV1pB6+vtkfvdXDRz0fg1s+kLjH1W dTizO8gF3R4hZrKys69Ppgz0JJ5afDgr+4paYGrVYJ0aauFr4Bxgk/4U3h9sxCeVUbG7 zfWQ== X-Gm-Message-State: AJIora9jQcZWa5m7WrRnN7tVEKj4RHRZ1tDgq5KARC2M4qfy0oLU93QH 8pN8aTmuSF4IWqPcVJk204M5rMw6ZSkGScIEeG806Q== X-Google-Smtp-Source: AGRyM1sXtqDomYOcNv9VoYB53aObLHNx+adeM1Ml3hiisDwkj9xMA0c9em4sVv6VpCS2bsk4MEFITCr2Dix8IeZEXBY= X-Received: by 2002:a05:6512:401a:b0:47f:6ea5:dace with SMTP id br26-20020a056512401a00b0047f6ea5dacemr9227307lfb.402.1656087933300; Fri, 24 Jun 2022 09:25:33 -0700 (PDT) MIME-Version: 1.0 References: <5d05799fc61994684aa2b2ddb8c5b326a3279e25.1655761627.git.ashish.kalra@amd.com> In-Reply-To: <5d05799fc61994684aa2b2ddb8c5b326a3279e25.1655761627.git.ashish.kalra@amd.com> From: Peter Gonda Date: Fri, 24 Jun 2022 10:25:21 -0600 Message-ID: Subject: Re: [PATCH Part2 v6 42/49] KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event To: Ashish Kalra Cc: "the arch/x86 maintainers" , LKML , kvm list , linux-coco@lists.linux.dev, linux-mm@kvack.org, Linux Crypto Mailing List , Thomas Gleixner , Ingo Molnar , Joerg Roedel , "Lendacky, Thomas" , "H. Peter Anvin" , Ard Biesheuvel , Paolo Bonzini , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Andy Lutomirski , Dave Hansen , Sergio Lopez , Peter Zijlstra , Srinivas Pandruvada , David Rientjes , Dov Murik , Tobin Feldman-Fitzthum , Borislav Petkov , Michael Roth , Vlastimil Babka , "Kirill A . Shutemov" , Andi Kleen , Tony Luck , Marc Orr , Sathyanarayanan Kuppuswamy , Alper Gun , "Dr. David Alan Gilbert" , jarkko@kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Bpuvm7GZ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of pgonda@google.com designates 209.85.167.53 as permitted sender) smtp.mailfrom=pgonda@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1656087935; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=7sfkYHgtbrbvRGaQ8qGJyoZw24OZdcFRjeTJQ0mC8q4=; b=MtS82G+dEtnLpeVA1K1XMQdZL7D/C7lFpgNoiHn/J2dm/q4eUHyZeU0PNx9HlCIHk/VL8S Zi3C7AtdW2Y5uk+5tCsgVciJm+aZSx9XuPIKDb052+R3JwkLW7Z3iAUFW+pGsKICwaPQiE vZRk3vAgmZqEGiuVrcVqIJ3fK19p3sk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1656087935; a=rsa-sha256; cv=none; b=fcWCdGsc5YD4AW5DWSFhxQ18f7ynB0TUzq+xqN4CQCDYowbKDmNOPK86lau6+lxhtANBpx 0GIZIOfOZvJyXbIE0t4bGtbfqYHLzUaJJlJ1anC4VwRsEDzR48pnw4PmoEtRrNVIbBWAm4 i4bgMzrK7kbb2tarJgArCod9dfZFdjA= X-Stat-Signature: x7t9ac5ru9dnbik1t5i953h8hf5dj4mi X-Rspamd-Queue-Id: 46A02A009D X-Rspam-User: Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Bpuvm7GZ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of pgonda@google.com designates 209.85.167.53 as permitted sender) smtp.mailfrom=pgonda@google.com X-Rspamd-Server: rspam12 X-HE-Tag: 1656087935-948738 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Jun 20, 2022 at 5:13 PM Ashish Kalra wrote: > > From: Brijesh Singh > > Version 2 of GHCB specification added the support for two SNP Guest > Request Message NAE events. The events 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_EXT_GUEST_REQUEST is similar to SNP_GUEST_REQUEST with the > difference of an additional certificate blob that can be passed through > the SNP_SET_CONFIG ioctl defined in the CCP driver. The CCP driver > provides snp_guest_ext_guest_request() that is used by the KVM to get > both the report and certificate data at once. > > Signed-off-by: Brijesh Singh > --- > arch/x86/kvm/svm/sev.c | 196 +++++++++++++++++++++++++++++++++++++++-- > arch/x86/kvm/svm/svm.h | 2 + > 2 files changed, 192 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 7fc0fad87054..089af21a4efe 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -343,6 +343,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > > spin_lock_init(&sev->psc_lock); > ret = sev_snp_init(&argp->error); > + mutex_init(&sev->guest_req_lock); > } else { > ret = sev_platform_init(&argp->error); > } > @@ -1884,23 +1885,39 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd) > > static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp) > { > + void *context = NULL, *certs_data = NULL, *resp_page = NULL; Is the NULL setting here unnecessary since all of these are set via functions snp_alloc_firmware_page(), kmalloc(), and snp_alloc_firmware_page() respectively? > + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > struct sev_data_snp_gctx_create data = {}; > - void *context; > int rc; > > + /* Allocate memory used for the certs data in SNP guest request */ > + certs_data = kmalloc(SEV_FW_BLOB_MAX_SIZE, GFP_KERNEL_ACCOUNT); > + if (!certs_data) > + return NULL; I think we want to use kzalloc() here to ensure we never give the guest uninitialized kernel memory. > + > /* Allocate memory for context page */ > context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT); > if (!context) > - return NULL; > + goto e_free; > + > + /* Allocate a firmware buffer used during the guest command handling. */ > + resp_page = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT); > + if (!resp_page) > + goto e_free; |resp_page| doesn't appear to be used anywhere? > > data.gctx_paddr = __psp_pa(context); > rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error); > - if (rc) { > - snp_free_firmware_page(context); > - return NULL; > - } > + if (rc) > + goto e_free; > + > + sev->snp_certs_data = certs_data; > > return context; > + > +e_free: > + snp_free_firmware_page(context); > + kfree(certs_data); > + return NULL; > } > > static int snp_bind_asid(struct kvm *kvm, int *error) > @@ -2565,6 +2582,8 @@ static int snp_decommission_context(struct kvm *kvm) > snp_free_firmware_page(sev->snp_context); > sev->snp_context = NULL; > > + kfree(sev->snp_certs_data); > + > return 0; > } > > @@ -3077,6 +3096,8 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm, u64 *exit_code) > case SVM_VMGEXIT_UNSUPPORTED_EVENT: > case SVM_VMGEXIT_HV_FEATURES: > case SVM_VMGEXIT_PSC: > + case SVM_VMGEXIT_GUEST_REQUEST: > + case SVM_VMGEXIT_EXT_GUEST_REQUEST: > break; > default: > reason = GHCB_ERR_INVALID_EVENT; > @@ -3502,6 +3523,155 @@ static unsigned long snp_handle_page_state_change(struct vcpu_svm *svm) > return rc ? map_to_psc_vmgexit_code(rc) : 0; > } > > +static unsigned long snp_setup_guest_buf(struct vcpu_svm *svm, > + struct sev_data_snp_guest_request *data, > + gpa_t req_gpa, gpa_t resp_gpa) > +{ > + struct kvm_vcpu *vcpu = &svm->vcpu; > + struct kvm *kvm = vcpu->kvm; > + kvm_pfn_t req_pfn, resp_pfn; > + struct kvm_sev_info *sev; > + > + sev = &to_kvm_svm(kvm)->sev_info; This is normally done at declaration in this file. Why not here? struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > + > + if (!IS_ALIGNED(req_gpa, PAGE_SIZE) || !IS_ALIGNED(resp_gpa, PAGE_SIZE)) > + return SEV_RET_INVALID_PARAM; > + > + req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa)); > + if (is_error_noslot_pfn(req_pfn)) > + return SEV_RET_INVALID_ADDRESS; > + > + resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa)); > + if (is_error_noslot_pfn(resp_pfn)) > + return SEV_RET_INVALID_ADDRESS; > + > + if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true)) > + return SEV_RET_INVALID_ADDRESS; > + > + data->gctx_paddr = __psp_pa(sev->snp_context); > + data->req_paddr = __sme_set(req_pfn << PAGE_SHIFT); > + data->res_paddr = __sme_set(resp_pfn << PAGE_SHIFT); > + > + return 0; > +} > + > +static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request *data, unsigned long *rc) > +{ > + u64 pfn = __sme_clr(data->res_paddr) >> PAGE_SHIFT; > + int ret; > + > + ret = snp_page_reclaim(pfn); > + if (ret) > + *rc = SEV_RET_INVALID_ADDRESS; Do we need a diff error code here? This means the page the guest gives us is now "stuck" in the FW owned state. How would the guest know this is the case? We return the exact same error in snp_setup_guest_buf() if the resp_gpa isn't page aligned so now if the guest ever sees a SEV_RET_INVALID_ADDRESS I think its only safe option is to either try and page_state_change it to a know state or mark it as unusable memory. > + > + ret = rmp_make_shared(pfn, PG_LEVEL_4K); > + if (ret) > + *rc = SEV_RET_INVALID_ADDRESS; Ditto here I think we need some way to signal to the guest what state this page is on return to guest execution. Also these errors shadow over FW successes, this means the guest's guest-request-sequence-numbers are now out of sync meaning this VMPCK is unusable less the guest risk reusing the AES IV (which would break the confidentiality/integrity). Should we have a way to signal to the guest the FW has successfully run your command but we could not change the page states back correctly, so the guest should increment their sequence numbers. > +} > + > +static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa) > +{ > + struct sev_data_snp_guest_request data = {0}; > + struct kvm_vcpu *vcpu = &svm->vcpu; > + struct kvm *kvm = vcpu->kvm; > + struct kvm_sev_info *sev; > + unsigned long rc; > + int err; > + > + if (!sev_snp_guest(vcpu->kvm)) { > + rc = SEV_RET_INVALID_GUEST; > + goto e_fail; > + } > + > + sev = &to_kvm_svm(kvm)->sev_info; Ditto why not due this above? > + > + mutex_lock(&sev->guest_req_lock); > + > + rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa); > + if (rc) > + goto unlock; > + > + rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err); > + if (rc) > + /* use the firmware error code */ > + rc = err; > + > + snp_cleanup_guest_buf(&data, &rc); > + > +unlock: > + mutex_unlock(&sev->guest_req_lock); > + > +e_fail: > + svm_set_ghcb_sw_exit_info_2(vcpu, rc); > +} > + > +static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa) > +{ > + struct sev_data_snp_guest_request req = {0}; > + struct kvm_vcpu *vcpu = &svm->vcpu; > + struct kvm *kvm = vcpu->kvm; > + unsigned long data_npages; > + struct kvm_sev_info *sev; > + unsigned long rc, err; > + u64 data_gpa; > + > + if (!sev_snp_guest(vcpu->kvm)) { > + rc = SEV_RET_INVALID_GUEST; > + goto e_fail; > + } > + > + sev = &to_kvm_svm(kvm)->sev_info; > + > + data_gpa = vcpu->arch.regs[VCPU_REGS_RAX]; > + data_npages = vcpu->arch.regs[VCPU_REGS_RBX]; > + > + if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) { > + rc = SEV_RET_INVALID_ADDRESS; > + goto e_fail; > + } > + > + /* Verify that requested blob will fit in certificate buffer */ > + if ((data_npages << PAGE_SHIFT) > SEV_FW_BLOB_MAX_SIZE) { > + rc = SEV_RET_INVALID_PARAM; > + goto e_fail; > + } > + > + mutex_lock(&sev->guest_req_lock); > + > + rc = snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa); > + if (rc) > + goto unlock; > + > + rc = snp_guest_ext_guest_request(&req, (unsigned long)sev->snp_certs_data, > + &data_npages, &err); > + if (rc) { > + /* > + * If buffer length is small then return the expected > + * length in rbx. > + */ > + if (err == SNP_GUEST_REQ_INVALID_LEN) > + vcpu->arch.regs[VCPU_REGS_RBX] = data_npages; > + > + /* pass the firmware error code */ > + rc = err; > + goto cleanup; > + } > + > + /* Copy the certificate blob in the guest memory */ > + if (data_npages && > + kvm_write_guest(kvm, data_gpa, sev->snp_certs_data, data_npages << PAGE_SHIFT)) > + rc = SEV_RET_INVALID_ADDRESS; Since at this point the PSP FW has correctly executed the command and incremented the VMPCK sequence number I think we need another error signal here since this will tell the guest the PSP had an error so it will not know if the VMPCK sequence number should be incremented. > + > +cleanup: > + snp_cleanup_guest_buf(&req, &rc); > + > +unlock: > + mutex_unlock(&sev->guest_req_lock); > + > +e_fail: > + svm_set_ghcb_sw_exit_info_2(vcpu, rc); > +} > + > static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm) > { > struct vmcb_control_area *control = &svm->vmcb->control; > @@ -3753,6 +3923,20 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) > svm_set_ghcb_sw_exit_info_2(vcpu, rc); > break; > } > + case SVM_VMGEXIT_GUEST_REQUEST: { > + snp_handle_guest_request(svm, control->exit_info_1, control->exit_info_2); > + > + ret = 1; > + break; > + } > + case SVM_VMGEXIT_EXT_GUEST_REQUEST: { > + snp_handle_ext_guest_request(svm, > + 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 3fd95193ed8d..3be24da1a743 100644 > --- a/arch/x86/kvm/svm/svm.h > +++ b/arch/x86/kvm/svm/svm.h > @@ -98,6 +98,8 @@ struct kvm_sev_info { > u64 snp_init_flags; > void *snp_context; /* SNP guest context page */ > spinlock_t psc_lock; > + void *snp_certs_data; > + struct mutex guest_req_lock; > }; > > struct kvm_svm { > -- > 2.25.1 >