Message-ID: <b0baa6ee-ea6d-3a30-d5fb-3ec395896750@amd.com>
Date:   Tue, 31 Jan 2023 12:54:37 +1100
From:   Alexey Kardashevskiy <aik@....com>
To:     "Kalra, Ashish" <ashish.kalra@....com>,
        Michael Roth <michael.roth@....com>, kvm@...r.kernel.org
Cc:     linux-coco@...ts.linux.dev, linux-mm@...ck.org,
        linux-crypto@...r.kernel.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
        jroedel@...e.de, thomas.lendacky@....com, hpa@...or.com,
        ardb@...nel.org, pbonzini@...hat.com, seanjc@...gle.com,
        vkuznets@...hat.com, wanpengli@...cent.com, jmattson@...gle.com,
        luto@...nel.org, dave.hansen@...ux.intel.com, slp@...hat.com,
        pgonda@...gle.com, peterz@...radead.org,
        srinivas.pandruvada@...ux.intel.com, rientjes@...gle.com,
        dovmurik@...ux.ibm.com, tobin@....com, bp@...en8.de,
        vbabka@...e.cz, kirill@...temov.name, ak@...ux.intel.com,
        tony.luck@...el.com, marcorr@...gle.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com, alpergun@...gle.com,
        dgilbert@...hat.com, jarkko@...nel.org, harald@...fian.com,
        Brijesh Singh <brijesh.singh@....com>
Subject: Re: [PATCH RFC v7 52/64] KVM: SVM: Provide support for
 SNP_GUEST_REQUEST NAE event



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@....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@....com>
>>>>>>> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
>>>>>>> Signed-off-by: Michael Roth <michael.roth@....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
2) SNP_{SET,GET}_EXT_CONFIG ioctls on /dev/sev
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?

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.

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 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,


-- 
Alexey

Powered by blists - more mailing lists