[PATCH 1/2] x86/sev: Add KVM commands for instance certs

[PATCH 1/2] x86/sev: Add KVM commands for instance certs

[Dionna Glaze]

The /dev/sev device has the ability to store host-wide certificates for
the key used by the AMD-SP for SEV-SNP attestation report signing,
but for hosts that want to specify additional certificates that are
specific to the image launched in a VM, we need a different way to
communicate those certificates.

This patch adds two new KVM ioctl commands: KVM_SEV_SNP_{GET,SET}_CERTS

The certificates that are set with this command are expected to follow
the same format as the host certificates, but that format is opaque
to the kernel.

The new behavior for custom certificates is that the extended guest
request command will now return the overridden certificates if they
were installed for the instance. The error condition for a too small
data buffer is changed to return the overridden certificate data size
if there is an overridden certificate set installed.

Setting a 0 length certificate returns the system state to only return
the host certificates on an extended guest request.

We also increase the SEV_FW_BLOB_MAX_SIZE another 4K page to allow
space for an extra certificate.

Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/kvm/svm/sev.c   | 111 ++++++++++++++++++++++++++++++++++++++-
 arch/x86/kvm/svm/svm.h   |   1 +
 include/linux/psp-sev.h  |   2 +-
 include/uapi/linux/kvm.h |  12 +++++
 4 files changed, 123 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index a35cd9f33f16..f1d846081213 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1636,6 +1636,7 @@ static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		goto e_free;
 
 	sev->snp_certs_data = certs_data;
+	sev->snp_certs_len = 0;
 
 	return context;
 
@@ -1940,6 +1941,86 @@ static int snp_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int snp_get_instance_certs(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct kvm_sev_snp_get_certs params;
+
+	if (!sev_snp_guest(kvm))
+		return -ENOTTY;
+
+	if (!sev->snp_context)
+		return -EINVAL;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+			   sizeof(params)))
+		return -EFAULT;
+
+	/* No instance certs set. */
+	if (!sev->snp_certs_len)
+		return -ENOENT;
+
+	if (params.certs_len < sev->snp_certs_len) {
+		/* Output buffer too small. Return the required size. */
+		params.certs_len = sev->snp_certs_len;
+
+		if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+				 sizeof(params)))
+			return -EFAULT;
+
+		return -EINVAL;
+	}
+
+	if (copy_to_user((void __user *)(uintptr_t)params.certs_uaddr,
+			 sev->snp_certs_data, sev->snp_certs_len))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int snp_set_instance_certs(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct kvm_sev_snp_set_certs params;
+	void *new_certs = NULL, *to_certs = sev->snp_certs_data;
+	unsigned long length = SEV_FW_BLOB_MAX_SIZE;
+
+	if (!sev_snp_guest(kvm))
+		return -ENOTTY;
+
+	if (!sev->snp_context)
+		return -EINVAL;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+			   sizeof(params)))
+		return -EFAULT;
+
+	if (params.certs_len > SEV_FW_BLOB_MAX_SIZE)
+		return -EINVAL;
+
+	/*
+	 * Setting a length of 0 is the same as "uninstalling" instance-
+	 * specific certificates.
+	 */
+	if (params.certs_len == 0) {
+		sev->snp_certs_len = 0;
+		return 0;
+	}
+
+	/* Page-align the length */
+	length = (params.certs_len + PAGE_SIZE - 1) & PAGE_MASK;
+
+	if (copy_from_user(to_certs,
+			   (void __user *)(uintptr_t)params.certs_uaddr,
+			   params.certs_len)) {
+		return -EFAULT;
+	}
+
+	sev->snp_certs_len = length;
+
+	return 0;
+}
+
 int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;
@@ -2038,6 +2119,12 @@ int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_SNP_LAUNCH_FINISH:
 		r = snp_launch_finish(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SNP_GET_CERTS:
+		r = snp_get_instance_certs(kvm, &sev_cmd);
+		break;
+	case KVM_SEV_SNP_SET_CERTS:
+		r = snp_set_instance_certs(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;
@@ -3361,8 +3448,28 @@ static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
 	if (rc)
 		goto unlock;
 
-	rc = snp_guest_ext_guest_request(&req, (unsigned long)sev->snp_certs_data,
-					 &data_npages, &err);
+	/*
+	 * If the VMM has overridden the certs, then we change the error message
+	 * if the size is inappropriate for the override. Otherwise we use a
+	 * regular guest request and copy back the instance certs.
+	 */
+	if (sev->snp_certs_len) {
+		if ((data_npages << PAGE_SHIFT) < sev->snp_certs_len) {
+			rc = -EINVAL;
+			err = SNP_GUEST_REQ_INVALID_LEN;
+			goto datalen;
+		}
+		rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
+				   (int *)&err);
+	} else {
+		rc = snp_guest_ext_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;
+
 	if (rc) {
 		/*
 		 * If buffer length is small then return the expected
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index e68b3aab57d6..9030a295cdf5 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -93,6 +93,7 @@ struct kvm_sev_info {
 	void *snp_context;      /* SNP guest context page */
 	struct srcu_struct psc_srcu;
 	void *snp_certs_data;
+	unsigned int snp_certs_len; /* Size of instance override for certs */
 	struct mutex guest_req_lock;
 
 	u64 sev_features;	/* Features set at VMSA creation */
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 859bbbcd5fa3..3d1811ffd9af 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -24,7 +24,7 @@
 #define __psp_pa(x)	__pa(x)
 #endif
 
-#define SEV_FW_BLOB_MAX_SIZE	0x4000	/* 16KB */
+#define SEV_FW_BLOB_MAX_SIZE	0x5000	/* 20KB */
 
 /**
  * SEV platform state
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index db9eb36da3ec..d47b36dc681d 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1717,6 +1717,8 @@ enum sev_cmd_id {
 	KVM_SEV_SNP_LAUNCH_START,
 	KVM_SEV_SNP_LAUNCH_UPDATE,
 	KVM_SEV_SNP_LAUNCH_FINISH,
+	KVM_SEV_SNP_GET_CERTS,
+	KVM_SEV_SNP_SET_CERTS,
 
 	KVM_SEV_NR_MAX,
 };
@@ -1864,6 +1866,16 @@ struct kvm_sev_snp_launch_finish {
 	__u8 pad[6];
 };
 
+struct kvm_sev_snp_get_certs {
+	__u64 certs_uaddr;
+	__u64 certs_len;
+};
+
+struct kvm_sev_snp_set_certs {
+	__u64 certs_uaddr;
+	__u64 certs_len;
+};
+
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
 #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
-- 
2.37.2.789.g6183377224-goog

[PATCH 2/2] x86/sev: Document KVM_SEV_SNP_{G,S}ET_CERTS

[Dionna Glaze]

Update the KVM_MEMORY_ENCRYPT_OP documentation to include the new
commands for overriding the host certificates that the guest receives
from an extended guest request.

Cc: Thomas Lendacky <Thomas.Lendacky@amd.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 .../virt/kvm/amd-memory-encryption.rst        | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
index c7332e0e0baa..699bde86948e 100644
--- a/Documentation/virt/kvm/amd-memory-encryption.rst
+++ b/Documentation/virt/kvm/amd-memory-encryption.rst
@@ -529,6 +529,50 @@ Returns: 0 on success, -negative on error
 
 See SEV-SNP specification for further details on launch finish input parameters.
 
+22. KVM_SEV_SNP_GET_CERTS
+-------------------------
+
+After the SNP guest launch flow has started, the KVM_SEV_SNP_GET_CERTS command
+can be issued to request the data that has been installed with the
+KVM_SEV_SNP_SET_CERTS command.
+
+Parameters (in/out): struct kvm_sev_snp_get_certs
+
+Returns: 0 on success, -negative on error
+
+::
+
+	struct kvm_sev_snp_get_certs {
+		__u64 certs_uaddr;
+		__u64 certs_len
+	};
+
+If no certs have been installed, then the return value is -ENOENT.
+If the buffer specified in the struct is too small, the certs_len field will be
+overwritten with the required bytes to receive all the certificate bytes and the
+return value will be -EINVAL.
+
+23. KVM_SEV_SNP_SET_CERTS
+-------------------------
+
+After the SNP guest launch flow has started, the KVM_SEV_SNP_SET_CERTS command
+can be issued to override the /dev/sev certs data that is returned when a
+guest issues an extended guest request. This is useful for instance-specific
+extensions to the host certificates.
+
+Parameters (in/out): struct kvm_sev_snp_set_certs
+
+Returns: 0 on success, -negative on error
+
+::
+
+	struct kvm_sev_snp_set_certs {
+		__u64 certs_uaddr;
+		__u64 certs_len
+	};
+
+The certs_len field may not exceed SEV_FW_BLOB_MAX_SIZE.
+
 References
 ==========
 
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH 1/2] x86/sev: Add KVM commands for instance certs

[Kalra, Ashish]

Hello Dionna,

On 9/1/2022 7:04 PM, Dionna Glaze wrote:
> The /dev/sev device has the ability to store host-wide certificates for
> the key used by the AMD-SP for SEV-SNP attestation report signing,
> but for hosts that want to specify additional certificates that are
> specific to the image launched in a VM, we need a different way to
> communicate those certificates.
> 
> This patch adds two new KVM ioctl commands: KVM_SEV_SNP_{GET,SET}_CERTS
> 
> The certificates that are set with this command are expected to follow
> the same format as the host certificates, but that format is opaque
> to the kernel.
> 
> The new behavior for custom certificates is that the extended guest
> request command will now return the overridden certificates if they
> were installed for the instance. The error condition for a too small
> data buffer is changed to return the overridden certificate data size
> if there is an overridden certificate set installed.
> 
> Setting a 0 length certificate returns the system state to only return
> the host certificates on an extended guest request.
> 
> We also increase the SEV_FW_BLOB_MAX_SIZE another 4K page to allow
> space for an extra certificate.
> 
> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>   arch/x86/kvm/svm/sev.c   | 111 ++++++++++++++++++++++++++++++++++++++-
>   arch/x86/kvm/svm/svm.h   |   1 +
>   include/linux/psp-sev.h  |   2 +-
>   include/uapi/linux/kvm.h |  12 +++++
>   4 files changed, 123 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index a35cd9f33f16..f1d846081213 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1636,6 +1636,7 @@ static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   		goto e_free;
>   
>   	sev->snp_certs_data = certs_data;
> +	sev->snp_certs_len = 0;
>   
>   	return context;
>   
> @@ -1940,6 +1941,86 @@ static int snp_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	return ret;
>   }
>   
> +static int snp_get_instance_certs(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	struct kvm_sev_snp_get_certs params;
> +
> +	if (!sev_snp_guest(kvm))
> +		return -ENOTTY;
> +
> +	if (!sev->snp_context)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> +			   sizeof(params)))
> +		return -EFAULT;
> +
> +	/* No instance certs set. */
> +	if (!sev->snp_certs_len)
> +		return -ENOENT;
> +
> +	if (params.certs_len < sev->snp_certs_len) {
> +		/* Output buffer too small. Return the required size. */
> +		params.certs_len = sev->snp_certs_len;
> +
> +		if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
> +				 sizeof(params)))
> +			return -EFAULT;
> +
> +		return -EINVAL;
> +	}
> +
> +	if (copy_to_user((void __user *)(uintptr_t)params.certs_uaddr,
> +			 sev->snp_certs_data, sev->snp_certs_len))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int snp_set_instance_certs(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> +	struct kvm_sev_snp_set_certs params;
> +	void *new_certs = NULL, *to_certs = sev->snp_certs_data;
> +	unsigned long length = SEV_FW_BLOB_MAX_SIZE;
> +
> +	if (!sev_snp_guest(kvm))
> +		return -ENOTTY;
> +
> +	if (!sev->snp_context)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> +			   sizeof(params)))
> +		return -EFAULT;
> +
> +	if (params.certs_len > SEV_FW_BLOB_MAX_SIZE)
> +		return -EINVAL;
> +
> +	/*
> +	 * Setting a length of 0 is the same as "uninstalling" instance-
> +	 * specific certificates.
> +	 */
> +	if (params.certs_len == 0) {
> +		sev->snp_certs_len = 0;
> +		return 0;
> +	}
> +
> +	/* Page-align the length */
> +	length = (params.certs_len + PAGE_SIZE - 1) & PAGE_MASK;

Probably can use PAGE_ALIGN() here.

> +
> +	if (copy_from_user(to_certs,
> +			   (void __user *)(uintptr_t)params.certs_uaddr,
> +			   params.certs_len)) {
> +		return -EFAULT;
> +	}
> +
> +	sev->snp_certs_len = length;
> +
> +	return 0;
> +}
> +
>   int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
>   {
>   	struct kvm_sev_cmd sev_cmd;
> @@ -2038,6 +2119,12 @@ int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
>   	case KVM_SEV_SNP_LAUNCH_FINISH:
>   		r = snp_launch_finish(kvm, &sev_cmd);
>   		break;
> +	case KVM_SEV_SNP_GET_CERTS:
> +		r = snp_get_instance_certs(kvm, &sev_cmd);
> +		break;
> +	case KVM_SEV_SNP_SET_CERTS:
> +		r = snp_set_instance_certs(kvm, &sev_cmd);
> +		break;
>   	default:
>   		r = -EINVAL;
>   		goto out;
> @@ -3361,8 +3448,28 @@ static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp
>   	if (rc)
>   		goto unlock;
>   
> -	rc = snp_guest_ext_guest_request(&req, (unsigned long)sev->snp_certs_data,
> -					 &data_npages, &err);
> +	/*
> +	 * If the VMM has overridden the certs, then we change the error message
> +	 * if the size is inappropriate for the override. Otherwise we use a
> +	 * regular guest request and copy back the instance certs.
> +	 */
> +	if (sev->snp_certs_len) {
> +		if ((data_npages << PAGE_SHIFT) < sev->snp_certs_len) {
> +			rc = -EINVAL;
> +			err = SNP_GUEST_REQ_INVALID_LEN;
> +			goto datalen;
> +		}
> +		rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
> +				   (int *)&err);
> +	} else {
> +		rc = snp_guest_ext_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;
> +
>   	if (rc) {
>   		/*
>   		 * If buffer length is small then return the expected
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index e68b3aab57d6..9030a295cdf5 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -93,6 +93,7 @@ struct kvm_sev_info {
>   	void *snp_context;      /* SNP guest context page */
>   	struct srcu_struct psc_srcu;
>   	void *snp_certs_data;
> +	unsigned int snp_certs_len; /* Size of instance override for certs */
>   	struct mutex guest_req_lock;
>   
>   	u64 sev_features;	/* Features set at VMSA creation */
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 859bbbcd5fa3..3d1811ffd9af 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -24,7 +24,7 @@
>   #define __psp_pa(x)	__pa(x)
>   #endif
>   
> -#define SEV_FW_BLOB_MAX_SIZE	0x4000	/* 16KB */
> +#define SEV_FW_BLOB_MAX_SIZE	0x5000	/* 20KB */
>   
>   /**
>    * SEV platform state
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index db9eb36da3ec..d47b36dc681d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1717,6 +1717,8 @@ enum sev_cmd_id {
>   	KVM_SEV_SNP_LAUNCH_START,
>   	KVM_SEV_SNP_LAUNCH_UPDATE,
>   	KVM_SEV_SNP_LAUNCH_FINISH,
> +	KVM_SEV_SNP_GET_CERTS,
> +	KVM_SEV_SNP_SET_CERTS,
>   
>   	KVM_SEV_NR_MAX,
>   };
> @@ -1864,6 +1866,16 @@ struct kvm_sev_snp_launch_finish {
>   	__u8 pad[6];
>   };
>   
> +struct kvm_sev_snp_get_certs {
> +	__u64 certs_uaddr;
> +	__u64 certs_len;
> +};
> +
> +struct kvm_sev_snp_set_certs {
> +	__u64 certs_uaddr;
> +	__u64 certs_len;
> +};
> +
>   #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
>   #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
>   #define KVM_DEV_ASSIGN_MASK_INTX	(1 << 2)
> 

Otherwise, patch looks good to me.

Reviewed-by: Ashish Kalra <ashish.kalra@amd.com>

Thanks,
Ashish

Re: [PATCH 1/2] x86/sev: Add KVM commands for instance certs

[Kalra, Ashish]

On 10/12/2022 6:39 PM, Kalra, Ashish wrote:
> Hello Dionna,
> 
> On 9/1/2022 7:04 PM, Dionna Glaze wrote:
>> The /dev/sev device has the ability to store host-wide certificates for
>> the key used by the AMD-SP for SEV-SNP attestation report signing,
>> but for hosts that want to specify additional certificates that are
>> specific to the image launched in a VM, we need a different way to
>> communicate those certificates.
>>
>> This patch adds two new KVM ioctl commands: KVM_SEV_SNP_{GET,SET}_CERTS
>>
>> The certificates that are set with this command are expected to follow
>> the same format as the host certificates, but that format is opaque
>> to the kernel.
>>
>> The new behavior for custom certificates is that the extended guest
>> request command will now return the overridden certificates if they
>> were installed for the instance. The error condition for a too small
>> data buffer is changed to return the overridden certificate data size
>> if there is an overridden certificate set installed.
>>
>> Setting a 0 length certificate returns the system state to only return
>> the host certificates on an extended guest request.
>>
>> We also increase the SEV_FW_BLOB_MAX_SIZE another 4K page to allow
>> space for an extra certificate.
>>
>> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
>> ---
>>   arch/x86/kvm/svm/sev.c   | 111 ++++++++++++++++++++++++++++++++++++++-
>>   arch/x86/kvm/svm/svm.h   |   1 +
>>   include/linux/psp-sev.h  |   2 +-
>>   include/uapi/linux/kvm.h |  12 +++++
>>   4 files changed, 123 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index a35cd9f33f16..f1d846081213 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -1636,6 +1636,7 @@ static void *snp_context_create(struct kvm *kvm, 
>> struct kvm_sev_cmd *argp)
>>           goto e_free;
>>       sev->snp_certs_data = certs_data;
>> +    sev->snp_certs_len = 0;
>>       return context;
>> @@ -1940,6 +1941,86 @@ static int snp_launch_finish(struct kvm *kvm, 
>> struct kvm_sev_cmd *argp)
>>       return ret;
>>   }
>> +static int snp_get_instance_certs(struct kvm *kvm, struct kvm_sev_cmd 
>> *argp)
>> +{
>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +    struct kvm_sev_snp_get_certs params;
>> +
>> +    if (!sev_snp_guest(kvm))
>> +        return -ENOTTY;
>> +
>> +    if (!sev->snp_context)
>> +        return -EINVAL;
>> +
>> +    if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
>> +               sizeof(params)))
>> +        return -EFAULT;
>> +
>> +    /* No instance certs set. */
>> +    if (!sev->snp_certs_len)
>> +        return -ENOENT;
>> +
>> +    if (params.certs_len < sev->snp_certs_len) {
>> +        /* Output buffer too small. Return the required size. */
>> +        params.certs_len = sev->snp_certs_len;
>> +
>> +        if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
>> +                 sizeof(params)))
>> +            return -EFAULT;
>> +
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (copy_to_user((void __user *)(uintptr_t)params.certs_uaddr,
>> +             sev->snp_certs_data, sev->snp_certs_len))
>> +        return -EFAULT;
>> +
>> +    return 0;
>> +}
>> +
>> +static int snp_set_instance_certs(struct kvm *kvm, struct kvm_sev_cmd 
>> *argp)
>> +{
>> +    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> +    struct kvm_sev_snp_set_certs params;
>> +    void *new_certs = NULL, *to_certs = sev->snp_certs_data;
>> +    unsigned long length = SEV_FW_BLOB_MAX_SIZE;
>> +
>> +    if (!sev_snp_guest(kvm))
>> +        return -ENOTTY;
>> +
>> +    if (!sev->snp_context)
>> +        return -EINVAL;
>> +
>> +    if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
>> +               sizeof(params)))
>> +        return -EFAULT;
>> +
>> +    if (params.certs_len > SEV_FW_BLOB_MAX_SIZE)
>> +        return -EINVAL;
>> +
>> +    /*
>> +     * Setting a length of 0 is the same as "uninstalling" instance-
>> +     * specific certificates.
>> +     */
>> +    if (params.certs_len == 0) {
>> +        sev->snp_certs_len = 0;
>> +        return 0;
>> +    }
>> +
>> +    /* Page-align the length */
>> +    length = (params.certs_len + PAGE_SIZE - 1) & PAGE_MASK;
> 
> Probably can use PAGE_ALIGN() here.
> 
>> +
>> +    if (copy_from_user(to_certs,
>> +               (void __user *)(uintptr_t)params.certs_uaddr,
>> +               params.certs_len)) {
>> +        return -EFAULT;
>> +    }
>> +
>> +    sev->snp_certs_len = length;
>> +
>> +    return 0;
>> +}
>> +
>>   int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
>>   {
>>       struct kvm_sev_cmd sev_cmd;
>> @@ -2038,6 +2119,12 @@ int svm_mem_enc_op(struct kvm *kvm, void __user 
>> *argp)
>>       case KVM_SEV_SNP_LAUNCH_FINISH:
>>           r = snp_launch_finish(kvm, &sev_cmd);
>>           break;
>> +    case KVM_SEV_SNP_GET_CERTS:
>> +        r = snp_get_instance_certs(kvm, &sev_cmd);
>> +        break;
>> +    case KVM_SEV_SNP_SET_CERTS:
>> +        r = snp_set_instance_certs(kvm, &sev_cmd);
>> +        break;
>>       default:
>>           r = -EINVAL;
>>           goto out;
>> @@ -3361,8 +3448,28 @@ static void snp_handle_ext_guest_request(struct 
>> vcpu_svm *svm, gpa_t req_gpa, gp
>>       if (rc)
>>           goto unlock;
>> -    rc = snp_guest_ext_guest_request(&req, (unsigned 
>> long)sev->snp_certs_data,
>> -                     &data_npages, &err);
>> +    /*
>> +     * If the VMM has overridden the certs, then we change the error 
>> message
>> +     * if the size is inappropriate for the override. Otherwise we use a
>> +     * regular guest request and copy back the instance certs.
>> +     */
>> +    if (sev->snp_certs_len) {
>> +        if ((data_npages << PAGE_SHIFT) < sev->snp_certs_len) {
>> +            rc = -EINVAL;
>> +            err = SNP_GUEST_REQ_INVALID_LEN;
>> +            goto datalen;
>> +        }
>> +        rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &req,
>> +                   (int *)&err);

Though, one thing i don't understand is that why do we need to issue
the SNP_GUEST_REQUEST to FW if we are going to return the VMM
overriden certs back to the guest ?

Thanks,
Ashish

>> +    } else {
>> +        rc = snp_guest_ext_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;
>> +
>>       if (rc) {
>>           /*
>>            * If buffer length is small then return the expected
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> index e68b3aab57d6..9030a295cdf5 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -93,6 +93,7 @@ struct kvm_sev_info {
>>       void *snp_context;      /* SNP guest context page */
>>       struct srcu_struct psc_srcu;
>>       void *snp_certs_data;
>> +    unsigned int snp_certs_len; /* Size of instance override for 
>> certs */
>>       struct mutex guest_req_lock;
>>       u64 sev_features;    /* Features set at VMSA creation */
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 859bbbcd5fa3..3d1811ffd9af 100644
>> --- a/include/linux/psp-sev.h
>> +++ b/include/linux/psp-sev.h
>> @@ -24,7 +24,7 @@
>>   #define __psp_pa(x)    __pa(x)
>>   #endif
>> -#define SEV_FW_BLOB_MAX_SIZE    0x4000    /* 16KB */
>> +#define SEV_FW_BLOB_MAX_SIZE    0x5000    /* 20KB */
>>   /**
>>    * SEV platform state
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index db9eb36da3ec..d47b36dc681d 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1717,6 +1717,8 @@ enum sev_cmd_id {
>>       KVM_SEV_SNP_LAUNCH_START,
>>       KVM_SEV_SNP_LAUNCH_UPDATE,
>>       KVM_SEV_SNP_LAUNCH_FINISH,
>> +    KVM_SEV_SNP_GET_CERTS,
>> +    KVM_SEV_SNP_SET_CERTS,
>>       KVM_SEV_NR_MAX,
>>   };
>> @@ -1864,6 +1866,16 @@ struct kvm_sev_snp_launch_finish {
>>       __u8 pad[6];
>>   };
>> +struct kvm_sev_snp_get_certs {
>> +    __u64 certs_uaddr;
>> +    __u64 certs_len;
>> +};
>> +
>> +struct kvm_sev_snp_set_certs {
>> +    __u64 certs_uaddr;
>> +    __u64 certs_len;
>> +};
>> +
>>   #define KVM_DEV_ASSIGN_ENABLE_IOMMU    (1 << 0)
>>   #define KVM_DEV_ASSIGN_PCI_2_3        (1 << 1)
>>   #define KVM_DEV_ASSIGN_MASK_INTX    (1 << 2)
>>
> 
> Otherwise, patch looks good to me.
> 
> Reviewed-by: Ashish Kalra <ashish.kalra@amd.com>
> 
> Thanks,
> Ashish

Re: [PATCH 1/2] x86/sev: Add KVM commands for instance certs

[Dionna Amalie Glaze]

> >> +    /* Page-align the length */
> >> +    length = (params.certs_len + PAGE_SIZE - 1) & PAGE_MASK;
> >
> > Probably can use PAGE_ALIGN() here.
> >

Ah, thanks. Will add in v2.

>
> Though, one thing i don't understand is that why do we need to issue
> the SNP_GUEST_REQUEST to FW if we are going to return the VMM
> overriden certs back to the guest ?
>
> Thanks,
> Ashish

If I'm reading the spec right, certs are supposed to come along with
the guest request when the user issues an extended guest request. If
the length is correct, we issue the command to get the report and we
simply override what the psp returns for the certs.

Is that your understanding too? If so, are you saying there's a bug in
this implementation?


-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH 1/2] x86/sev: Add KVM commands for instance certs

[Tom Lendacky]

On 10/12/22 20:02, Dionna Amalie Glaze wrote:
>>>> +    /* Page-align the length */
>>>> +    length = (params.certs_len + PAGE_SIZE - 1) & PAGE_MASK;
>>>
>>> Probably can use PAGE_ALIGN() here.
>>>
> 
> Ah, thanks. Will add in v2.
> 
>>
>> Though, one thing i don't understand is that why do we need to issue
>> the SNP_GUEST_REQUEST to FW if we are going to return the VMM
>> overriden certs back to the guest ?
>>
>> Thanks,
>> Ashish
> 
> If I'm reading the spec right, certs are supposed to come along with
> the guest request when the user issues an extended guest request. If
> the length is correct, we issue the command to get the report and we
> simply override what the psp returns for the certs.

The SNP Extended Guest Request doesn't override anything from the PSP, it 
just supplies additional data associated with the MSG_REPORT_REQ so that 
an additional call does not have to be made to obtain the certificate 
chain needed validate the report.

The idea is to not have to make two calls, since, theoretically, it is 
possible for the guest to be migrated in between a MSG_REPORT_REQ call and 
the call to obtain the certificates, at which point the VCEK would not match.

Thanks,
Tom

> 
> Is that your understanding too? If so, are you saying there's a bug in
> this implementation?
> 
>