Add AMD SEV guest live migration support

From: Brijesh Singh <[email protected]>

The command is used to create an outgoing SEV guest encryption context.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../virt/kvm/amd-memory-encryption.rst        |  27 ++++
 arch/x86/kvm/svm.c                            | 128 ++++++++++++++++++
 include/linux/psp-sev.h                       |   8 +-
 include/uapi/linux/kvm.h                      |  12 ++
 4 files changed, 171 insertions(+), 4 deletions(-)

Documentation/virt/kvm/amd-memory-encryption.rst +8 -0

index c3129b9ba5cb..4fd34fc5c7a7 100644
@@ -263,6 +263,33 @@ Returns: 0 on success, -negative on error
                 __u32 trans_len;
         };
 
+10. KVM_SEV_SEND_START
+----------------------
+
+The KVM_SEV_SEND_START command can be used by the hypervisor to create an
+outgoing guest encryption context.
+
+Parameters (in): struct kvm_sev_send_start
+

Documentation/virt/kvm/amd-memory-encryption.rst +5 -0

+
+                __u64 session_uaddr;          /* Guest session information */
+                __u32 session_len;
+        };
+
 References
 ==========
 

arch/x86/kvm/svm.c +95 -0

index 50d1ebafe0b3..63d172e974ad 100644
@@ -7149,6 +7149,131 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+/* Userspace wants to query session length. */
+static int
+__sev_send_start_query_session_length(struct kvm *kvm, struct kvm_sev_cmd *argp,
+				      struct kvm_sev_send_start *params)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	params->session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;
+
+	kfree(data);
+	return ret;
+}
+
+static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	struct kvm_sev_send_start params;
+	void *amd_certs, *session_data;
+	void *pdh_cert, *plat_certs;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+				sizeof(struct kvm_sev_send_start)))
+		return -EFAULT;
+
+	/* if session_len is zero, userspace wants to query the session length */
+	if (!params.session_len)
+		return __sev_send_start_query_session_length(kvm, argp,
+				&params);
+
+	/* some sanity checks */
+	if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
+	    !params.session_uaddr || params.session_len > SEV_FW_BLOB_MAX_SIZE)
+		return -EINVAL;
+
+	/* allocate the memory to hold the session data blob */
+	session_data = kmalloc(params.session_len, GFP_KERNEL_ACCOUNT);
+	if (!session_data)
+		return -ENOMEM;
+
+	/* copy the certificate blobs from userspace */
+	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr,
+				params.pdh_cert_len);
+	if (IS_ERR(pdh_cert)) {
+		ret = PTR_ERR(pdh_cert);
+		goto e_free_session;
+	}
+
+	plat_certs = psp_copy_user_blob(params.plat_certs_uaddr,
+				params.plat_certs_len);
+	if (IS_ERR(plat_certs)) {
+		ret = PTR_ERR(plat_certs);
+		goto e_free_pdh;
+	}
+
+	amd_certs = psp_copy_user_blob(params.amd_certs_uaddr,
+				params.amd_certs_len);
+	if (IS_ERR(amd_certs)) {
+		ret = PTR_ERR(amd_certs);
+		goto e_free_plat_cert;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL) {
+		ret = -ENOMEM;
+		goto e_free_amd_cert;
+	}
+
+	/* populate the FW SEND_START field with system physical address */
+	data->pdh_cert_address = __psp_pa(pdh_cert);
+	data->pdh_cert_len = params.pdh_cert_len;
+	data->plat_certs_address = __psp_pa(plat_certs);
+	data->plat_certs_len = params.plat_certs_len;
+	data->amd_certs_address = __psp_pa(amd_certs);
+	data->amd_certs_len = params.amd_certs_len;
+	data->session_address = __psp_pa(session_data);
+	data->session_len = params.session_len;
+	data->handle = sev->handle;
+

arch/x86/kvm/svm.c +17 -0

+
+e_free:
+	kfree(data);
+e_free_amd_cert:
+	kfree(amd_certs);
+e_free_plat_cert:
+	kfree(plat_certs);
+e_free_pdh:
+	kfree(pdh_cert);
+e_free_session:
+	kfree(session_data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;

@@ -7193,6 +7318,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_LAUNCH_SECRET:
 		r = sev_launch_secret(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_START:
+		r = sev_send_start(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;

include/linux/psp-sev.h +4 -4

index 5167bf2bfc75..9f63b9d48b63 100644
@@ -323,11 +323,11 @@ struct sev_data_send_start {
 	u64 pdh_cert_address;			/* In */
 	u32 pdh_cert_len;			/* In */
 	u32 reserved1;
-	u64 plat_cert_address;			/* In */
-	u32 plat_cert_len;			/* In */
+	u64 plat_certs_address;			/* In */
+	u32 plat_certs_len;			/* In */
 	u32 reserved2;
-	u64 amd_cert_address;			/* In */
-	u32 amd_cert_len;			/* In */
+	u64 amd_certs_address;			/* In */
+	u32 amd_certs_len;			/* In */
 	u32 reserved3;
 	u64 session_address;			/* In */
 	u32 session_len;			/* In/Out */

include/uapi/linux/kvm.h +1 -0

index 4b95f9a31a2f..17bef4c245e1 100644
@@ -1558,6 +1558,18 @@ struct kvm_sev_dbg {
 	__u32 len;
 };
 
+struct kvm_sev_send_start {

include/uapi/linux/kvm.h +1 -0

+
 #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)

Signature · 1 line

2.17.1

Quoted 41 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The command is used to create an outgoing SEV guest encryption context.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../virt/kvm/amd-memory-encryption.rst        |  27 ++++
 arch/x86/kvm/svm.c                            | 128 ++++++++++++++++++
 include/linux/psp-sev.h                       |   8 +-
 include/uapi/linux/kvm.h                      |  12 ++
 4 files changed, 171 insertions(+), 4 deletions(-)

Documentation/virt/kvm/amd-memory-encryption.rst +8 -0

index c3129b9ba5cb..4fd34fc5c7a7 100644
@@ -263,6 +263,33 @@ Returns: 0 on success, -negative on error
                 __u32 trans_len;
         };
 
+10. KVM_SEV_SEND_START
+----------------------
+
+The KVM_SEV_SEND_START command can be used by the hypervisor to create an
+outgoing guest encryption context.
+
+Parameters (in): struct kvm_sev_send_start
+

Documentation/virt/kvm/amd-memory-encryption.rst Returns: 0 on success, -negative on error · line 283

Quoted 10 lines

Documentation/virt/kvm/amd-memory-encryption.rst +10 -0

+Returns: 0 on success, -negative on error
+
+::
+        struct kvm_sev_send_start {
+                __u32 policy;                 /* guest policy */
+
+                __u64 pdh_cert_uaddr;         /* platform Diffie-Hellman certificate */
+                __u32 pdh_cert_len;
+
+                __u64 plat_certs_uadr;        /* platform certificate chain */

Could this please be changed to plat_certs_uaddr, as it is referred to in the rest of the code?

Documentation/virt/kvm/amd-memory-encryption.rst Returns: 0 on success, -negative on error · line 287

Quoted 4 lines

Documentation/virt/kvm/amd-memory-encryption.rst +4 -0

+                __u32 plat_certs_len;
+
+                __u64 amd_certs_uaddr;        /* AMD certificate */
+                __u32 amd_cert_len;

Could this please be changed to amd_certs_len, as it is referred to in the rest of the code?

Quoted 111 lines

Documentation/virt/kvm/amd-memory-encryption.rst +5 -0

+
+                __u64 session_uaddr;          /* Guest session information */
+                __u32 session_len;
+        };
+
 References
 ==========
 

arch/x86/kvm/svm.c +95 -0

index 50d1ebafe0b3..63d172e974ad 100644
@@ -7149,6 +7149,131 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+/* Userspace wants to query session length. */
+static int
+__sev_send_start_query_session_length(struct kvm *kvm, struct kvm_sev_cmd *argp,
+				      struct kvm_sev_send_start *params)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	params->session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;
+
+	kfree(data);
+	return ret;
+}
+
+static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	struct kvm_sev_send_start params;
+	void *amd_certs, *session_data;
+	void *pdh_cert, *plat_certs;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+				sizeof(struct kvm_sev_send_start)))
+		return -EFAULT;
+
+	/* if session_len is zero, userspace wants to query the session length */
+	if (!params.session_len)
+		return __sev_send_start_query_session_length(kvm, argp,
+				&params);
+
+	/* some sanity checks */
+	if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
+	    !params.session_uaddr || params.session_len > SEV_FW_BLOB_MAX_SIZE)
+		return -EINVAL;
+
+	/* allocate the memory to hold the session data blob */
+	session_data = kmalloc(params.session_len, GFP_KERNEL_ACCOUNT);
+	if (!session_data)
+		return -ENOMEM;
+
+	/* copy the certificate blobs from userspace */
+	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr,
+				params.pdh_cert_len);
+	if (IS_ERR(pdh_cert)) {
+		ret = PTR_ERR(pdh_cert);
+		goto e_free_session;
+	}
+
+	plat_certs = psp_copy_user_blob(params.plat_certs_uaddr,
+				params.plat_certs_len);
+	if (IS_ERR(plat_certs)) {
+		ret = PTR_ERR(plat_certs);
+		goto e_free_pdh;
+	}
+
+	amd_certs = psp_copy_user_blob(params.amd_certs_uaddr,
+				params.amd_certs_len);
+	if (IS_ERR(amd_certs)) {
+		ret = PTR_ERR(amd_certs);
+		goto e_free_plat_cert;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL) {
+		ret = -ENOMEM;
+		goto e_free_amd_cert;
+	}
+
+	/* populate the FW SEND_START field with system physical address */
+	data->pdh_cert_address = __psp_pa(pdh_cert);
+	data->pdh_cert_len = params.pdh_cert_len;
+	data->plat_certs_address = __psp_pa(plat_certs);
+	data->plat_certs_len = params.plat_certs_len;
+	data->amd_certs_address = __psp_pa(amd_certs);
+	data->amd_certs_len = params.amd_certs_len;
+	data->session_address = __psp_pa(session_data);
+	data->session_len = params.session_len;
+	data->handle = sev->handle;
+

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7256

Quoted 10 lines

arch/x86/kvm/svm.c +10 -0

+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	if (ret)
+		goto e_free;
+
+	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
+			session_data, params.session_len)) {
+		ret = -EFAULT;
+		goto e_free;
+	}

To optimize the amount of data being copied to user space, could the above section of code changed as follows?

	params.session_len = data->session_len;
	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
			session_data, params.session_len)) {
		ret = -EFAULT;
		goto e_free;
	}

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7262

Quoted 6 lines

arch/x86/kvm/svm.c +6 -0

+
+	params.policy = data->policy;
+	params.session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;

Since the only fields that are changed in the kvm_sev_send_start structure are session_len and policy, why do we need to copy the entire structure back to the user? Why not just those two values? Please see the changes proposed to kvm_sev_send_start structure further below to accomplish this.

	params.policy = data->policy;
	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
			sizeof(params.policy) + sizeof(params.session_len))
		ret = -EFAULT;

Quoted 56 lines

arch/x86/kvm/svm.c +17 -0

+
+e_free:
+	kfree(data);
+e_free_amd_cert:
+	kfree(amd_certs);
+e_free_plat_cert:
+	kfree(plat_certs);
+e_free_pdh:
+	kfree(pdh_cert);
+e_free_session:
+	kfree(session_data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;

@@ -7193,6 +7318,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_LAUNCH_SECRET:
 		r = sev_launch_secret(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_START:
+		r = sev_send_start(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;

include/linux/psp-sev.h +4 -4

index 5167bf2bfc75..9f63b9d48b63 100644
@@ -323,11 +323,11 @@ struct sev_data_send_start {
 	u64 pdh_cert_address;			/* In */
 	u32 pdh_cert_len;			/* In */
 	u32 reserved1;
-	u64 plat_cert_address;			/* In */
-	u32 plat_cert_len;			/* In */
+	u64 plat_certs_address;			/* In */
+	u32 plat_certs_len;			/* In */
 	u32 reserved2;
-	u64 amd_cert_address;			/* In */
-	u32 amd_cert_len;			/* In */
+	u64 amd_certs_address;			/* In */
+	u32 amd_certs_len;			/* In */
 	u32 reserved3;
 	u64 session_address;			/* In */
 	u32 session_len;			/* In/Out */

include/uapi/linux/kvm.h +1 -0

index 4b95f9a31a2f..17bef4c245e1 100644
@@ -1558,6 +1558,18 @@ struct kvm_sev_dbg {
 	__u32 len;
 };
 
+struct kvm_sev_send_start {

include/uapi/linux/kvm.h struct kvm_sev_dbg { · line 1571

Quoted 10 lines

include/uapi/linux/kvm.h +10 -0

+	__u32 policy;
+	__u64 pdh_cert_uaddr;
+	__u32 pdh_cert_len;
+	__u64 plat_certs_uaddr;
+	__u32 plat_certs_len;
+	__u64 amd_certs_uaddr;
+	__u32 amd_certs_len;
+	__u64 session_uaddr;
+	__u32 session_len;
+};

Redo this structure as below:

struct kvm_sev_send_start {
	__u32 policy;
	__u32 session_len;
	__u64 session_uaddr;
	__u64 pdh_cert_uaddr;
	__u32 pdh_cert_len;
	__u64 plat_certs_uaddr;
	__u32 plat_certs_len;
	__u64 amd_certs_uaddr;
	__u32 amd_certs_len;
};

Or as below, just to make it look better.

struct kvm_sev_send_start {
	__u32 policy;
	__u32 session_len;
	__u64 session_uaddr;
	__u32 pdh_cert_len;
	__u64 pdh_cert_uaddr;
	__u32 plat_certs_len;
	__u64 plat_certs_uaddr;
	__u32 amd_certs_len;
	__u64 amd_certs_uaddr;
};

Quoted 7 lines

include/uapi/linux/kvm.h +1 -0

+
 #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)

Signature · 1 line

2.17.1

Hi Venu,

Thanks for the feedback.

Quoted 186 lines · Venu Busireddy

Quoted 41 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The command is used to create an outgoing SEV guest encryption context.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../virt/kvm/amd-memory-encryption.rst        |  27 ++++
 arch/x86/kvm/svm.c                            | 128 ++++++++++++++++++
 include/linux/psp-sev.h                       |   8 +-
 include/uapi/linux/kvm.h                      |  12 ++
 4 files changed, 171 insertions(+), 4 deletions(-)

Documentation/virt/kvm/amd-memory-encryption.rst +8 -0

index c3129b9ba5cb..4fd34fc5c7a7 100644
@@ -263,6 +263,33 @@ Returns: 0 on success, -negative on error
                 __u32 trans_len;
         };
 
+10. KVM_SEV_SEND_START
+----------------------
+
+The KVM_SEV_SEND_START command can be used by the hypervisor to create an
+outgoing guest encryption context.
+
+Parameters (in): struct kvm_sev_send_start
+

Documentation/virt/kvm/amd-memory-encryption.rst Returns: 0 on success, -negative on error · line 283

Quoted 10 lines

Documentation/virt/kvm/amd-memory-encryption.rst +10 -0

+Returns: 0 on success, -negative on error
+
+::
+        struct kvm_sev_send_start {
+                __u32 policy;                 /* guest policy */
+
+                __u64 pdh_cert_uaddr;         /* platform Diffie-Hellman certificate */
+                __u32 pdh_cert_len;
+
+                __u64 plat_certs_uadr;        /* platform certificate chain */

Could this please be changed to plat_certs_uaddr, as it is referred to in the rest of the code?

Documentation/virt/kvm/amd-memory-encryption.rst Returns: 0 on success, -negative on error · line 287

Quoted 4 lines

Documentation/virt/kvm/amd-memory-encryption.rst +4 -0

+                __u32 plat_certs_len;
+
+                __u64 amd_certs_uaddr;        /* AMD certificate */
+                __u32 amd_cert_len;

Could this please be changed to amd_certs_len, as it is referred to in the rest of the code?

Quoted 111 lines

Documentation/virt/kvm/amd-memory-encryption.rst +5 -0

+
+                __u64 session_uaddr;          /* Guest session information */
+                __u32 session_len;
+        };
+
 References
 ==========
 

arch/x86/kvm/svm.c +95 -0

index 50d1ebafe0b3..63d172e974ad 100644
@@ -7149,6 +7149,131 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+/* Userspace wants to query session length. */
+static int
+__sev_send_start_query_session_length(struct kvm *kvm, struct kvm_sev_cmd *argp,
+				      struct kvm_sev_send_start *params)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	params->session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;
+
+	kfree(data);
+	return ret;
+}
+
+static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	struct kvm_sev_send_start params;
+	void *amd_certs, *session_data;
+	void *pdh_cert, *plat_certs;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+				sizeof(struct kvm_sev_send_start)))
+		return -EFAULT;
+
+	/* if session_len is zero, userspace wants to query the session length */
+	if (!params.session_len)
+		return __sev_send_start_query_session_length(kvm, argp,
+				&params);
+
+	/* some sanity checks */
+	if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
+	    !params.session_uaddr || params.session_len > SEV_FW_BLOB_MAX_SIZE)
+		return -EINVAL;
+
+	/* allocate the memory to hold the session data blob */
+	session_data = kmalloc(params.session_len, GFP_KERNEL_ACCOUNT);
+	if (!session_data)
+		return -ENOMEM;
+
+	/* copy the certificate blobs from userspace */
+	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr,
+				params.pdh_cert_len);
+	if (IS_ERR(pdh_cert)) {
+		ret = PTR_ERR(pdh_cert);
+		goto e_free_session;
+	}
+
+	plat_certs = psp_copy_user_blob(params.plat_certs_uaddr,
+				params.plat_certs_len);
+	if (IS_ERR(plat_certs)) {
+		ret = PTR_ERR(plat_certs);
+		goto e_free_pdh;
+	}
+
+	amd_certs = psp_copy_user_blob(params.amd_certs_uaddr,
+				params.amd_certs_len);
+	if (IS_ERR(amd_certs)) {
+		ret = PTR_ERR(amd_certs);
+		goto e_free_plat_cert;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL) {
+		ret = -ENOMEM;
+		goto e_free_amd_cert;
+	}
+
+	/* populate the FW SEND_START field with system physical address */
+	data->pdh_cert_address = __psp_pa(pdh_cert);
+	data->pdh_cert_len = params.pdh_cert_len;
+	data->plat_certs_address = __psp_pa(plat_certs);
+	data->plat_certs_len = params.plat_certs_len;
+	data->amd_certs_address = __psp_pa(amd_certs);
+	data->amd_certs_len = params.amd_certs_len;
+	data->session_address = __psp_pa(session_data);
+	data->session_len = params.session_len;
+	data->handle = sev->handle;
+

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7256

Quoted 10 lines

arch/x86/kvm/svm.c +10 -0

+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	if (ret)
+		goto e_free;
+
+	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
+			session_data, params.session_len)) {
+		ret = -EFAULT;
+		goto e_free;
+	}

To optimize the amount of data being copied to user space, could the above section of code changed as follows?

Quoted 6 lines

	params.session_len = data->session_len;
	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
			session_data, params.session_len)) {
		ret = -EFAULT;
		goto e_free;
	}

We should not be using the data->session_len, it will cause -EFAULT when user has not allocated enough space in the session_uaddr. Lets consider the case where user passes session_len=10 but firmware thinks the session length should be 64. In that case the data->session_len will contains a value of 64 but userspace has allocated space for 10 bytes and copy_to_user() will fail. If we are really concern about the amount of data getting copied to userspace then use min_t(size_t, params.session_len, data->session_len).

Quoted 10 lines

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7262

Quoted 6 lines

arch/x86/kvm/svm.c +6 -0

+
+	params.policy = data->policy;
+	params.session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;

Since the only fields that are changed in the kvm_sev_send_start structure are session_len and policy, why do we need to copy the entire structure back to the user? Why not just those two values? Please see the changes proposed to kvm_sev_send_start structure further below to accomplish this.

I think we also need to consider the code readability while saving the CPU cycles. This is very small structure. By duplicating into two calls #1 copy params.policy and #2 copy params.session_len we will add more CPU cycle. And, If we get creative and rearrange the structure then code readability is lost because now the copy will depend on how the structure is layout in the memory.

Quoted 88 lines

	params.policy = data->policy;
	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
			sizeof(params.policy) + sizeof(params.session_len))
		ret = -EFAULT;

Quoted 56 lines

arch/x86/kvm/svm.c +17 -0

+
+e_free:
+	kfree(data);
+e_free_amd_cert:
+	kfree(amd_certs);
+e_free_plat_cert:
+	kfree(plat_certs);
+e_free_pdh:
+	kfree(pdh_cert);
+e_free_session:
+	kfree(session_data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;

@@ -7193,6 +7318,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_LAUNCH_SECRET:
 		r = sev_launch_secret(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_START:
+		r = sev_send_start(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;

include/linux/psp-sev.h +4 -4

index 5167bf2bfc75..9f63b9d48b63 100644
@@ -323,11 +323,11 @@ struct sev_data_send_start {
 	u64 pdh_cert_address;			/* In */
 	u32 pdh_cert_len;			/* In */
 	u32 reserved1;
-	u64 plat_cert_address;			/* In */
-	u32 plat_cert_len;			/* In */
+	u64 plat_certs_address;			/* In */
+	u32 plat_certs_len;			/* In */
 	u32 reserved2;
-	u64 amd_cert_address;			/* In */
-	u32 amd_cert_len;			/* In */
+	u64 amd_certs_address;			/* In */
+	u32 amd_certs_len;			/* In */
 	u32 reserved3;
 	u64 session_address;			/* In */
 	u32 session_len;			/* In/Out */

include/uapi/linux/kvm.h +1 -0

index 4b95f9a31a2f..17bef4c245e1 100644
@@ -1558,6 +1558,18 @@ struct kvm_sev_dbg {
 	__u32 len;
 };
 
+struct kvm_sev_send_start {

include/uapi/linux/kvm.h struct kvm_sev_dbg { · line 1571

Quoted 10 lines

include/uapi/linux/kvm.h +10 -0

+	__u32 policy;
+	__u64 pdh_cert_uaddr;
+	__u32 pdh_cert_len;
+	__u64 plat_certs_uaddr;
+	__u32 plat_certs_len;
+	__u64 amd_certs_uaddr;
+	__u32 amd_certs_len;
+	__u64 session_uaddr;
+	__u32 session_len;
+};

Redo this structure as below:

struct kvm_sev_send_start {
	__u32 policy;
	__u32 session_len;
	__u64 session_uaddr;
	__u64 pdh_cert_uaddr;
	__u32 pdh_cert_len;
	__u64 plat_certs_uaddr;
	__u32 plat_certs_len;
	__u64 amd_certs_uaddr;
	__u32 amd_certs_len;
};

Or as below, just to make it look better.

Quoted 12 lines

struct kvm_sev_send_start {
	__u32 policy;
	__u32 session_len;
	__u64 session_uaddr;
	__u32 pdh_cert_len;
	__u64 pdh_cert_uaddr;
	__u32 plat_certs_len;
	__u64 plat_certs_uaddr;
	__u32 amd_certs_len;
	__u64 amd_certs_uaddr;
};

Wherever applicable, I tried  best to not divert from the SEV spec structure layout. Anyone who is reading the SEV FW spec  will see a similar structure layout in the KVM/PSP header files. I would prefer to stick to that approach.

Quoted 9 lines

Quoted 9 lines

include/uapi/linux/kvm.h +1 -0

+
 #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)

Signature · 1 line

2.17.1

Quoted 198 lines · Brijesh Singh

Hi Venu,

Thanks for the feedback.

Quoted 186 lines · Venu Busireddy

Quoted 41 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The command is used to create an outgoing SEV guest encryption context.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../virt/kvm/amd-memory-encryption.rst        |  27 ++++
 arch/x86/kvm/svm.c                            | 128 ++++++++++++++++++
 include/linux/psp-sev.h                       |   8 +-
 include/uapi/linux/kvm.h                      |  12 ++
 4 files changed, 171 insertions(+), 4 deletions(-)

Documentation/virt/kvm/amd-memory-encryption.rst +8 -0

index c3129b9ba5cb..4fd34fc5c7a7 100644
@@ -263,6 +263,33 @@ Returns: 0 on success, -negative on error
                 __u32 trans_len;
         };
 
+10. KVM_SEV_SEND_START
+----------------------
+
+The KVM_SEV_SEND_START command can be used by the hypervisor to create an
+outgoing guest encryption context.
+
+Parameters (in): struct kvm_sev_send_start
+

Documentation/virt/kvm/amd-memory-encryption.rst Returns: 0 on success, -negative on error · line 283

Quoted 10 lines

Documentation/virt/kvm/amd-memory-encryption.rst +10 -0

+Returns: 0 on success, -negative on error
+
+::
+        struct kvm_sev_send_start {
+                __u32 policy;                 /* guest policy */
+
+                __u64 pdh_cert_uaddr;         /* platform Diffie-Hellman certificate */
+                __u32 pdh_cert_len;
+
+                __u64 plat_certs_uadr;        /* platform certificate chain */

Could this please be changed to plat_certs_uaddr, as it is referred to in the rest of the code?

Documentation/virt/kvm/amd-memory-encryption.rst Returns: 0 on success, -negative on error · line 287

Quoted 4 lines

Documentation/virt/kvm/amd-memory-encryption.rst +4 -0

+                __u32 plat_certs_len;
+
+                __u64 amd_certs_uaddr;        /* AMD certificate */
+                __u32 amd_cert_len;

Could this please be changed to amd_certs_len, as it is referred to in the rest of the code?

Quoted 111 lines

Documentation/virt/kvm/amd-memory-encryption.rst +5 -0

+
+                __u64 session_uaddr;          /* Guest session information */
+                __u32 session_len;
+        };
+
 References
 ==========
 

arch/x86/kvm/svm.c +95 -0

index 50d1ebafe0b3..63d172e974ad 100644
@@ -7149,6 +7149,131 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+/* Userspace wants to query session length. */
+static int
+__sev_send_start_query_session_length(struct kvm *kvm, struct kvm_sev_cmd *argp,
+				      struct kvm_sev_send_start *params)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	params->session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;
+
+	kfree(data);
+	return ret;
+}
+
+static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	struct kvm_sev_send_start params;
+	void *amd_certs, *session_data;
+	void *pdh_cert, *plat_certs;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+				sizeof(struct kvm_sev_send_start)))
+		return -EFAULT;
+
+	/* if session_len is zero, userspace wants to query the session length */
+	if (!params.session_len)
+		return __sev_send_start_query_session_length(kvm, argp,
+				&params);
+
+	/* some sanity checks */
+	if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
+	    !params.session_uaddr || params.session_len > SEV_FW_BLOB_MAX_SIZE)
+		return -EINVAL;
+
+	/* allocate the memory to hold the session data blob */
+	session_data = kmalloc(params.session_len, GFP_KERNEL_ACCOUNT);
+	if (!session_data)
+		return -ENOMEM;
+
+	/* copy the certificate blobs from userspace */
+	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr,
+				params.pdh_cert_len);
+	if (IS_ERR(pdh_cert)) {
+		ret = PTR_ERR(pdh_cert);
+		goto e_free_session;
+	}
+
+	plat_certs = psp_copy_user_blob(params.plat_certs_uaddr,
+				params.plat_certs_len);
+	if (IS_ERR(plat_certs)) {
+		ret = PTR_ERR(plat_certs);
+		goto e_free_pdh;
+	}
+
+	amd_certs = psp_copy_user_blob(params.amd_certs_uaddr,
+				params.amd_certs_len);
+	if (IS_ERR(amd_certs)) {
+		ret = PTR_ERR(amd_certs);
+		goto e_free_plat_cert;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL) {
+		ret = -ENOMEM;
+		goto e_free_amd_cert;
+	}
+
+	/* populate the FW SEND_START field with system physical address */
+	data->pdh_cert_address = __psp_pa(pdh_cert);
+	data->pdh_cert_len = params.pdh_cert_len;
+	data->plat_certs_address = __psp_pa(plat_certs);
+	data->plat_certs_len = params.plat_certs_len;
+	data->amd_certs_address = __psp_pa(amd_certs);
+	data->amd_certs_len = params.amd_certs_len;
+	data->session_address = __psp_pa(session_data);
+	data->session_len = params.session_len;
+	data->handle = sev->handle;
+

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7256

Quoted 10 lines

arch/x86/kvm/svm.c +10 -0

+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	if (ret)
+		goto e_free;
+
+	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
+			session_data, params.session_len)) {
+		ret = -EFAULT;
+		goto e_free;
+	}

To optimize the amount of data being copied to user space, could the above section of code changed as follows?

Quoted 6 lines

	params.session_len = data->session_len;
	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
			session_data, params.session_len)) {
		ret = -EFAULT;
		goto e_free;
	}

Quoted 8 lines

We should not be using the data->session_len, it will cause -EFAULT when user has not allocated enough space in the session_uaddr. Lets consider the case where user passes session_len=10 but firmware thinks the session length should be 64. In that case the data->session_len will contains a value of 64 but userspace has allocated space for 10 bytes and copy_to_user() will fail. If we are really concern about the amount of data getting copied to userspace then use min_t(size_t, params.session_len, data->session_len).

We are allocating a buffer of params.session_len size and passing that buffer, and that length via data->session_len, to the firmware. Why would the firmware set data->session_len to a larger value, in spite of telling it that the buffer is only params.session_len long? I thought that only the reverse is possible, that is, the user sets the params.session_len to the MAX, but the session data is actually smaller than that size.

Also, if for whatever reason the firmware sets data->session_len to a larger value than what is passed, what is the user space expected to do when the call returns? If the user space tries to access params.session_len amount of data, it will possibly get a memory access violation, because it did not originally allocate that large a buffer.

If we do go with using min_t(size_t, params.session_len, data->session_len), then params.session_len should also be set to the smaller of the two, right?

Quoted 10 lines

Quoted 10 lines

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7262

Quoted 6 lines

arch/x86/kvm/svm.c +6 -0

+
+	params.policy = data->policy;
+	params.session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;

Since the only fields that are changed in the kvm_sev_send_start structure are session_len and policy, why do we need to copy the entire structure back to the user? Why not just those two values? Please see the changes proposed to kvm_sev_send_start structure further below to accomplish this.

Quoted 6 lines

I think we also need to consider the code readability while saving the CPU cycles. This is very small structure. By duplicating into two calls #1 copy params.policy and #2 copy params.session_len we will add more CPU cycle. And, If we get creative and rearrange the structure then code readability is lost because now the copy will depend on how the structure is layout in the memory.

I was not recommending splitting that call into two. That would certainly be more expensive, than copying the entire structure. That is the reason why I suggested reordering the members of kvm_sev_send_start. Isn't there plenty of code where structures are defined in a way to keep the data movement efficient? :-)

Please see my other comment below.

Quoted 98 lines

Quoted 86 lines

	params.policy = data->policy;
	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
			sizeof(params.policy) + sizeof(params.session_len))
		ret = -EFAULT;

Quoted 56 lines

arch/x86/kvm/svm.c +17 -0

+
+e_free:
+	kfree(data);
+e_free_amd_cert:
+	kfree(amd_certs);
+e_free_plat_cert:
+	kfree(plat_certs);
+e_free_pdh:
+	kfree(pdh_cert);
+e_free_session:
+	kfree(session_data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;

@@ -7193,6 +7318,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_LAUNCH_SECRET:
 		r = sev_launch_secret(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_START:
+		r = sev_send_start(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;

include/linux/psp-sev.h +4 -4

index 5167bf2bfc75..9f63b9d48b63 100644
@@ -323,11 +323,11 @@ struct sev_data_send_start {
 	u64 pdh_cert_address;			/* In */
 	u32 pdh_cert_len;			/* In */
 	u32 reserved1;
-	u64 plat_cert_address;			/* In */
-	u32 plat_cert_len;			/* In */
+	u64 plat_certs_address;			/* In */
+	u32 plat_certs_len;			/* In */
 	u32 reserved2;
-	u64 amd_cert_address;			/* In */
-	u32 amd_cert_len;			/* In */
+	u64 amd_certs_address;			/* In */
+	u32 amd_certs_len;			/* In */
 	u32 reserved3;
 	u64 session_address;			/* In */
 	u32 session_len;			/* In/Out */

include/uapi/linux/kvm.h +1 -0

index 4b95f9a31a2f..17bef4c245e1 100644
@@ -1558,6 +1558,18 @@ struct kvm_sev_dbg {
 	__u32 len;
 };
 
+struct kvm_sev_send_start {

include/uapi/linux/kvm.h struct kvm_sev_dbg { · line 1571

Quoted 10 lines

include/uapi/linux/kvm.h +10 -0

+	__u32 policy;
+	__u64 pdh_cert_uaddr;
+	__u32 pdh_cert_len;
+	__u64 plat_certs_uaddr;
+	__u32 plat_certs_len;
+	__u64 amd_certs_uaddr;
+	__u32 amd_certs_len;
+	__u64 session_uaddr;
+	__u32 session_len;
+};

Redo this structure as below:

struct kvm_sev_send_start {
	__u32 policy;
	__u32 session_len;
	__u64 session_uaddr;
	__u64 pdh_cert_uaddr;
	__u32 pdh_cert_len;
	__u64 plat_certs_uaddr;
	__u32 plat_certs_len;
	__u64 amd_certs_uaddr;
	__u32 amd_certs_len;
};

Or as below, just to make it look better.

Quoted 12 lines

struct kvm_sev_send_start {
	__u32 policy;
	__u32 session_len;
	__u64 session_uaddr;
	__u32 pdh_cert_len;
	__u64 pdh_cert_uaddr;
	__u32 plat_certs_len;
	__u64 plat_certs_uaddr;
	__u32 amd_certs_len;
	__u64 amd_certs_uaddr;
};

Quoted 4 lines

Wherever applicable, I tried  best to not divert from the SEV spec structure layout. Anyone who is reading the SEV FW spec  will see a similar structure layout in the KVM/PSP header files. I would prefer to stick to that approach.

This structure is in uapi, and is anyway different from the sev_data_send_start, right? Does it really need to stay close to the firmware structure layout? Just because the firmware folks thought of a structure layout, that should not prevent our code to be efficient.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

include/uapi/linux/kvm.h +1 -0

+
 #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)

Signature · 1 line

2.17.1

Quoted 207 lines · Venu Busireddy

Quoted 198 lines · Brijesh Singh

Hi Venu,

Thanks for the feedback.

Quoted 186 lines · Venu Busireddy

Quoted 41 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The command is used to create an outgoing SEV guest encryption context.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../virt/kvm/amd-memory-encryption.rst        |  27 ++++
 arch/x86/kvm/svm.c                            | 128 ++++++++++++++++++
 include/linux/psp-sev.h                       |   8 +-
 include/uapi/linux/kvm.h                      |  12 ++
 4 files changed, 171 insertions(+), 4 deletions(-)

Documentation/virt/kvm/amd-memory-encryption.rst +8 -0

index c3129b9ba5cb..4fd34fc5c7a7 100644
@@ -263,6 +263,33 @@ Returns: 0 on success, -negative on error
                 __u32 trans_len;
         };
 
+10. KVM_SEV_SEND_START
+----------------------
+
+The KVM_SEV_SEND_START command can be used by the hypervisor to create an
+outgoing guest encryption context.
+
+Parameters (in): struct kvm_sev_send_start
+

Documentation/virt/kvm/amd-memory-encryption.rst Returns: 0 on success, -negative on error · line 283

Quoted 10 lines

Documentation/virt/kvm/amd-memory-encryption.rst +10 -0

+Returns: 0 on success, -negative on error
+
+::
+        struct kvm_sev_send_start {
+                __u32 policy;                 /* guest policy */
+
+                __u64 pdh_cert_uaddr;         /* platform Diffie-Hellman certificate */
+                __u32 pdh_cert_len;
+
+                __u64 plat_certs_uadr;        /* platform certificate chain */

Could this please be changed to plat_certs_uaddr, as it is referred to in the rest of the code?

Documentation/virt/kvm/amd-memory-encryption.rst Returns: 0 on success, -negative on error · line 287

Quoted 4 lines

Documentation/virt/kvm/amd-memory-encryption.rst +4 -0

+                __u32 plat_certs_len;
+
+                __u64 amd_certs_uaddr;        /* AMD certificate */
+                __u32 amd_cert_len;

Could this please be changed to amd_certs_len, as it is referred to in the rest of the code?

Quoted 111 lines

Documentation/virt/kvm/amd-memory-encryption.rst +5 -0

+
+                __u64 session_uaddr;          /* Guest session information */
+                __u32 session_len;
+        };
+
 References
 ==========
 

arch/x86/kvm/svm.c +95 -0

index 50d1ebafe0b3..63d172e974ad 100644
@@ -7149,6 +7149,131 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+/* Userspace wants to query session length. */
+static int
+__sev_send_start_query_session_length(struct kvm *kvm, struct kvm_sev_cmd *argp,
+				      struct kvm_sev_send_start *params)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	params->session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;
+
+	kfree(data);
+	return ret;
+}
+
+static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	struct kvm_sev_send_start params;
+	void *amd_certs, *session_data;
+	void *pdh_cert, *plat_certs;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+				sizeof(struct kvm_sev_send_start)))
+		return -EFAULT;
+
+	/* if session_len is zero, userspace wants to query the session length */
+	if (!params.session_len)
+		return __sev_send_start_query_session_length(kvm, argp,
+				&params);
+
+	/* some sanity checks */
+	if (!params.pdh_cert_uaddr || !params.pdh_cert_len ||
+	    !params.session_uaddr || params.session_len > SEV_FW_BLOB_MAX_SIZE)
+		return -EINVAL;
+
+	/* allocate the memory to hold the session data blob */
+	session_data = kmalloc(params.session_len, GFP_KERNEL_ACCOUNT);
+	if (!session_data)
+		return -ENOMEM;
+
+	/* copy the certificate blobs from userspace */
+	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr,
+				params.pdh_cert_len);
+	if (IS_ERR(pdh_cert)) {
+		ret = PTR_ERR(pdh_cert);
+		goto e_free_session;
+	}
+
+	plat_certs = psp_copy_user_blob(params.plat_certs_uaddr,
+				params.plat_certs_len);
+	if (IS_ERR(plat_certs)) {
+		ret = PTR_ERR(plat_certs);
+		goto e_free_pdh;
+	}
+
+	amd_certs = psp_copy_user_blob(params.amd_certs_uaddr,
+				params.amd_certs_len);
+	if (IS_ERR(amd_certs)) {
+		ret = PTR_ERR(amd_certs);
+		goto e_free_plat_cert;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL) {
+		ret = -ENOMEM;
+		goto e_free_amd_cert;
+	}
+
+	/* populate the FW SEND_START field with system physical address */
+	data->pdh_cert_address = __psp_pa(pdh_cert);
+	data->pdh_cert_len = params.pdh_cert_len;
+	data->plat_certs_address = __psp_pa(plat_certs);
+	data->plat_certs_len = params.plat_certs_len;
+	data->amd_certs_address = __psp_pa(amd_certs);
+	data->amd_certs_len = params.amd_certs_len;
+	data->session_address = __psp_pa(session_data);
+	data->session_len = params.session_len;
+	data->handle = sev->handle;
+

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7256

Quoted 10 lines

arch/x86/kvm/svm.c +10 -0

+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	if (ret)
+		goto e_free;
+
+	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
+			session_data, params.session_len)) {
+		ret = -EFAULT;
+		goto e_free;
+	}

To optimize the amount of data being copied to user space, could the above section of code changed as follows?

Quoted 6 lines

	params.session_len = data->session_len;
	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
			session_data, params.session_len)) {
		ret = -EFAULT;
		goto e_free;
	}

Quoted 8 lines

We should not be using the data->session_len, it will cause -EFAULT when user has not allocated enough space in the session_uaddr. Lets consider the case where user passes session_len=10 but firmware thinks the session length should be 64. In that case the data->session_len will contains a value of 64 but userspace has allocated space for 10 bytes and copy_to_user() will fail. If we are really concern about the amount of data getting copied to userspace then use min_t(size_t, params.session_len, data->session_len).

Quoted 6 lines

We are allocating a buffer of params.session_len size and passing that buffer, and that length via data->session_len, to the firmware. Why would the firmware set data->session_len to a larger value, in spite of telling it that the buffer is only params.session_len long? I thought that only the reverse is possible, that is, the user sets the params.session_len to the MAX, but the session data is actually smaller than that size.

The question is, how does a userspace know the session length ? One method is you can precalculate a value based on your firmware version and have userspace pass that, or another approach is set params.session_len = 0 and query it from the FW. The FW spec allow to query the length, please see the spec. In the qemu patches I choose second approach. This is because session blob can change from one FW version to another and I tried to avoid calculating or hardcoding the length for a one version of the FW. You can certainly choose the first method. We want to ensure that kernel interface works on the both cases.

Quoted 151 lines

Also, if for whatever reason the firmware sets data->session_len to a larger value than what is passed, what is the user space expected to do when the call returns? If the user space tries to access params.session_len amount of data, it will possibly get a memory access violation, because it did not originally allocate that large a buffer.

If we do go with using min_t(size_t, params.session_len, data->session_len), then params.session_len should also be set to the smaller of the two, right?

Quoted 10 lines

Quoted 10 lines

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7262

Quoted 6 lines

arch/x86/kvm/svm.c +6 -0

+
+	params.policy = data->policy;
+	params.session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;

Since the only fields that are changed in the kvm_sev_send_start structure are session_len and policy, why do we need to copy the entire structure back to the user? Why not just those two values? Please see the changes proposed to kvm_sev_send_start structure further below to accomplish this.

Quoted 6 lines

I think we also need to consider the code readability while saving the CPU cycles. This is very small structure. By duplicating into two calls #1 copy params.policy and #2 copy params.session_len we will add more CPU cycle. And, If we get creative and rearrange the structure then code readability is lost because now the copy will depend on how the structure is layout in the memory.

I was not recommending splitting that call into two. That would certainly be more expensive, than copying the entire structure. That is the reason why I suggested reordering the members of kvm_sev_send_start. Isn't there plenty of code where structures are defined in a way to keep the data movement efficient? :-)

Please see my other comment below.

Quoted 98 lines

Quoted 86 lines

	params.policy = data->policy;
	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
			sizeof(params.policy) + sizeof(params.session_len))
		ret = -EFAULT;

Quoted 56 lines

arch/x86/kvm/svm.c +17 -0

+
+e_free:
+	kfree(data);
+e_free_amd_cert:
+	kfree(amd_certs);
+e_free_plat_cert:
+	kfree(plat_certs);
+e_free_pdh:
+	kfree(pdh_cert);
+e_free_session:
+	kfree(session_data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;

@@ -7193,6 +7318,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_LAUNCH_SECRET:
 		r = sev_launch_secret(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_START:
+		r = sev_send_start(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;

include/linux/psp-sev.h +4 -4

index 5167bf2bfc75..9f63b9d48b63 100644
@@ -323,11 +323,11 @@ struct sev_data_send_start {
 	u64 pdh_cert_address;			/* In */
 	u32 pdh_cert_len;			/* In */
 	u32 reserved1;
-	u64 plat_cert_address;			/* In */
-	u32 plat_cert_len;			/* In */
+	u64 plat_certs_address;			/* In */
+	u32 plat_certs_len;			/* In */
 	u32 reserved2;
-	u64 amd_cert_address;			/* In */
-	u32 amd_cert_len;			/* In */
+	u64 amd_certs_address;			/* In */
+	u32 amd_certs_len;			/* In */
 	u32 reserved3;
 	u64 session_address;			/* In */
 	u32 session_len;			/* In/Out */

include/uapi/linux/kvm.h +1 -0

index 4b95f9a31a2f..17bef4c245e1 100644
@@ -1558,6 +1558,18 @@ struct kvm_sev_dbg {
 	__u32 len;
 };
 
+struct kvm_sev_send_start {

include/uapi/linux/kvm.h struct kvm_sev_dbg { · line 1571

Quoted 10 lines

include/uapi/linux/kvm.h +10 -0

+	__u32 policy;
+	__u64 pdh_cert_uaddr;
+	__u32 pdh_cert_len;
+	__u64 plat_certs_uaddr;
+	__u32 plat_certs_len;
+	__u64 amd_certs_uaddr;
+	__u32 amd_certs_len;
+	__u64 session_uaddr;
+	__u32 session_len;
+};

Redo this structure as below:

struct kvm_sev_send_start {
	__u32 policy;
	__u32 session_len;
	__u64 session_uaddr;
	__u64 pdh_cert_uaddr;
	__u32 pdh_cert_len;
	__u64 plat_certs_uaddr;
	__u32 plat_certs_len;
	__u64 amd_certs_uaddr;
	__u32 amd_certs_len;
};

Or as below, just to make it look better.

Quoted 12 lines

struct kvm_sev_send_start {
	__u32 policy;
	__u32 session_len;
	__u64 session_uaddr;
	__u32 pdh_cert_len;
	__u64 pdh_cert_uaddr;
	__u32 plat_certs_len;
	__u64 plat_certs_uaddr;
	__u32 amd_certs_len;
	__u64 amd_certs_uaddr;
};

Quoted 4 lines

Wherever applicable, I tried  best to not divert from the SEV spec structure layout. Anyone who is reading the SEV FW spec  will see a similar structure layout in the KVM/PSP header files. I would prefer to stick to that approach.

This structure is in uapi, and is anyway different from the sev_data_send_start, right? Does it really need to stay close to the firmware structure layout? Just because the firmware folks thought of a structure layout, that should not prevent our code to be efficient.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

include/uapi/linux/kvm.h +1 -0

+
 #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)

Signature · 1 line

2.17.1

Quoted 9 lines

The question is, how does a userspace know the session length ? One method is you can precalculate a value based on your firmware version and have userspace pass that, or another approach is set params.session_len = 0 and query it from the FW. The FW spec allow to query the length, please see the spec. In the qemu patches I choose second approach. This is because session blob can change from one FW version to another and I tried to avoid calculating or hardcoding the length for a one version of the FW. You can certainly choose the first method. We want to ensure that kernel interface works on the both cases.

I like the fact that you have already implemented the functionality to facilitate the user space to obtain the session length from the firmware (by setting params.session_len to 0). However, I am trying to address the case where the user space sets the params.session_len to a size smaller than the size needed.

Let me put it differently. Let us say that the session blob needs 128 bytes, but the user space sets params.session_len to 16. That results in us allocating a buffer of 16 bytes, and set data->session_len to 16.

What does the firmware do now?

Does it copy 128 bytes into data->session_address, or, does it copy 16 bytes?

If it copies 128 bytes, we most certainly will end up with a kernel crash.

If it copies 16 bytes, then what does it set in data->session_len? 16, or 128? If 16, everything is good. If 128, we end up causing memory access violation for the user space.

Perhaps, this can be dealt a little differently? Why not always call sev_issue_cmd(kvm, SEV_CMD_SEND_START, ...) with zeroed out data? Then, if the user space has set params.session_len to 0, we return with the needed params.session_len. Otherwise, we check if params.session_len is large enough, and if not, we return -EINVAL?

On 4/2/20 1:57 PM, Venu Busireddy wrote:
[snip]...

Quoted 26 lines

Quoted 9 lines

The question is, how does a userspace know the session length ? One method is you can precalculate a value based on your firmware version and have userspace pass that, or another approach is set params.session_len = 0 and query it from the FW. The FW spec allow to query the length, please see the spec. In the qemu patches I choose second approach. This is because session blob can change from one FW version to another and I tried to avoid calculating or hardcoding the length for a one version of the FW. You can certainly choose the first method. We want to ensure that kernel interface works on the both cases.

I like the fact that you have already implemented the functionality to facilitate the user space to obtain the session length from the firmware (by setting params.session_len to 0). However, I am trying to address the case where the user space sets the params.session_len to a size smaller than the size needed.

Let me put it differently. Let us say that the session blob needs 128 bytes, but the user space sets params.session_len to 16. That results in us allocating a buffer of 16 bytes, and set data->session_len to 16.

What does the firmware do now?

Does it copy 128 bytes into data->session_address, or, does it copy 16 bytes?

If it copies 128 bytes, we most certainly will end up with a kernel crash.

Quoted 3 lines

If it copies 16 bytes, then what does it set in data->session_len? 16, or 128? If 16, everything is good. If 128, we end up causing memory access violation for the user space.

My interpretation of the spec is, if user provided length is smaller than the FW expected length then FW will reports an error with data->session_len set to the expected length. In other words, it should *not* copy anything into the session buffer in the event of failure.

If FW is touching memory beyond what is specified in the session_len then its FW bug and we can't do much from kernel.

Am I missing something ?

Quoted 5 lines

Perhaps, this can be dealt a little differently? Why not always call sev_issue_cmd(kvm, SEV_CMD_SEND_START, ...) with zeroed out data? Then, if the user space has set params.session_len to 0, we return with the needed params.session_len. Otherwise, we check if params.session_len is large enough, and if not, we return -EINVAL?

Quoted 29 lines · Brijesh Singh

On 4/2/20 1:57 PM, Venu Busireddy wrote:
[snip]...

Quoted 26 lines

Quoted 9 lines

The question is, how does a userspace know the session length ? One method is you can precalculate a value based on your firmware version and have userspace pass that, or another approach is set params.session_len = 0 and query it from the FW. The FW spec allow to query the length, please see the spec. In the qemu patches I choose second approach. This is because session blob can change from one FW version to another and I tried to avoid calculating or hardcoding the length for a one version of the FW. You can certainly choose the first method. We want to ensure that kernel interface works on the both cases.

I like the fact that you have already implemented the functionality to facilitate the user space to obtain the session length from the firmware (by setting params.session_len to 0). However, I am trying to address the case where the user space sets the params.session_len to a size smaller than the size needed.

Let me put it differently. Let us say that the session blob needs 128 bytes, but the user space sets params.session_len to 16. That results in us allocating a buffer of 16 bytes, and set data->session_len to 16.

What does the firmware do now?

Does it copy 128 bytes into data->session_address, or, does it copy 16 bytes?

If it copies 128 bytes, we most certainly will end up with a kernel crash.

Quoted 7 lines

Quoted 3 lines

If it copies 16 bytes, then what does it set in data->session_len? 16, or 128? If 16, everything is good. If 128, we end up causing memory access violation for the user space.

My interpretation of the spec is, if user provided length is smaller than the FW expected length then FW will reports an error with data->session_len set to the expected length. In other words, it should *not* copy anything into the session buffer in the event of failure.

That is good, and expected behavior.

Quoted 2 lines

If FW is touching memory beyond what is specified in the session_len then its FW bug and we can't do much from kernel.

Agreed. But let us assume that the firmware is not touching memory that it is not supposed to.

Quoted 1 line

Am I missing something ?

I believe you are agreeing that if the session blob needs 128 bytes and user space sets params.session_len to 16, the firmware does not copy any data to data->session_address, and sets data->session_len to 128.

Now, when we return, won't the user space try to access 128 bytes (params.session_len) of data in params.session_uaddr, and crash? Because, instead of returning an error that buffer is not large enough, we return the call successfully!

That is why I was suggesting the following, which you seem to have missed.

Quoted 5 lines

Quoted 5 lines

Perhaps, this can be dealt a little differently? Why not always call sev_issue_cmd(kvm, SEV_CMD_SEND_START, ...) with zeroed out data? Then, if the user space has set params.session_len to 0, we return with the needed params.session_len. Otherwise, we check if params.session_len is large enough, and if not, we return -EINVAL?

Doesn't the above approach address all scenarios?

Quoted 45 lines · Venu Busireddy

Quoted 29 lines · Brijesh Singh

On 4/2/20 1:57 PM, Venu Busireddy wrote:
[snip]...

Quoted 26 lines

Quoted 9 lines

The question is, how does a userspace know the session length ? One method is you can precalculate a value based on your firmware version and have userspace pass that, or another approach is set params.session_len = 0 and query it from the FW. The FW spec allow to query the length, please see the spec. In the qemu patches I choose second approach. This is because session blob can change from one FW version to another and I tried to avoid calculating or hardcoding the length for a one version of the FW. You can certainly choose the first method. We want to ensure that kernel interface works on the both cases.

I like the fact that you have already implemented the functionality to facilitate the user space to obtain the session length from the firmware (by setting params.session_len to 0). However, I am trying to address the case where the user space sets the params.session_len to a size smaller than the size needed.

Let me put it differently. Let us say that the session blob needs 128 bytes, but the user space sets params.session_len to 16. That results in us allocating a buffer of 16 bytes, and set data->session_len to 16.

What does the firmware do now?

Does it copy 128 bytes into data->session_address, or, does it copy 16 bytes?

If it copies 128 bytes, we most certainly will end up with a kernel crash.

Quoted 7 lines

Quoted 3 lines

If it copies 16 bytes, then what does it set in data->session_len? 16, or 128? If 16, everything is good. If 128, we end up causing memory access violation for the user space.

My interpretation of the spec is, if user provided length is smaller than the FW expected length then FW will reports an error with data->session_len set to the expected length. In other words, it should *not* copy anything into the session buffer in the event of failure.

That is good, and expected behavior.

Quoted 2 lines

If FW is touching memory beyond what is specified in the session_len then its FW bug and we can't do much from kernel.

Agreed. But let us assume that the firmware is not touching memory that it is not supposed to.

Quoted 1 line

Am I missing something ?

Quoted 8 lines

I believe you are agreeing that if the session blob needs 128 bytes and user space sets params.session_len to 16, the firmware does not copy any data to data->session_address, and sets data->session_len to 128.

Now, when we return, won't the user space try to access 128 bytes (params.session_len) of data in params.session_uaddr, and crash? Because, instead of returning an error that buffer is not large enough, we return the call successfully!

Ah, so the main issue is we should not be going to e_free on error. If session_len is less than the expected len then FW will return an error. In the case of an error we can skip copying the session_data into userspace buffer but we still need to pass the session_len and policy back to the userspace.

(quoted diff) +5 -0

+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	if (ret)
+		goto e_free;
+

If user space gets an error then it can decode it further to get additional information (e.g buffer too small).

Quoted 11 lines

That is why I was suggesting the following, which you seem to have missed.

Quoted 5 lines

Quoted 5 lines

Perhaps, this can be dealt a little differently? Why not always call sev_issue_cmd(kvm, SEV_CMD_SEND_START, ...) with zeroed out data? Then, if the user space has set params.session_len to 0, we return with the needed params.session_len. Otherwise, we check if params.session_len is large enough, and if not, we return -EINVAL?

Doesn't the above approach address all scenarios?

From: Brijesh Singh <[email protected]>

The command is used to create an outgoing SEV guest encryption context.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
  .../virt/kvm/amd-memory-encryption.rst        |  27 ++++
  arch/x86/kvm/svm.c                            | 128 ++++++++++++++++++
  include/linux/psp-sev.h                       |   8 +-
  include/uapi/linux/kvm.h                      |  12 ++
  4 files changed, 171 insertions(+), 4 deletions(-)

Documentation/virt/kvm/amd-memory-encryption.rst +0 -0

index c3129b9ba5cb..4fd34fc5c7a7 100644

Documentation/virt/kvm/amd-memory-encryption.rst +22 -0

+
+Parameters (in): struct kvm_sev_send_start
+
+Returns: 0 on success, -negative on error
+
+::
+        struct kvm_sev_send_start {
+                __u32 policy;                 /* guest policy */
+
+                __u64 pdh_cert_uaddr;         /* platform Diffie-Hellman certificate */
+                __u32 pdh_cert_len;
+
+                __u64 plat_certs_uadr;        /* platform certificate chain */
+                __u32 plat_certs_len;
+
+                __u64 amd_certs_uaddr;        /* AMD certificate */
+                __u32 amd_cert_len;
+
+                __u64 session_uaddr;          /* Guest session information */
+                __u32 session_len;
+        };
+
  References
  ==========
 

arch/x86/kvm/svm.c +0 -0

index 50d1ebafe0b3..63d172e974ad 100644

arch/x86/kvm/svm.c +14 -0

+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_start *data;
+	struct kvm_sev_send_start params;
+	void *amd_certs, *session_data;
+	void *pdh_cert, *plat_certs;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+				sizeof(struct kvm_sev_send_start)))
+		return -EFAULT;

arch/x86/kvm/svm.c +71 -0

+	pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr,
+				params.pdh_cert_len);
+	if (IS_ERR(pdh_cert)) {
+		ret = PTR_ERR(pdh_cert);
+		goto e_free_session;
+	}
+
+	plat_certs = psp_copy_user_blob(params.plat_certs_uaddr,
+				params.plat_certs_len);
+	if (IS_ERR(plat_certs)) {
+		ret = PTR_ERR(plat_certs);
+		goto e_free_pdh;
+	}
+
+	amd_certs = psp_copy_user_blob(params.amd_certs_uaddr,
+				params.amd_certs_len);
+	if (IS_ERR(amd_certs)) {
+		ret = PTR_ERR(amd_certs);
+		goto e_free_plat_cert;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (data == NULL) {
+		ret = -ENOMEM;
+		goto e_free_amd_cert;
+	}
+
+	/* populate the FW SEND_START field with system physical address */
+	data->pdh_cert_address = __psp_pa(pdh_cert);
+	data->pdh_cert_len = params.pdh_cert_len;
+	data->plat_certs_address = __psp_pa(plat_certs);
+	data->plat_certs_len = params.plat_certs_len;
+	data->amd_certs_address = __psp_pa(amd_certs);
+	data->amd_certs_len = params.amd_certs_len;
+	data->session_address = __psp_pa(session_data);
+	data->session_len = params.session_len;
+	data->handle = sev->handle;
+
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+	if (ret)
+		goto e_free;
+
+	if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
+			session_data, params.session_len)) {
+		ret = -EFAULT;
+		goto e_free;
+	}
+
+	params.policy = data->policy;
+	params.session_len = data->session_len;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+				sizeof(struct kvm_sev_send_start)))
+		ret = -EFAULT;
+
+e_free:
+	kfree(data);
+e_free_amd_cert:
+	kfree(amd_certs);
+e_free_plat_cert:
+	kfree(plat_certs);
+e_free_pdh:
+	kfree(pdh_cert);
+e_free_session:
+	kfree(session_data);
+	return ret;
+}
+
  static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
  {
  	struct kvm_sev_cmd sev_cmd;

@@ -7193,6 +7318,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
  	case KVM_SEV_LAUNCH_SECRET:
  		r = sev_launch_secret(kvm, &sev_cmd);
  		break;
+	case KVM_SEV_SEND_START:
+		r = sev_send_start(kvm, &sev_cmd);
+		break;
  	default:
  		r = -EINVAL;
  		goto out;

include/linux/psp-sev.h +0 -0

index 5167bf2bfc75..9f63b9d48b63 100644

include/linux/psp-sev.h +2 -2

  	u32 reserved2;
-	u64 amd_cert_address;			/* In */
-	u32 amd_cert_len;			/* In */
+	u64 amd_certs_address;			/* In */
+	u32 amd_certs_len;			/* In */
  	u32 reserved3;
  	u64 session_address;			/* In */
  	u32 session_len;			/* In/Out */

include/uapi/linux/kvm.h +12 -0

index 4b95f9a31a2f..17bef4c245e1 100644
@@ -1558,6 +1558,18 @@ struct kvm_sev_dbg {
  	__u32 len;
  };
 
+struct kvm_sev_send_start {
+	__u32 policy;
+	__u64 pdh_cert_uaddr;
+	__u32 pdh_cert_len;
+	__u64 plat_certs_uaddr;
+	__u32 plat_certs_len;
+	__u64 amd_certs_uaddr;
+	__u32 amd_certs_len;
+	__u64 session_uaddr;
+	__u32 session_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)

Quoted 25 lines · Ashish Kalra

From: Brijesh Singh <[email protected]>

The command is used to create an outgoing SEV guest encryption context.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
  .../virt/kvm/amd-memory-encryption.rst        |  27 ++++ arch/x86/kvm/svm.c                            | 128 ++++++++++++++++++ include/linux/psp-sev.h                       |   8 +- include/uapi/linux/kvm.h                      |  12 ++

  4 files changed, 171 insertions(+), 4 deletions(-)

Quoted 12 lines

Documentation/virt/kvm/amd-memory-encryption.rst +0 -0

index c3129b9ba5cb..4fd34fc5c7a7 100644
@@ -263,6 +263,33 @@ Returns: 0 on success, -negative on error
                   __u32 trans_len;
           };

  +10. KVM_SEV_SEND_START
+----------------------

Documentation/virt/kvm/amd-memory-encryption.rst +2 -0

+
+The KVM_SEV_SEND_START command can be used by the hypervisor to

Quoted 2 lines

create an
+outgoing guest encryption context.

Shouldn't we mention that this command is also used to save the guest to the disk ?

Quoted 38 lines

Documentation/virt/kvm/amd-memory-encryption.rst +10 -0

+
+Parameters (in): struct kvm_sev_send_start
+
+Returns: 0 on success, -negative on error
+
+::
+        struct kvm_sev_send_start {
+                __u32 policy;                 /* guest policy */
+
+                __u64 pdh_cert_uaddr;         /* platform

Diffie-Hellman certificate */
+                __u32 pdh_cert_len;
+
+                __u64 plat_certs_uadr;        /* platform certificate chain */
+                __u32 plat_certs_len;

Documentation/virt/kvm/amd-memory-encryption.rst +5 -0

+
+                __u64 amd_certs_uaddr;        /* AMD certificate */
+                __u32 amd_cert_len;
+
+                __u64 session_uaddr;          /* Guest session

information */
+                __u32 session_len;
+        };
+
  References
  ==========
  diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 50d1ebafe0b3..63d172e974ad 100644

arch/x86/kvm/svm.c +0 -0

@@ -7149,6 +7149,131 @@ static int sev_launch_secret(struct kvm *kvm,
 struct kvm_sev_cmd *argp)
       return ret;
   }

  +/* Userspace wants to query session length. */

arch/x86/kvm/svm.c +2 -0

+static int
+__sev_send_start_query_session_length(struct kvm *kvm, struct

Quoted 1 line

kvm_sev_cmd *argp,

__sev_query_send_start_session_length a better name perhaps ?

Quoted 9 lines

+                      struct kvm_sev_send_start *params)

arch/x86/kvm/svm.c +8 -0

+{
+    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+    struct sev_data_send_start *data;
+    int ret;
+
+    data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+    if (data == NULL)
+        return -ENOMEM;

Quoted 3 lines

+
+    data->handle = sev->handle;
+    ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);

We are not checking ret here as we are assuming that the command will always be successful. Is there any chance that sev->handle can be junk and should we have an ASSERT for it ?

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7177

Quoted 12 lines

arch/x86/kvm/svm.c +11 -0

+
+    params->session_len = data->session_len;
+    if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
+                sizeof(struct kvm_sev_send_start)))
+        ret = -EFAULT;
+
+    kfree(data);
+    return ret;
+}
+
+static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)

For readability and ease of cscope searches, isn't it better to append "svm" to all these functions ?

It seems svm_sev_enabled() is an example of an appropriate naming style.

Quoted 19 lines

arch/x86/kvm/svm.c +16 -0

+{
+    struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+    struct sev_data_send_start *data;
+    struct kvm_sev_send_start params;
+    void *amd_certs, *session_data;
+    void *pdh_cert, *plat_certs;
+    int ret;
+
+    if (!sev_guest(kvm))
+        return -ENOTTY;
+
+    if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+                sizeof(struct kvm_sev_send_start)))
+        return -EFAULT;
+
+    /* if session_len is zero, userspace wants to query the session

length */
+    if (!params.session_len)

Quoted 8 lines

+        return __sev_send_start_query_session_length(kvm, argp, +                &params);
+
+    /* some sanity checks */
+    if (!params.pdh_cert_uaddr || !params.pdh_cert_len || +        !params.session_uaddr || params.session_len > SEV_FW_BLOB_MAX_SIZE)
+        return -EINVAL;

What if params.plat_certs_uaddr or params.amd_certs_uaddr is NULL ?

arch/x86/kvm/svm.c static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp) · line 7208

Quoted 7 lines

arch/x86/kvm/svm.c +7 -0

+
+    /* allocate the memory to hold the session data blob */
+    session_data = kmalloc(params.session_len, GFP_KERNEL_ACCOUNT);
+    if (!session_data)
+        return -ENOMEM;
+
+    /* copy the certificate blobs from userspace */

You haven't added comments for plat_cert and amd_cert. Also, it's much more readable if you add block comments like,

        /*

         *  PDH cert

         */

Quoted 87 lines

+    pdh_cert = psp_copy_user_blob(params.pdh_cert_uaddr, +                params.pdh_cert_len);
+    if (IS_ERR(pdh_cert)) {
+        ret = PTR_ERR(pdh_cert);
+        goto e_free_session;
+    }

arch/x86/kvm/svm.c +62 -0

+
+    plat_certs = psp_copy_user_blob(params.plat_certs_uaddr,
+                params.plat_certs_len);
+    if (IS_ERR(plat_certs)) {
+        ret = PTR_ERR(plat_certs);
+        goto e_free_pdh;
+    }
+
+    amd_certs = psp_copy_user_blob(params.amd_certs_uaddr,
+                params.amd_certs_len);
+    if (IS_ERR(amd_certs)) {
+        ret = PTR_ERR(amd_certs);
+        goto e_free_plat_cert;
+    }
+
+    data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+    if (data == NULL) {
+        ret = -ENOMEM;
+        goto e_free_amd_cert;
+    }
+
+    /* populate the FW SEND_START field with system physical address */
+    data->pdh_cert_address = __psp_pa(pdh_cert);
+    data->pdh_cert_len = params.pdh_cert_len;
+    data->plat_certs_address = __psp_pa(plat_certs);
+    data->plat_certs_len = params.plat_certs_len;
+    data->amd_certs_address = __psp_pa(amd_certs);
+    data->amd_certs_len = params.amd_certs_len;
+    data->session_address = __psp_pa(session_data);
+    data->session_len = params.session_len;
+    data->handle = sev->handle;
+
+    ret = sev_issue_cmd(kvm, SEV_CMD_SEND_START, data, &argp->error);
+
+    if (ret)
+        goto e_free;
+
+    if (copy_to_user((void __user *)(uintptr_t) params.session_uaddr,
+            session_data, params.session_len)) {
+        ret = -EFAULT;
+        goto e_free;
+    }
+
+    params.policy = data->policy;
+    params.session_len = data->session_len;
+    if (copy_to_user((void __user *)(uintptr_t)argp->data, &params,
+                sizeof(struct kvm_sev_send_start)))
+        ret = -EFAULT;
+
+e_free:
+    kfree(data);
+e_free_amd_cert:
+    kfree(amd_certs);
+e_free_plat_cert:
+    kfree(plat_certs);
+e_free_pdh:
+    kfree(pdh_cert);
+e_free_session:
+    kfree(session_data);
+    return ret;
+}
+

  static int svm_mem_enc_op(struct kvm *kvm, void __user *argp) {
      struct kvm_sev_cmd sev_cmd;

arch/x86/kvm/svm.c +0 -0

@@ -7193,6 +7318,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void
 __user *argp)

      case KVM_SEV_LAUNCH_SECRET:
          r = sev_launch_secret(kvm, &sev_cmd);
          break;
+    case KVM_SEV_SEND_START:
+        r = sev_send_start(kvm, &sev_cmd);
+        break;
      default:
          r = -EINVAL;
          goto out;

include/linux/psp-sev.h +0 -0

index 5167bf2bfc75..9f63b9d48b63 100644
@@ -323,11 +323,11 @@ struct sev_data_send_start {

Quoted 7 lines

      u64 pdh_cert_address;            /* In */
      u32 pdh_cert_len;            /* In */
      u32 reserved1;
-    u64 plat_cert_address;            /* In */
-    u32 plat_cert_len;            /* In */
+    u64 plat_certs_address;            /* In */
+    u32 plat_certs_len;            /* In */

It seems that the 'platform certificate' and the 'amd_certificate' are single entities, meaning only copy is there for the particular platform and particular the AMD product. Why are these plural then ?

Quoted 30 lines

      u32 reserved2;
-    u64 amd_cert_address;            /* In */
-    u32 amd_cert_len;            /* In */
+    u64 amd_certs_address;            /* In */
+    u32 amd_certs_len;            /* In */
      u32 reserved3;
      u64 session_address;            /* In */
      u32 session_len;            /* In/Out */

include/uapi/linux/kvm.h +11 -0

index 4b95f9a31a2f..17bef4c245e1 100644
@@ -1558,6 +1558,18 @@ struct kvm_sev_dbg {
       __u32 len;
   };
   +struct kvm_sev_send_start {
+    __u32 policy;
+    __u64 pdh_cert_uaddr;
+    __u32 pdh_cert_len;
+    __u64 plat_certs_uaddr;
+    __u32 plat_certs_len;
+    __u64 amd_certs_uaddr;
+    __u32 amd_certs_len;
+    __u64 session_uaddr;
+    __u32 session_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)

From: Brijesh Singh <[email protected]>

The command is used for encrypting the guest memory region using the encryption context created with KVM_SEV_SEND_START.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../virt/kvm/amd-memory-encryption.rst        |  24 ++++
 arch/x86/kvm/svm.c                            | 136 +++++++++++++++++-
 include/uapi/linux/kvm.h                      |   9 ++
 3 files changed, 165 insertions(+), 4 deletions(-)

Documentation/virt/kvm/amd-memory-encryption.rst +24 -0

index 4fd34fc5c7a7..f46817ef7019 100644
@@ -290,6 +290,30 @@ Returns: 0 on success, -negative on error
                 __u32 session_len;
         };
 
+11. KVM_SEV_SEND_UPDATE_DATA
+----------------------------
+
+The KVM_SEV_SEND_UPDATE_DATA command can be used by the hypervisor to encrypt the
+outgoing guest memory region with the encryption context creating using
+KVM_SEV_SEND_START.
+
+Parameters (in): struct kvm_sev_send_update_data
+
+Returns: 0 on success, -negative on error
+
+::
+
+        struct kvm_sev_launch_send_update_data {
+                __u64 hdr_uaddr;        /* userspace address containing the packet header */
+                __u32 hdr_len;
+
+                __u64 guest_uaddr;      /* the source memory region to be encrypted */
+                __u32 guest_len;
+
+                __u64 trans_uaddr;      /* the destition memory region  */
+                __u32 trans_len;
+        };
+
 References
 ==========
 

arch/x86/kvm/svm.c +101 -4

index 63d172e974ad..8561c47cc4f9 100644
@@ -428,6 +428,7 @@ static DECLARE_RWSEM(sev_deactivate_lock);
 static DEFINE_MUTEX(sev_bitmap_lock);
 static unsigned int max_sev_asid;
 static unsigned int min_sev_asid;
+static unsigned long sev_me_mask;
 static unsigned long *sev_asid_bitmap;
 static unsigned long *sev_reclaim_asid_bitmap;
 #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)

@@ -1232,16 +1233,22 @@ static int avic_ga_log_notifier(u32 ga_tag)
 static __init int sev_hardware_setup(void)
 {
 	struct sev_user_data_status *status;
+	u32 eax, ebx;
 	int rc;
 
-	/* Maximum number of encrypted guests supported simultaneously */
-	max_sev_asid = cpuid_ecx(0x8000001F);
+	/*
+	 * Query the memory encryption information.
+	 *  EBX:  Bit 0:5 Pagetable bit position used to indicate encryption
+	 *  (aka Cbit).
+	 *  ECX:  Maximum number of encrypted guests supported simultaneously.
+	 *  EDX:  Minimum ASID value that should be used for SEV guest.
+	 */
+	cpuid(0x8000001f, &eax, &ebx, &max_sev_asid, &min_sev_asid);
 
 	if (!max_sev_asid)
 		return 1;
 
-	/* Minimum ASID value that should be used for SEV guest */
-	min_sev_asid = cpuid_edx(0x8000001F);
+	sev_me_mask = 1UL << (ebx & 0x3f);
 
 	/* Initialize SEV ASID bitmaps */
 	sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);

@@ -7274,6 +7281,124 @@ static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+/* Userspace wants to query either header or trans length. */
+static int
+__sev_send_update_data_query_lengths(struct kvm *kvm, struct kvm_sev_cmd *argp,
+				     struct kvm_sev_send_update_data *params)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_update_data *data;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (!data)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
+
+	params->hdr_len = data->hdr_len;
+	params->trans_len = data->trans_len;
+
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
+			 sizeof(struct kvm_sev_send_update_data)))
+		ret = -EFAULT;
+
+	kfree(data);
+	return ret;
+}
+
+static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_update_data *data;
+	struct kvm_sev_send_update_data params;
+	void *hdr, *trans_data;
+	struct page **guest_page;
+	unsigned long n;
+	int ret, offset;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+			sizeof(struct kvm_sev_send_update_data)))
+		return -EFAULT;
+
+	/* userspace wants to query either header or trans length */
+	if (!params.trans_len || !params.hdr_len)
+		return __sev_send_update_data_query_lengths(kvm, argp, &params);
+
+	if (!params.trans_uaddr || !params.guest_uaddr ||
+	    !params.guest_len || !params.hdr_uaddr)
+		return -EINVAL;
+
+
+	/* Check if we are crossing the page boundary */
+	offset = params.guest_uaddr & (PAGE_SIZE - 1);
+	if ((params.guest_len + offset > PAGE_SIZE))
+		return -EINVAL;
+
+	/* Pin guest memory */
+	guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK,
+				    PAGE_SIZE, &n, 0);
+	if (!guest_page)
+		return -EFAULT;
+
+	/* allocate memory for header and transport buffer */
+	ret = -ENOMEM;
+	hdr = kmalloc(params.hdr_len, GFP_KERNEL_ACCOUNT);
+	if (!hdr)
+		goto e_unpin;
+
+	trans_data = kmalloc(params.trans_len, GFP_KERNEL_ACCOUNT);
+	if (!trans_data)
+		goto e_free_hdr;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		goto e_free_trans_data;
+
+	data->hdr_address = __psp_pa(hdr);
+	data->hdr_len = params.hdr_len;
+	data->trans_address = __psp_pa(trans_data);
+	data->trans_len = params.trans_len;
+
+	/* The SEND_UPDATE_DATA command requires C-bit to be always set. */
+	data->guest_address = (page_to_pfn(guest_page[0]) << PAGE_SHIFT) +
+				offset;
+	data->guest_address |= sev_me_mask;
+	data->guest_len = params.guest_len;
+	data->handle = sev->handle;
+

arch/x86/kvm/svm.c +21 -0

+	}
+
+	/* Copy packet header to userspace. */
+	ret = copy_to_user((void __user *)(uintptr_t)params.hdr_uaddr, hdr,
+				params.hdr_len);
+
+e_free:
+	kfree(data);
+e_free_trans_data:
+	kfree(trans_data);
+e_free_hdr:
+	kfree(hdr);
+e_unpin:
+	sev_unpin_memory(kvm, guest_page, n);
+
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;

@@ -7321,6 +7446,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_SEND_START:
 		r = sev_send_start(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_UPDATE_DATA:
+		r = sev_send_update_data(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;

include/uapi/linux/kvm.h +9 -0

index 17bef4c245e1..d9dc81bb9c55 100644
@@ -1570,6 +1570,15 @@ struct kvm_sev_send_start {
 	__u32 session_len;
 };
 
+struct kvm_sev_send_update_data {
+	__u64 hdr_uaddr;
+	__u32 hdr_len;
+	__u64 guest_uaddr;
+	__u32 guest_len;
+	__u64 trans_uaddr;
+	__u32 trans_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)

Signature · 1 line

2.17.1

From: Brijesh Singh <[email protected]>

The command is used for encrypting the guest memory region using the encryption context created with KVM_SEV_SEND_START.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
  .../virt/kvm/amd-memory-encryption.rst        |  24 ++++
  arch/x86/kvm/svm.c                            | 136 +++++++++++++++++-
  include/uapi/linux/kvm.h                      |   9 ++
  3 files changed, 165 insertions(+), 4 deletions(-)

Documentation/virt/kvm/amd-memory-encryption.rst +24 -0

index 4fd34fc5c7a7..f46817ef7019 100644
@@ -290,6 +290,30 @@ Returns: 0 on success, -negative on error
                  __u32 session_len;
          };
 
+11. KVM_SEV_SEND_UPDATE_DATA
+----------------------------
+
+The KVM_SEV_SEND_UPDATE_DATA command can be used by the hypervisor to encrypt the
+outgoing guest memory region with the encryption context creating using
+KVM_SEV_SEND_START.
+
+Parameters (in): struct kvm_sev_send_update_data
+
+Returns: 0 on success, -negative on error
+
+::
+
+        struct kvm_sev_launch_send_update_data {
+                __u64 hdr_uaddr;        /* userspace address containing the packet header */
+                __u32 hdr_len;
+
+                __u64 guest_uaddr;      /* the source memory region to be encrypted */
+                __u32 guest_len;
+
+                __u64 trans_uaddr;      /* the destition memory region  */
+                __u32 trans_len;
+        };
+
  References
  ==========
 

arch/x86/kvm/svm.c +2 -0

index 63d172e974ad..8561c47cc4f9 100644
@@ -428,6 +428,7 @@ static DECLARE_RWSEM(sev_deactivate_lock);
  static DEFINE_MUTEX(sev_bitmap_lock);
  static unsigned int max_sev_asid;
  static unsigned int min_sev_asid;
+static unsigned long sev_me_mask;
  static unsigned long *sev_asid_bitmap;
  static unsigned long *sev_reclaim_asid_bitmap;
  #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)

@@ -1232,16 +1233,22 @@ static int avic_ga_log_notifier(u32 ga_tag)
  static __init int sev_hardware_setup(void)
  {
  	struct sev_user_data_status *status;
+	u32 eax, ebx;
  	int rc;
 

arch/x86/kvm/svm.c +30 -2

  	if (!max_sev_asid)
  		return 1;
 
-	/* Minimum ASID value that should be used for SEV guest */
-	min_sev_asid = cpuid_edx(0x8000001F);
+	sev_me_mask = 1UL << (ebx & 0x3f);
 
  	/* Initialize SEV ASID bitmaps */
  	sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);

@@ -7274,6 +7281,124 @@ static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
  	return ret;
  }
 
+/* Userspace wants to query either header or trans length. */
+static int
+__sev_send_update_data_query_lengths(struct kvm *kvm, struct kvm_sev_cmd *argp,
+				     struct kvm_sev_send_update_data *params)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_update_data *data;
+	int ret;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
+	if (!data)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
+
+	params->hdr_len = data->hdr_len;
+	params->trans_len = data->trans_len;
+
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
+			 sizeof(struct kvm_sev_send_update_data)))
+		ret = -EFAULT;
+
+	kfree(data);
+	return ret;
+}
+
+static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{

arch/x86/kvm/svm.c +38 -0

+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+			sizeof(struct kvm_sev_send_update_data)))
+		return -EFAULT;
+
+	/* userspace wants to query either header or trans length */
+	if (!params.trans_len || !params.hdr_len)
+		return __sev_send_update_data_query_lengths(kvm, argp, &params);
+
+	if (!params.trans_uaddr || !params.guest_uaddr ||
+	    !params.guest_len || !params.hdr_uaddr)
+		return -EINVAL;
+
+
+	/* Check if we are crossing the page boundary */
+	offset = params.guest_uaddr & (PAGE_SIZE - 1);
+	if ((params.guest_len + offset > PAGE_SIZE))
+		return -EINVAL;
+
+	/* Pin guest memory */
+	guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK,
+				    PAGE_SIZE, &n, 0);
+	if (!guest_page)
+		return -EFAULT;
+
+	/* allocate memory for header and transport buffer */
+	ret = -ENOMEM;
+	hdr = kmalloc(params.hdr_len, GFP_KERNEL_ACCOUNT);
+	if (!hdr)
+		goto e_unpin;
+
+	trans_data = kmalloc(params.trans_len, GFP_KERNEL_ACCOUNT);
+	if (!trans_data)
+		goto e_free_hdr;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		goto e_free_trans_data;

arch/x86/kvm/svm.c +34 -0

+	data->guest_len = params.guest_len;
+	data->handle = sev->handle;
+
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
+
+	if (ret)
+		goto e_free;
+
+	/* copy transport buffer to user space */
+	if (copy_to_user((void __user *)(uintptr_t)params.trans_uaddr,
+			 trans_data, params.trans_len)) {
+		ret = -EFAULT;
+		goto e_unpin;
+	}
+
+	/* Copy packet header to userspace. */
+	ret = copy_to_user((void __user *)(uintptr_t)params.hdr_uaddr, hdr,
+				params.hdr_len);
+
+e_free:
+	kfree(data);
+e_free_trans_data:
+	kfree(trans_data);
+e_free_hdr:
+	kfree(hdr);
+e_unpin:
+	sev_unpin_memory(kvm, guest_page, n);
+
+	return ret;
+}
+
  static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
  {
  	struct kvm_sev_cmd sev_cmd;

@@ -7321,6 +7446,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
  	case KVM_SEV_SEND_START:
  		r = sev_send_start(kvm, &sev_cmd);
  		break;
+	case KVM_SEV_SEND_UPDATE_DATA:
+		r = sev_send_update_data(kvm, &sev_cmd);
+		break;
  	default:
  		r = -EINVAL;
  		goto out;

include/uapi/linux/kvm.h +2 -0

index 17bef4c245e1..d9dc81bb9c55 100644
@@ -1570,6 +1570,15 @@ struct kvm_sev_send_start {
  	__u32 session_len;
  };
 
+struct kvm_sev_send_update_data {
+	__u64 hdr_uaddr;

From: Brijesh Singh <[email protected]>

The command is used to finailize the encryption context created with KVM_SEV_SEND_START command.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../virt/kvm/amd-memory-encryption.rst        |  8 +++++++
 arch/x86/kvm/svm.c                            | 23 +++++++++++++++++++
 2 files changed, 31 insertions(+)

Documentation/virt/kvm/amd-memory-encryption.rst +0 -0

index f46817ef7019..a45dcb5f8687 100644

Documentation/virt/kvm/amd-memory-encryption.rst +1 -0

+
 References
 ==========
 

arch/x86/kvm/svm.c +23 -0

index 8561c47cc4f9..71a4cb3b817d 100644
@@ -7399,6 +7399,26 @@ static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_send_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_finish *data;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_FINISH, data, &argp->error);
+
+	kfree(data);
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;

@@ -7449,6 +7469,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_SEND_UPDATE_DATA:
 		r = sev_send_update_data(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_SEND_FINISH:
+		r = sev_send_finish(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;

Signature · 1 line

2.17.1

From: Brijesh Singh <[email protected]>

The command is used to finailize the encryption context created with KVM_SEV_SEND_START command.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
  .../virt/kvm/amd-memory-encryption.rst        |  8 +++++++
  arch/x86/kvm/svm.c                            | 23 +++++++++++++++++++
  2 files changed, 31 insertions(+)

Documentation/virt/kvm/amd-memory-encryption.rst +8 -0

index f46817ef7019..a45dcb5f8687 100644
@@ -314,6 +314,14 @@ Returns: 0 on success, -negative on error
                  __u32 trans_len;
          };
 
+12. KVM_SEV_SEND_FINISH
+------------------------
+
+After completion of the migration flow, the KVM_SEV_SEND_FINISH command can be
+issued by the hypervisor to delete the encryption context.
+
+Returns: 0 on success, -negative on error
+
  References
  ==========
 

arch/x86/kvm/svm.c +20 -0

index 8561c47cc4f9..71a4cb3b817d 100644
@@ -7399,6 +7399,26 @@ static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
  	return ret;
  }
 
+static int sev_send_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_send_finish *data;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_SEND_FINISH, data, &argp->error);
+
+	kfree(data);
+	return ret;
+}
+
  static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
  {
  	struct kvm_sev_cmd sev_cmd;

From: Brijesh Singh <[email protected]>

The command is used to create the encryption context for an incoming SEV guest. The encryption context can be later used by the hypervisor to import the incoming data into the SEV guest memory space.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Tom Lendacky <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

Reviewed-by Steve Rutherford <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 .../virt/kvm/amd-memory-encryption.rst        | 29 +++++++
 arch/x86/kvm/svm.c                            | 81 +++++++++++++++++++
 include/uapi/linux/kvm.h                      |  9 +++
 3 files changed, 119 insertions(+)

Documentation/virt/kvm/amd-memory-encryption.rst +10 -0

index a45dcb5f8687..ef1f1f3a5b40 100644
@@ -322,6 +322,35 @@ issued by the hypervisor to delete the encryption context.
 
 Returns: 0 on success, -negative on error
 
+13. KVM_SEV_RECEIVE_START
+------------------------
+
+The KVM_SEV_RECEIVE_START command is used for creating the memory encryption
+context for an incoming SEV guest. To create the encryption context, the user must
+provide a guest policy, the platform public Diffie-Hellman (PDH) key and session
+information.
+
+Parameters: struct  kvm_sev_receive_start (in/out)
+

Documentation/virt/kvm/amd-memory-encryption.rst +7 -0

+                __u32 session_len;
+        };
+
+On success, the 'handle' field contains a new handle and on error, a negative value.
+
+For more details, see SEV spec Section 6.12.
+
 References
 ==========
 

arch/x86/kvm/svm.c +81 -0

index 71a4cb3b817d..038b47685733 100644
@@ -7419,6 +7419,84 @@ static int sev_send_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	return ret;
 }
 
+static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+	struct sev_data_receive_start *start;
+	struct kvm_sev_receive_start params;
+	int *error = &argp->error;
+	void *session_data;
+	void *pdh_data;
+	int ret;
+
+	if (!sev_guest(kvm))
+		return -ENOTTY;
+
+	/* Get parameter from the userspace */
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
+			sizeof(struct kvm_sev_receive_start)))
+		return -EFAULT;
+
+	/* some sanity checks */
+	if (!params.pdh_uaddr || !params.pdh_len ||
+	    !params.session_uaddr || !params.session_len)
+		return -EINVAL;
+
+	pdh_data = psp_copy_user_blob(params.pdh_uaddr, params.pdh_len);
+	if (IS_ERR(pdh_data))
+		return PTR_ERR(pdh_data);
+
+	session_data = psp_copy_user_blob(params.session_uaddr,
+			params.session_len);
+	if (IS_ERR(session_data)) {
+		ret = PTR_ERR(session_data);
+		goto e_free_pdh;
+	}
+
+	ret = -ENOMEM;
+	start = kzalloc(sizeof(*start), GFP_KERNEL);
+	if (!start)
+		goto e_free_session;
+
+	start->handle = params.handle;
+	start->policy = params.policy;
+	start->pdh_cert_address = __psp_pa(pdh_data);
+	start->pdh_cert_len = params.pdh_len;
+	start->session_address = __psp_pa(session_data);
+	start->session_len = params.session_len;
+
+	/* create memory encryption context */
+	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_RECEIVE_START, start,
+				error);
+	if (ret)
+		goto e_free;
+
+	/* Bind ASID to this guest */
+	ret = sev_bind_asid(kvm, start->handle, error);
+	if (ret)
+		goto e_free;
+
+	params.handle = start->handle;
+	if (copy_to_user((void __user *)(uintptr_t)argp->data,
+			 &params, sizeof(struct kvm_sev_receive_start))) {
+		ret = -EFAULT;
+		sev_unbind_asid(kvm, start->handle);
+		goto e_free;
+	}
+
+	sev->handle = start->handle;
+	sev->fd = argp->sev_fd;
+
+e_free:
+	kfree(start);
+e_free_session:
+	kfree(session_data);
+e_free_pdh:
+	kfree(pdh_data);
+
+	return ret;
+}
+
 static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 {
 	struct kvm_sev_cmd sev_cmd;

@@ -7472,6 +7550,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
 	case KVM_SEV_SEND_FINISH:
 		r = sev_send_finish(kvm, &sev_cmd);
 		break;
+	case KVM_SEV_RECEIVE_START:
+		r = sev_receive_start(kvm, &sev_cmd);
+		break;
 	default:
 		r = -EINVAL;
 		goto out;

include/uapi/linux/kvm.h +9 -0

index d9dc81bb9c55..74764b9db5fa 100644
@@ -1579,6 +1579,15 @@ struct kvm_sev_send_update_data {
 	__u32 trans_len;
 };
 
+struct kvm_sev_receive_start {
+	__u32 handle;
+	__u32 policy;
+	__u64 pdh_uaddr;
+	__u32 pdh_len;
+	__u64 session_uaddr;
+	__u32 session_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)

Signature · 1 line

2.17.1