Message-ID: <f18fae8b-a928-cd82-e0b3-eac62ad3e106@amd.com>
Date:   Mon, 6 Feb 2023 15:57:11 -0600
From:   "Kalra, Ashish" <ashish.kalra@....com>
To:     Alexey Kardashevskiy <aik@....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 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.

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

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.

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)
>   {
> 

Powered by blists - more mailing lists