Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 1 Feb 2023 07:21:08 +1100 | Subject | Re: [PATCH RFC v7 52/64] KVM: SVM: Provide support for SNP_GUEST_REQUEST NAE event | From | Alexey Kardashevskiy <> |
| |
On 01/02/2023 03:23, Tom Lendacky wrote: > On 1/30/23 19:54, Alexey Kardashevskiy wrote: >> >> >> On 11/1/23 13:01, Kalra, Ashish wrote: >>> On 1/10/2023 6:48 PM, Alexey Kardashevskiy wrote: >>>> On 10/1/23 19:33, Kalra, Ashish wrote: >>>>> >>>>> On 1/9/2023 8:28 PM, Alexey Kardashevskiy wrote: >>>>>> >>>>>> >>>>>> On 10/1/23 10:41, Kalra, Ashish wrote: >>>>>>> On 1/8/2023 9:33 PM, Alexey Kardashevskiy wrote: >>>>>>>> On 15/12/22 06:40, Michael Roth wrote: >>>>>>>>> From: Brijesh Singh <brijesh.singh@amd.com> >>>>>>>>> >>>>>>>>> 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 <brijesh.singh@amd.com> >>>>>>>>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >>>>>>>>> Signed-off-by: Michael Roth <michael.roth@amd.com> >>>>>>>>> --- >>>>>>>>> arch/x86/kvm/svm/sev.c | 185 >>>>>>>>> +++++++++++++++++++++++++++++++++++++++-- >>>>>>>>> arch/x86/kvm/svm/svm.h | 2 + >>>>>>>>> 2 files changed, 181 insertions(+), 6 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >>>>>>>>> index 5f2b2092cdae..18efa70553c2 100644 >>>>>>>>> --- a/arch/x86/kvm/svm/sev.c >>>>>>>>> +++ b/arch/x86/kvm/svm/sev.c >>>>>>>>> @@ -331,6 +331,7 @@ static int sev_guest_init(struct kvm *kvm, >>>>>>>>> struct kvm_sev_cmd *argp) >>>>>>>>> if (ret) >>>>>>>>> goto e_free; >>>>>>>>> + mutex_init(&sev->guest_req_lock); >>>>>>>>> ret = sev_snp_init(&argp->error, false); >>>>>>>>> } else { >>>>>>>>> ret = sev_platform_init(&argp->error); >>>>>>>>> @@ -2051,23 +2052,34 @@ 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) >>>>>>>>> { >>>>>>>>> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; >>>>>>>>> struct sev_data_snp_addr data = {}; >>>>>>>>> - void *context; >>>>>>>>> + void *context, *certs_data; >>>>>>>>> int rc; >>>>>>>>> + /* Allocate memory used for the certs data in SNP guest >>>>>>>>> request */ >>>>>>>>> + certs_data = kzalloc(SEV_FW_BLOB_MAX_SIZE, >>>>>>>>> GFP_KERNEL_ACCOUNT); >>>>>>>>> + if (!certs_data) >>>>>>>>> + return NULL; >>>>>>>>> + >>>>>>>>> /* Allocate memory for context page */ >>>>>>>>> context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT); >>>>>>>>> if (!context) >>>>>>>>> - return NULL; >>>>>>>>> + goto e_free; >>>>>>>>> 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) >>>>>>>>> @@ -2653,6 +2665,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; >>>>>>>>> } >>>>>>>>> @@ -3174,6 +3188,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; >>>>>>>>> @@ -3396,6 +3412,149 @@ static int snp_complete_psc(struct >>>>>>>>> kvm_vcpu *vcpu) >>>>>>>>> return 1; >>>>>>>>> } >>>>>>>>> +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; >>>>>>>>> + >>>>>>>>> + 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; >>>>>>>>> + >>>>>>>>> + ret = rmp_make_shared(pfn, PG_LEVEL_4K); >>>>>>>>> + if (ret) >>>>>>>>> + *rc = SEV_RET_INVALID_ADDRESS; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +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; >>>>>>>>> + >>>>>>>>> + 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); >>>>>>>> >>>>>>>> >>>>>>>> This one goes via sev_issue_cmd_external_user() and uses sev-fd... >>>>>>>> >>>>>>>>> + 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; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + 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); >>>>>>>> >>>>>>>> but this one does not and jump straight to >>>>>>>> drivers/crypto/ccp/sev-dev.c ignoring sev->fd. Why different? >>>>>>>> Can these two be unified? sev_issue_cmd_external_user() only >>>>>>>> checks if fd is /dev/sev which is hardly useful. >>>>>>>> >>>>>>>> "[PATCH RFC v7 32/64] crypto: ccp: Provide APIs to query >>>>>>>> extended attestation report" added this one. >>>>>>> >>>>>>> SNP_EXT_GUEST_REQUEST additionally returns a certificate blob and >>>>>>> that's why it goes through the CCP driver interface >>>>>>> snp_guest_ext_guest_request() that is used to get both the report >>>>>>> and certificate data/blob at the same time. >>>>>> >>>>>> True. I thought though that this calls for extending >>>>>> sev_issue_cmd() to take care of these extra parameters rather than >>>>>> just skipping the sev->fd. >>>>>> >>>>>> >>>>>>> All the FW API calls on the KVM side go through sev_issue_cmd() >>>>>>> and sev_issue_cmd_external_user() interfaces and that i believe >>>>>>> uses sev->fd more of as a sanity check. >>>>>> >>>>>> Does not look like it: >>>>>> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccp/sev-dev.c?h=v6.2-rc3#n1290 >>>>>> >>>>>> === >>>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd, >>>>>> void *data, int *error) >>>>>> { >>>>>> if (!filep || filep->f_op != &sev_fops) >>>>>> return -EBADF; >>>>>> >>>>>> return sev_do_cmd(cmd, data, error); >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user); >>>>>> === >>>>>> >>>>>> The only "more" is that it requires sev->fd to be a valid open fd, >>>>>> what is the value in that? I may easily miss the bigger picture >>>>>> here. Thanks, >>>>>> >>>>>> >>>>> >>>>> Have a look at following functions in drivers/crypto/ccp/sev-dev.c: >>>>> sev_dev_init() and sev_misc_init(). >>>>> >>>>> static int sev_misc_init(struct sev_device *sev) >>>>> { >>>>> struct device *dev = sev->dev; >>>>> int ret; >>>>> >>>>> /* >>>>> * SEV feature support can be detected on multiple devices >>>>> but >>>>> * the SEV FW commands must be issued on the master. During >>>>> * probe, we do not know the master hence we create >>>>> /dev/sev on >>>>> * the first device probe. >>>>> * sev_do_cmd() finds the right master device to which to >>>>> issue >>>>> * the command to the firmware. >>>>> */ >>>> >>>> >>>> It is still a single /dev/sev node and the userspace cannot get it >>>> wrong, it does not have to choose between (for instance) /dev/sev0 >>>> and /dev/sev1 on a 2 SOC system. >>>> >>>>> ... >>>>> ... >>>>> >>>>> Hence, sev_issue_cmd_external_user() needs to ensure that the >>>>> correct device (master device) is being operated upon and that's >>>>> why there is the check for file operations matching sev_fops as >>>>> below : >>>>> >>>>> int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd, >>>>> void *data, int *error) >>>>> { >>>>> if (!filep || filep->f_op != &sev_fops) >>>>> return -EBADF; >>>>> .. >>>>> .. >>>>> >>>>> Essentially, sev->fd is the misc. device created for the master PSP >>>>> device on which the SEV/SNP firmware commands are issued, hence, >>>>> sev_issue_cmd() uses sev->fd. >>>> >>>> There is always just one fd which always uses psp_master, nothing >>>> from that fd is used. >>> >>> It also ensures that we can only issue commands (sev_issue_cmd) after >>> SEV/SNP guest has launched. >> >> I can open /dev/sev and start sending commands to the firmware with no >> KVM running at all. Oh well, we discussed this offline :) >> >>> We don't have a valid fd to use before the guest launch. The file >>> descriptor is passed as part of the guest launch flow, for example, >>> in snp_launch_start(). >>>> >>>> More to the point, if sev->fd is still important, why is it ok to >>>> skip it for snp_handle_ext_guest_request()? Thanks, >>>> >>>> >>> Then, we should do the same for snp_handle_ext_guest_request(). >> >> Okay. >> >> This snp_handle_ext_guest_request() helper is for returning "Table 21. >> ATTESTATION_REPORT Structure" along with the certificate(s) used to >> sign the report: "This usage allows the attestation report and the >> certificates required to verify the report to be returned at the same >> time". >> >> I can see: >> 1) KVM_SEV_SNP_{G,S}ET_CERTS ioctls on KVM VM and > > This allows the VMM to (optionally) supply per-VM certificates that the > guest can use to validate the attestation report, instead of the guest > requesting separately. > >> 2) SNP_{SET,GET}_EXT_CONFIG ioctls on /dev/sev > > This allows the VMM to (optionally) supply certificates used for all > VMs, i.e., there is no need for per-VM certificates. > >> Both store the passed blob and neither communicate it to the firmware. >> This makes me wonder - how does the attestation report (cooked by the >> firmware) get signed with those certificates passed on by the HV >> userspace? > > These are for use by the guest to validate the attestation report. It > allows the guest to obtain the certificate information without having to > use another method to request the certificates. > > By having this certificate store, the hypervisor can request the > certificates from the KDS once, rather than every time a guest requests > an attestation report. > >> >> Also, the cached blob in /dev/sev seems redundand - the attestation >> report is retuned for a specific guest so having a blob in the KVM VM >> makes sense and KVM unconditionally reserves memory for it anyway. And >> for the HV itself the blob is useless (?) so why bother with caching >> it in /dev/sev. > > In general, the certificates are for the machine (VCEK, ASK, ARK), so > they can be for all VMs on the machine. The per-VM blob allows a VMM to > supply additional per-VM certficates, if it desires, but is not required. > >> >> And GET ioctls() return what SET passed on (not something the firware >> returned, for example), what is ever going to call SET? The userspace can > > As stated above, the firmware already has the information needed to sign > the attestation report. The SET IOCTL is used to supply the certficates > to the guest for validation of the attestation report.
Does the firmware have to have all certificates beforehand? How does the firmware choose which certificate to use for a specific VM, or just signs all reports with all certificates it knows?
> This reduces the > traffic and complexity of the guest requesting the certficates from the > KDS.
Guest <-> HV interaction is clear, I am only wondering about HV <-> FW.
>> as well cache what it passed and save a bit of the code/memory in the >> kernel. >> >> btw SNP_{SET,GET}_EXT_CONFIG are documented in >> Documentation/virt/coco/sev-guest.rst but implemented in >> drivers/crypto/ccp/sev-dev.c (not sev-guest.c). >> >> What do I miss in the big picture here? :) Thanks, > > The reason for the extended request is to make the attestation request > appear atomic to the guest. If you had to make two calls to request the > information, in the future, when live migration is possible, there is no > guarantee that the guest couldn't have been migrated in between the > calls to obtain the certificates and the call to obtain the attestation > report and thus validation of the attestation report could fail.
-- Alexey
| ![\](/images/icornerr.gif) |