linux-kernel - RE: [PATCH Part2 v6 42/49] KVM: SVM: Provide support for SNP_GUEST

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SN6PR12MB27675E821D5D1C423F2AF5768EBB9@SN6PR12MB2767.namprd12.prod.outlook.com>
Date:   Wed, 29 Jun 2022 19:15:15 +0000
From:   "Kalra, Ashish" <Ashish.Kalra@....com>
To:     Peter Gonda <pgonda@...gle.com>
CC:     the arch/x86 maintainers <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kvm list <kvm@...r.kernel.org>,
        "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        "Lendacky, Thomas" <Thomas.Lendacky@....com>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@....com>,
        Borislav Petkov <bp@...en8.de>,
        "Roth, Michael" <Michael.Roth@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Andi Kleen <ak@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>, Marc Orr <marcorr@...gle.com>,
        Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Alper Gun <alpergun@...gle.com>,
        "Dr. David Alan Gilbert" <dgilbert@...hat.com>,
        "jarkko@...nel.org" <jarkko@...nel.org>
Subject: RE: [PATCH Part2 v6 42/49] KVM: SVM: Provide support for
 SNP_GUEST_REQUEST NAE event

[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 = {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.

>Similarly as above, as this is an error path, so what's the guarantee that the next guest message request will succeed completely,  isn’t it better to let the 
>FW reject any subsequent guest messages once it has detected that the sequence numbers are out of sync ?

Alternately, we probably can return SEV_RET_INVALID_PAGE_STATE/SEV_RET_INVALID_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 number. 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 indicative 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.

Thanks,
Ashish