Message-ID: <ff0ffddf-9a4b-e1cd-7c34-73412c89ded6@amd.com>
Date:   Tue, 7 Feb 2023 12:24:43 +1100
From:   Alexey Kardashevskiy <aik@....com>
To:     "Kalra, Ashish" <ashish.kalra@....com>, kvm@...r.kernel.org
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        linux-crypto@...r.kernel.org,
        Tom Lendacky <thomas.lendacky@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Michael Roth <michael.roth@....com>,
        John Allen <john.allen@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Dionna Glaze <dionnaglaze@...gle.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Brijesh Singh <brijesh.singh@....com>,
        Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH kernel] KVM: SVM: Fix SVM_VMGEXIT_EXT_GUEST_REQUEST to
 follow the rest of API



On 07/02/2023 08:57, Kalra, Ashish wrote:
> On 2/5/2023 9:13 PM, Alexey Kardashevskiy wrote:
>> When SVM VM is up, KVM uses sev_issue_cmd_external_user() with an open
>> /dev/sev fd which ensures that the SVM initialization was done correctly.
>> The only helper not following the scheme is snp_guest_ext_guest_request()
>> which bypasses the fd check.
>>
>> Change the SEV API to require passing a file.
>>
>> Handle errors with care in the SNP Extended Guest Request handler
>> (snp_handle_ext_guest_request()) as there are actually 3 types of errors:
>> - @rc: return code SEV device's sev_issue_cmd() which is int==int32;
>> - @err: a psp return code in sev_issue_cmd(), also int==int32 (probably
>> a mistake but kvm_sev_cmd::error uses __u32 for some time now);
>> - (added by this) @exitcode: GHCB's exit code sw_exit_info_2, uint64.
>>
>> Use the right types, remove cast to int* and return ENOSPC from SEV
>> device for converting it to the GHCB's exit code
>> SNP_GUEST_REQ_INVALID_LEN==BIT(32).
>>
>> Fixes: 17f1d0c995ac ("KVM: SVM: Provide support for SNP_GUEST_REQUEST 
>> NAE event")
>> While at this, preserve the original error in snp_cleanup_guest_buf().
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@....com>
>> ---
>>
>> This can easily be squashed into what it fixes.
>>
>> The patch is made for
>> https://github.com/AMDESE/linux/commits/upmv10-host-snp-v7-rfc
>> ---
>>   include/linux/psp-sev.h      | 62 +++++++++++---------
>>   arch/x86/kvm/svm/sev.c       | 50 +++++++++++-----
>>   drivers/crypto/ccp/sev-dev.c | 11 ++--
>>   3 files changed, 73 insertions(+), 50 deletions(-)
>>
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 970a9de0ed20..466b1a6e7d7b 100644
>> --- a/include/linux/psp-sev.h
>> +++ b/include/linux/psp-sev.h
>> @@ -848,6 +848,36 @@ int sev_platform_status(struct 
>> sev_user_data_status *status, int *error);
>>   int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
>>                   void *data, int *error);
>> +/**
>> + * sev_issue_cmd_external_user_cert - issue SEV command by other 
>> driver with a file
>> + * handle and return certificates set onto SEV device via 
>> SNP_SET_EXT_CONFIG;
>> + * intended for use by the SNP extended guest request command defined
>> + * in the GHCB specification.
>> + *
>> + * @filep - SEV device file pointer
>> + * @cmd - command to issue
>> + * @data - command buffer
>> + * @vaddr: address where the certificate blob need to be copied.
>> + * @npages: number of pages for the certificate blob.
>> + *    If the specified page count is less than the certificate blob 
>> size, then the
>> + *    required page count is returned with ENOSPC error code.
>> + *    If the specified page count is more than the certificate blob 
>> size, then
>> + *    page count is updated to reflect the amount of valid data 
>> copied in the
>> + *    vaddr.
>> + *
>> + * @error: SEV command return code
>> + *
>> + * Returns:
>> + * 0 if the sev successfully processed the command
>> + * -%ENODEV    if the sev device is not available
>> + * -%ENOTSUPP  if the sev does not support SEV
>> + * -%ETIMEDOUT if the sev command timed out
>> + * -%EIO       if the sev returned a non-zero return code
>> + * -%ENOSPC    if the specified page count is too small
>> + */
>> +int sev_issue_cmd_external_user_cert(struct file *filep, unsigned int 
>> cmd, void *data,
>> +                     unsigned long vaddr, unsigned long *npages, int 
>> *error);
>> +
>>   /**
>>    * sev_guest_deactivate - perform SEV DEACTIVATE command
>>    *
>> @@ -945,32 +975,6 @@ void snp_free_firmware_page(void *addr);
>>    */
>>   void snp_mark_pages_offline(unsigned long pfn, unsigned int npages);
>> -/**
>> - * snp_guest_ext_guest_request - perform the SNP extended guest 
>> request command
>> - *  defined in the GHCB specification.
>> - *
>> - * @data: the input guest request structure
>> - * @vaddr: address where the certificate blob need to be copied.
>> - * @npages: number of pages for the certificate blob.
>> - *    If the specified page count is less than the certificate blob 
>> size, then the
>> - *    required page count is returned with error code defined in the 
>> GHCB spec.
>> - *    If the specified page count is more than the certificate blob 
>> size, then
>> - *    page count is updated to reflect the amount of valid data 
>> copied in the
>> - *    vaddr.
>> - *
>> - * @sev_ret: sev command return code
>> - *
>> - * Returns:
>> - * 0 if the sev successfully processed the command
>> - * -%ENODEV    if the sev device is not available
>> - * -%ENOTSUPP  if the sev does not support SEV
>> - * -%ETIMEDOUT if the sev command timed out
>> - * -%EIO       if the sev returned a non-zero return code
>> - */
>> -int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
>> -                unsigned long vaddr, unsigned long *npages,
>> -                unsigned long *error);
>> -
>>   #else    /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>   static inline int
>> @@ -1013,9 +1017,9 @@ static inline void 
>> *snp_alloc_firmware_page(gfp_t mask)
>>   static inline void snp_free_firmware_page(void *addr) { }
>> -static inline int snp_guest_ext_guest_request(struct 
>> sev_data_snp_guest_request *data,
>> -                          unsigned long vaddr, unsigned long *n,
>> -                          unsigned long *error)
>> +static inline int sev_issue_cmd_external_user_cert(struct file 
>> *filep, unsigned int cmd,
>> +                           void *data, unsigned long vaddr,
>> +                           unsigned long *npages, int *error)
>>   {
>>       return -ENODEV;
>>   }
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index d0e58cffd1ed..b268c35efab4 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -394,6 +394,23 @@ static int sev_issue_cmd(struct kvm *kvm, int id, 
>> void *data, int *error)
>>       return __sev_issue_cmd(sev->fd, id, data, error);
>>   }
>> +static int sev_issue_cmd_cert(struct kvm *kvm, int id, void *data,
>> +                  unsigned long vaddr, unsigned long *npages, int 
>> *error)
>> +{
>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +    struct fd f;
>> +    int ret;
>> +
>> +    f = fdget(sev->fd);
>> +    if (!f.file)
>> +        return -EBADF;
>> +
>> +    ret = sev_issue_cmd_external_user_cert(f.file, id, data, vaddr, 
>> npages, error);
>> +
>> +    fdput(f);
>> +    return ret;
>> +}
>> +
>>   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   {
>>       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> @@ -3587,11 +3604,11 @@ static void snp_cleanup_guest_buf(struct 
>> sev_data_snp_guest_request *data, unsig
>>       int ret;
>>       ret = snp_page_reclaim(pfn);
>> -    if (ret)
>> +    if (ret && (*rc == SEV_RET_SUCCESS))
>>           *rc = SEV_RET_INVALID_ADDRESS;
>>       ret = rmp_make_shared(pfn, PG_LEVEL_4K);
>> -    if (ret)
>> +    if (ret && (*rc == SEV_RET_SUCCESS))
>>           *rc = SEV_RET_INVALID_ADDRESS;
>>   }
> 
> I believe we need to fix this as per the GHCB specifications.
> 
> As per GHCB 2.0 specifications:
> 
> SW_EXITINFO2
> ...
> State from Hypervisor: Upper
> 32-bits (63:32) will contain the
> return code from the hypervisor.
> Lower 32-bits (31:0) will contain
> the return code from the firmware
> call (0 = success)
> 
> So i believe the FW error code (which is the FW error code from 
> SNP_GUEST_REQUEST or *rc here) should be contained in the lower 32-bits 
> and the error code being returned back due to response buffer pages 
> reclaim failure and/or failure to transisition these pages back to 
> shared state is basically hypervisor (error) return code and that should 
> be returned in the upper 32-bit of the exitinfo.
> 
> There is work in progress to check conformance of SNP v7 patches to GHCB 
> 2.0 specifications, so probably this fix can be included as part of 
> those patches.

Yes, please :)


> 
>> @@ -3638,8 +3655,9 @@ static void snp_handle_ext_guest_request(struct 
>> vcpu_svm *svm, gpa_t req_gpa, gp
>>       struct kvm *kvm = vcpu->kvm;
>>       unsigned long data_npages;
>>       struct kvm_sev_info *sev;
>> -    unsigned long rc, err;
> 
> This needs to be looked at more carefully. The SEV firmware status code 
> is defined as 32-bit, but is being handled as unsigned long in the 
> KVM/SNP code and as int in the CCP driver. So this needs to be fixed 
> consistently across,

Ultimately it should be explicit u32 in SEV and u64 in GHCB because PSP 
and GHCB are binary interfaces and the sizes should be explicit. Error 
codes between KVM and CCP can be anything (unsigned long, u64) as it is 
the same binary.

 > snp_setup_guest_buf() return value will need to be
> fixed accordingly.
> 
>> +    unsigned long exitcode;
>>       u64 data_gpa;
>> +    int err, rc;
>>       if (!sev_snp_guest(vcpu->kvm)) {
>>           rc = SEV_RET_INVALID_GUEST;
>> @@ -3669,17 +3687,16 @@ static void 
>> snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>>        */
>>       if (sev->snp_certs_len) {
>>           if ((data_npages << PAGE_SHIFT) < sev->snp_certs_len) {
>> -            rc = -EINVAL;
>> -            err = SNP_GUEST_REQ_INVALID_LEN;
>> +            rc = -ENOSPC;
> 
> Why do we need to introduce ENOSPC error code?

To distinguish it from other errors and return SNP_GUEST_REQ_INVALID_LEN 
when needed (the commit log mentions this).


> If we continue to use SNP_GUEST_REQ_INVALID_LEN we don't need to map 
> ENOSPC to SNP_GUEST_REQ_INVALID_LEN below.
> And the CCP driver can return SNP_GUEST_REQ_INVALID_LEN as earlier via 
> the fw_err parameter.

imho this is a bad idea.

SNP_GUEST_REQ_INVALID_LEN is defined in the GHCB spec and GHCB is 
between KVM and VM, /dev/sev is neither GHCB nor KVM. err here is for 
the firmware errors but SNP_GUEST_REQ_INVALID_LEN is not from the 
firmware and for not-from-the-firmware-errors we already have "return 
rc" so lets just use that. Also err is 32bit across the place, in things 
like sev_issue_cmd() and then there is this ugly cast to int*. Thanks,


> 
> Thanks,
> Ashish
> 
>>               goto datalen;
>>           }
>> -        rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
>> -                   (int *)&err);
>> +        rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req, &err);
>>       } else {
>> -        rc = snp_guest_ext_guest_request(&req,
>> -                         (unsigned long)sev->snp_certs_data,
>> -                         &data_npages, &err);
>> +        rc = sev_issue_cmd_cert(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
>> +                    (unsigned long)sev->snp_certs_data,
>> +                    &data_npages, &err);
>>       }
>> +
>>   datalen:
>>       if (sev->snp_certs_len)
>>           data_npages = sev->snp_certs_len >> PAGE_SHIFT;
>> @@ -3689,27 +3706,30 @@ static void 
>> snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>>            * If buffer length is small then return the expected
>>            * length in rbx.
>>            */
>> -        if (err == SNP_GUEST_REQ_INVALID_LEN)
>> +        if (rc == -ENOSPC) {
>>               vcpu->arch.regs[VCPU_REGS_RBX] = data_npages;
>> +            exitcode = SNP_GUEST_REQ_INVALID_LEN;
>> +            goto cleanup;
>> +        }
>>           /* pass the firmware error code */
>> -        rc = err;
>> +        exitcode = 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;
>> +        exitcode = SEV_RET_INVALID_ADDRESS;
>>   cleanup:
>> -    snp_cleanup_guest_buf(&req, &rc);
>> +    snp_cleanup_guest_buf(&req, &exitcode);
>>   unlock:
>>       mutex_unlock(&sev->guest_req_lock);
>>   e_fail:
>> -    svm_set_ghcb_sw_exit_info_2(vcpu, rc);
>> +    svm_set_ghcb_sw_exit_info_2(vcpu, exitcode);
>>   }
>>   static kvm_pfn_t gfn_to_pfn_restricted(struct kvm *kvm, gfn_t gfn)
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 6c4fdcaed72b..73f56c20255c 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -2070,8 +2070,8 @@ int snp_guest_dbg_decrypt_page(u64 gctx_pfn, u64 
>> src_pfn, u64 dst_pfn, int *erro
>>   }
>>   EXPORT_SYMBOL_GPL(snp_guest_dbg_decrypt_page);
>> -int snp_guest_ext_guest_request(struct sev_data_snp_guest_request *data,
>> -                unsigned long vaddr, unsigned long *npages, unsigned 
>> long *fw_err)
>> +int sev_issue_cmd_external_user_cert(struct file *filep, unsigned int 
>> cmd, void *data,
>> +                     unsigned long vaddr, unsigned long *npages, int 
>> *error)
>>   {
>>       unsigned long expected_npages;
>>       struct sev_device *sev;
>> @@ -2093,12 +2093,11 @@ int snp_guest_ext_guest_request(struct 
>> sev_data_snp_guest_request *data,
>>       expected_npages = sev->snp_certs_len >> PAGE_SHIFT;
>>       if (*npages < expected_npages) {
>>           *npages = expected_npages;
>> -        *fw_err = SNP_GUEST_REQ_INVALID_LEN;
>>           mutex_unlock(&sev->snp_certs_lock);
>> -        return -EINVAL;
>> +        return -ENOSPC;
>>       }
>> -    rc = sev_do_cmd(SEV_CMD_SNP_GUEST_REQUEST, data, (int *)fw_err);
>> +    rc = sev_issue_cmd_external_user(filep, cmd, data, error);
>>       if (rc) {
>>           mutex_unlock(&sev->snp_certs_lock);
>>           return rc;
>> @@ -2115,7 +2114,7 @@ int snp_guest_ext_guest_request(struct 
>> sev_data_snp_guest_request *data,
>>       mutex_unlock(&sev->snp_certs_lock);
>>       return rc;
>>   }
>> -EXPORT_SYMBOL_GPL(snp_guest_ext_guest_request);
>> +EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user_cert);
>>   static void sev_exit(struct kref *ref)
>>   {
>>

-- 
Alexey

Powered by blists - more mailing lists