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 B1DFBC43334 for ; Fri, 8 Jul 2022 15:29:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3E5E96B0073; Fri, 8 Jul 2022 11:29:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 395E56B0074; Fri, 8 Jul 2022 11:29:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 284DA8E0001; Fri, 8 Jul 2022 11:29:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 1A3296B0073 for ; Fri, 8 Jul 2022 11:29:05 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay12.hostedemail.com (Postfix) with ESMTP id E54121208B1 for ; Fri, 8 Jul 2022 15:29:04 +0000 (UTC) X-FDA: 79664315808.24.FFEFC7E Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) by imf18.hostedemail.com (Postfix) with ESMTP id 5624D1C0058 for ; Fri, 8 Jul 2022 15:29:04 +0000 (UTC) Received: by mail-lf1-f48.google.com with SMTP id e20so8162315lfq.11 for ; Fri, 08 Jul 2022 08:29:04 -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:content-transfer-encoding; bh=EsqbPN5x+9PQgkB3EhpcwOrofU0GdaTgCdJPXG3XIDs=; b=Cjnkmx6zMzqZ3p3G/vWvCi7H5Ak27tLHkdjhmkw9kNmIAPJUo8Zng3TS5lXIfi6DBw X3tUPRJlg4MJWu+CehqG44HL/yX8UnZbaetVtGCue+/dkdQhexRD9soeCLZkESw3xgav PADev3iQqDx6T63UIbOBv+bZJGarOCx/gjp/mT5vs2iAEenyNWCIpiGzUNYHEMlGwPxW qZC7lCmjwJmRxifn9Rc6bcipQ7WFHZgUtl2Ho9HL5gHQrUb+LvEpMSwQ6e1O6/soqAhT LZRfSUQ7NKWdx7lQcXIdC9xYJnjTWcyo16zxdsDVt5YINNiZ4TeBSrYhfklXh5wMLdbo tZuw== 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:content-transfer-encoding; bh=EsqbPN5x+9PQgkB3EhpcwOrofU0GdaTgCdJPXG3XIDs=; b=KEEcNcgvae7tUKDWiI9kNqiji59f61eYmXNGXNXkH5ziybybqOp5PNYkNBt8o79P0L IPF51c5qyTIEoRcKIOxJ56Appm4A4egWirHHfiFo0NtPVq2uL9U2ixSVoHkk836P75hV IQOEpG4b8HTRgkERoC9gciV6RzD+d3S08snt1H73IiEpFN2yRchGTrbT+vzjyDZ/8I11 l7h0dzlZXYahpc2Vfqd4v/wdQ8Tviocj6SvnFC4ZSsFeoS+iGFVgf3JHfr/9qmormFzl xzM0W8YEls5DivEYgUIJPkVAG2StSZJyumn89a0NCrcAl9tSJg9L0vAZBzVI9+f1zsnh Xnjg== X-Gm-Message-State: AJIora+r7j2NxDvlfH367p9pZcATFlIzWVBqUnCjD3QsD9RDu9xxLAYS bncCoWcEqT1rOM5A9iwVi205oPohj8pO/8/elklVYQ== X-Google-Smtp-Source: AGRyM1sV1zq1ZnN2yjXTPuiG/7Xv8X9xkix73ZrgQZOxRd61c9fzZcS0BmfVarULhrHnBVAu75EPaoUECCydheenFzI= X-Received: by 2002:a05:6512:1112:b0:488:e0ac:fb41 with SMTP id l18-20020a056512111200b00488e0acfb41mr3001464lfg.456.1657294142373; Fri, 08 Jul 2022 08:29:02 -0700 (PDT) MIME-Version: 1.0 References: <5d05799fc61994684aa2b2ddb8c5b326a3279e25.1655761627.git.ashish.kalra@amd.com> In-Reply-To: From: Peter Gonda Date: Fri, 8 Jul 2022 09:28:50 -0600 Message-ID: Subject: Re: [PATCH Part2 v6 42/49] KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event To: "Kalra, Ashish" 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 , "Roth, Michael" , 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" Content-Transfer-Encoding: quoted-printable ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1657294144; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=EsqbPN5x+9PQgkB3EhpcwOrofU0GdaTgCdJPXG3XIDs=; b=Z5M9aSW2jbFf6MGyVCAP5YKqTOSLnsYfSxPYzY0ZOHRGmQ2FjFapSe8+58KAIsT2Ym/Yll MtszHWicF5N/wvXZqKdT7mXt0Tubi2J/+Jwfw3vU+9xsj+krjgbKZhY8KXF5PbXGpbTPk0 7/8F8Df4neSYhiM9d5/Zd/Ubd6Q7BCQ= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1657294144; a=rsa-sha256; cv=none; b=LbhayI1FKhh57ORqHxIiNj6K/TNYRuagtCWglLyhCRafHMz8zPJsUkEakxyLqSfyC/HPs1 Sk73lptaJxxTvxPKg2C9qLbvK0mBUZ3Z8lMFlvhKc85DzndILyQyXV/Tuo/4ntflKES0Rv 5OE4rPOcFbgC8SUDGwat3gOZedhYOyI= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Cjnkmx6z; spf=pass (imf18.hostedemail.com: domain of pgonda@google.com designates 209.85.167.48 as permitted sender) smtp.mailfrom=pgonda@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Queue-Id: 5624D1C0058 X-Rspam-User: Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Cjnkmx6z; spf=pass (imf18.hostedemail.com: domain of pgonda@google.com designates 209.85.167.48 as permitted sender) smtp.mailfrom=pgonda@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam03 X-Stat-Signature: w7j8ychacwu8x3gmse599fntgg3w91fi X-HE-Tag: 1657294144-648925 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 Wed, Jun 29, 2022 at 1:15 PM Kalra, Ashish wrote: > > [Public] > > > >> +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 =3D {0}; > >> + struct kvm_vcpu *vcpu =3D &svm->vcpu; > >> + struct kvm *kvm =3D 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 =3D SEV_RET_INVALID_GUEST; > >> + goto e_fail; > >> + } > >> + > >> + sev =3D &to_kvm_svm(kvm)->sev_info; > >> + > >> + data_gpa =3D vcpu->arch.regs[VCPU_REGS_RAX]; > >> + data_npages =3D vcpu->arch.regs[VCPU_REGS_RBX]; > >> + > >> + if (!IS_ALIGNED(data_gpa, PAGE_SIZE)) { > >> + rc =3D 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 =3D SEV_RET_INVALID_PARAM; > >> + goto e_fail; > >> + } > >> + > >> + mutex_lock(&sev->guest_req_lock); > >> + > >> + rc =3D snp_setup_guest_buf(svm, &req, req_gpa, resp_gpa); > >> + if (rc) > >> + goto unlock; > >> + > >> + rc =3D snp_guest_ext_guest_request(&req, (unsigned long)sev->s= np_certs_data, > >> + &data_npages, &err); > >> + if (rc) { > >> + /* > >> + * If buffer length is small then return the expected > >> + * length in rbx. > >> + */ > >> + if (err =3D=3D SNP_GUEST_REQ_INVALID_LEN) > >> + vcpu->arch.regs[VCPU_REGS_RBX] =3D data_npages= ; > >> + > >> + /* pass the firmware error code */ > >> + rc =3D 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_n= pages << PAGE_SHIFT)) > >> + rc =3D SEV_RET_INVALID_ADDRESS; > > >>Since at this point the PSP FW has correctly executed the command and i= ncremented the VMPCK sequence number I think we need another error signal h= ere since this will tell the guest the PSP had an error so it will not know= if the VMPCK sequence >number should be incremented. > > >Similarly as above, as this is an error path, so what's the guarantee th= at the next guest message request will succeed completely, isn=E2=80=99t i= t better to let the > >FW reject any subsequent guest messages once it has detected that the se= quence numbers are out of sync ? > > Alternately, we probably can return SEV_RET_INVALID_PAGE_STATE/SEV_RET_IN= VALID_PAGE_OWNER here, but that still does not indicate to the guest > that the FW has successfully executed the command and the error occurred = during cleanup/result phase and it needs to increment the VMPCK sequence nu= mber. There is nothing as such defined in SNP FW API specs to indicate such= kind of failures to guest. As I mentioned earlier, this is probably indica= tive of > a bigger system failure and it is better to let the FW reject subsequent = guest messages/requests once it has detected that the sequence numbers are = out of sync. Hmm I think the guest must be careful here because the guest could not trust the hypervisor here to be truthful about the sequence numbers incrementing. That's unfortunate since this means if these operations do fail with a well behaved hypervisor the guest cannot use that VMPCK again. But there is no harm in the guest re-issuing the SNP_GUEST_REQUEST (or extended version) with the exact same request just in at a different address. The GHCB spec actually calls this out " It is recommended that the hypervisor validate the guest physical address of the response page before invoking the SNP_GUEST_REQUEST API so that the sequence numbers do not get out of sync for the guest, possibly resulting in all successive requests failing". Currently SVM_VMGEXIT_GUEST_REQUEST and SVM_VMGEXIT_EXT_GUEST_REQUEST have different hypervisor -> guest usage for SW_EXITINFO2. I think they both should be defined as what SVM_VMGEXIT_EXT_GUEST_REQUEST is now: the high 32bits are the hypervisor error code, the low 32bits are the FW error code. This would allow for both NAEs to have some signal to the guest say SEV_RET_INVALID_REQ_ADDRESS. The hypervisor can use this error code when doing the validation on the request and response regions, if some is wrong with them the guest can retry with the exact same request (so no IV reuse) in a corrected region. But another reason I think SVM_VMGEXIT_GUEST_REQUEST SW_EXITINFO2 hypervisor->guest state should include this change is because in this patch we are currently overloading the lower 32bits with hypervisor error codes. In snp_handle_guest_request() if sev_snp_guest(), snp_setup_guest_buf(), or snp_cleanup_guest_buf() fails we use the low 32bits of SW_EXITINFO2 to return hypervisor errors to the guest. > > Thanks, > Ashish