From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f47.google.com (mail-lf1-f47.google.com [209.85.167.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9B88328E2 for ; Fri, 8 Jul 2022 15:29:04 +0000 (UTC) Received: by mail-lf1-f47.google.com with SMTP id e12so36921854lfr.6 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=vJShvXIhLjnHnaLh5r3gpU4PGp7CjuaBwPmXRpnXYfordoVNppZMM3yN4pnqBnoUpp dcfh37StI75gO9rM1d3H+psSV71jWOh+IdPYQZClRHp4F0YvvmYjVhi1UIq7iX9yQLrH me9oosuQughH0WRYjUWTjJC+soW07Ulk5QEkmcGiHwt5jRNkd4uUNJT5y4FNcnYhvctG Shn8XJIa5866lsLbf/jxQbBE64vnq/Fva7CjHnhLlVENP5iVRG4PdAJdh80FgQIorpoD 9jewW5PHEDs7vuBOC9v4SNepDIjn29XlmhytkM6IPuDKYbeRIpzmwLYxIMACw0id59j7 XTGg== X-Gm-Message-State: AJIora8BSLkTSVIJFglMd8yMNuCiZOKN2L4qwUOcDBfdAffL+anfQ+At UIMkF2GHEzNg4bik8bh2ZTKkmaL/hZwayEr5cAOP0w== 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) Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: 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 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