Re: [PATCH Part2 v6 42/49] KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event

[Peter Gonda <pgonda@google.com>]

On Wed, Jun 29, 2022 at 1:15 PM Kalra, Ashish <Ashish.Kalra@amd.com> 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 = {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.

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