[PATCH v2 0/8] ccp: KVM: SVM: Use stack for SEV command buffers

[PATCH v2 0/8] ccp: KVM: SVM: Use stack for SEV command buffers

[Sean Christopherson]

This series teaches __sev_do_cmd_locked() to gracefully handle vmalloc'd
command buffers by copying _all_ incoming data pointers to an internal
buffer before sending the command to the PSP.  The SEV driver and KVM are
then converted to use the stack for all command buffers.

Tested everything except sev_ioctl_do_pek_import(), I don't know anywhere
near enough about the PSP to give it the right input.

v2:
  - Rebase to kvm/queue, commit f96be2deac9b ("KVM: x86: Support KVM VMs
    sharing SEV context").
  - Unconditionally copy @data to the internal buffer. [Christophe, Brijesh]
  - Allocate a full page for the buffer. [Brijesh]
  - Drop one set of the "!"s. [Christophe]
  - Use virt_addr_valid() instead of is_vmalloc_addr() for the temporary
    patch (definitely feel free to drop the patch if it's not worth
    backporting). [Christophe]
  - s/intput/input/. [Tom]
  - Add a patch to free "sev" if init fails.  This is not strictly
    necessary (I think; I suck horribly when it comes to the driver
    framework).   But it felt wrong to not free cmd_buf on failure, and
    even more wrong to free cmd_buf but not sev.

v1:
  - https://lkml.kernel.org/r/20210402233702.3291792-1-seanjc@google.com

Sean Christopherson (8):
  crypto: ccp: Free SEV device if SEV init fails
  crypto: ccp: Detect and reject "invalid" addresses destined for PSP
  crypto: ccp: Reject SEV commands with mismatching command buffer
  crypto: ccp: Play nice with vmalloc'd memory for SEV command structs
  crypto: ccp: Use the stack for small SEV command buffers
  crypto: ccp: Use the stack and common buffer for status commands
  crypto: ccp: Use the stack and common buffer for INIT command
  KVM: SVM: Allocate SEV command structures on local stack

 arch/x86/kvm/svm/sev.c       | 262 +++++++++++++----------------------
 drivers/crypto/ccp/sev-dev.c | 197 +++++++++++++-------------
 drivers/crypto/ccp/sev-dev.h |   4 +-
 3 files changed, 196 insertions(+), 267 deletions(-)

-- 
2.31.0.208.g409f899ff0-goog

[PATCH v2 1/8] crypto: ccp: Free SEV device if SEV init fails

[Sean Christopherson]

Free the SEV device if later initialization fails.  The memory isn't
technically leaked as it's tracked in the top-level device's devres
list, but unless the top-level device is removed, the memory won't be
freed and is effectively leaked.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index cb9b4c4e371e..ba240d33d26e 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -987,7 +987,7 @@ int sev_dev_init(struct psp_device *psp)
 	if (!sev->vdata) {
 		ret = -ENODEV;
 		dev_err(dev, "sev: missing driver data\n");
-		goto e_err;
+		goto e_sev;
 	}
 
 	psp_set_sev_irq_handler(psp, sev_irq_handler, sev);
@@ -1002,6 +1002,8 @@ int sev_dev_init(struct psp_device *psp)
 
 e_irq:
 	psp_clear_sev_irq_handler(psp);
+e_sev:
+	devm_kfree(dev, sev);
 e_err:
 	psp->sev_data = NULL;
 
-- 
2.31.0.208.g409f899ff0-goog

[PATCH v2 2/8] crypto: ccp: Detect and reject "invalid" addresses destined for PSP

[Sean Christopherson]

Explicitly reject using pointers that are not virt_to_phys() friendly
as the source for SEV commands that are sent to the PSP.  The PSP works
with physical addresses, and __pa()/virt_to_phys() will not return the
correct address in these cases, e.g. for a vmalloc'd pointer.  At best,
the bogus address will cause the command to fail, and at worst lead to
system instability.

While it's unlikely that callers will deliberately use a bad pointer for
SEV buffers, a caller can easily use a vmalloc'd pointer unknowingly when
running with CONFIG_VMAP_STACK=y as it's not obvious that putting the
command buffers on the stack would be bad.  The command buffers are
relative  small and easily fit on the stack, and the APIs to do not
document that the incoming pointer must be a physically contiguous,
__pa() friendly pointer.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Fixes: 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index ba240d33d26e..3e0d1d6922ba 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -150,6 +150,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
 	sev = psp->sev_data;
 
+	if (data && WARN_ON_ONCE(!virt_addr_valid(data)))
+		return -EINVAL;
+
 	/* Get the physical address of the command buffer */
 	phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
 	phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
-- 
2.31.0.208.g409f899ff0-goog

[PATCH v2 3/8] crypto: ccp: Reject SEV commands with mismatching command buffer

[Sean Christopherson]

WARN on and reject SEV commands that provide a valid data pointer, but do
not have a known, non-zero length.  And conversely, reject commands that
take a command buffer but none is provided (data is null).

Aside from sanity checking input, disallowing a non-null pointer without
a non-zero size will allow a future patch to cleanly handle vmalloc'd
data by copying the data to an internal __pa() friendly buffer.

Note, this also effectively prevents callers from using commands that
have a non-zero length and are not known to the kernel.  This is not an
explicit goal, but arguably the side effect is a good thing from the
kernel's perspective.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 3e0d1d6922ba..47a372e07223 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -141,6 +141,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 	struct sev_device *sev;
 	unsigned int phys_lsb, phys_msb;
 	unsigned int reg, ret = 0;
+	int buf_len;
 
 	if (!psp || !psp->sev_data)
 		return -ENODEV;
@@ -150,6 +151,10 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
 	sev = psp->sev_data;
 
+	buf_len = sev_cmd_buffer_len(cmd);
+	if (WARN_ON_ONCE(!data != !buf_len))
+		return -EINVAL;
+
 	if (data && WARN_ON_ONCE(!virt_addr_valid(data)))
 		return -EINVAL;
 
@@ -161,7 +166,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 		cmd, phys_msb, phys_lsb, psp_timeout);
 
 	print_hex_dump_debug("(in):  ", DUMP_PREFIX_OFFSET, 16, 2, data,
-			     sev_cmd_buffer_len(cmd), false);
+			     buf_len, false);
 
 	iowrite32(phys_lsb, sev->io_regs + sev->vdata->cmdbuff_addr_lo_reg);
 	iowrite32(phys_msb, sev->io_regs + sev->vdata->cmdbuff_addr_hi_reg);
@@ -197,7 +202,7 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 	}
 
 	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
-			     sev_cmd_buffer_len(cmd), false);
+			     buf_len, false);
 
 	return ret;
 }
-- 
2.31.0.208.g409f899ff0-goog

[PATCH v2 4/8] crypto: ccp: Play nice with vmalloc'd memory for SEV command structs

[Sean Christopherson]

Copy the incoming @data comman to an internal buffer so that callers can
put SEV command buffers on the stack without running afoul of
CONFIG_VMAP_STACK=y, i.e. without bombing on vmalloc'd pointers.  As of
today, the largest supported command takes a 68 byte buffer, i.e. pretty
much every command can be put on the stack.  Because sev_cmd_mutex is
held for the entirety of a transaction, only a single bounce buffer is
required.

Use the internal buffer unconditionally, as the majority of in-kernel
users will soon switch to using the stack.  At that point, checking
virt_addr_valid() becomes (negligible) overhead in most cases, and
supporting both paths slightly increases complexity.  Since the commands
are all quite small, the cost of the copies is insignificant compared to
the latency of communicating with the PSP.

Allocate a full page for the buffer as opportunistic preparation for
SEV-SNP, which requires the command buffer to be in firmware state for
commands that trigger memory writes from the PSP firmware.  Using a full
page now will allow SEV-SNP support to simply transition the page as
needed.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 28 +++++++++++++++++++++++-----
 drivers/crypto/ccp/sev-dev.h |  2 ++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 47a372e07223..4aedbdaffe90 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -155,12 +155,17 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 	if (WARN_ON_ONCE(!data != !buf_len))
 		return -EINVAL;
 
-	if (data && WARN_ON_ONCE(!virt_addr_valid(data)))
-		return -EINVAL;
+	/*
+	 * Copy the incoming data to driver's scratch buffer as __pa() will not
+	 * work for some memory, e.g. vmalloc'd addresses, and @data may not be
+	 * physically contiguous.
+	 */
+	if (data)
+		memcpy(sev->cmd_buf, data, buf_len);
 
 	/* Get the physical address of the command buffer */
-	phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
-	phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
+	phys_lsb = data ? lower_32_bits(__psp_pa(sev->cmd_buf)) : 0;
+	phys_msb = data ? upper_32_bits(__psp_pa(sev->cmd_buf)) : 0;
 
 	dev_dbg(sev->dev, "sev command id %#x buffer 0x%08x%08x timeout %us\n",
 		cmd, phys_msb, phys_lsb, psp_timeout);
@@ -204,6 +209,13 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
 			     buf_len, false);
 
+	/*
+	 * Copy potential output from the PSP back to data.  Do this even on
+	 * failure in case the caller wants to glean something from the error.
+	 */
+	if (data)
+		memcpy(data, sev->cmd_buf, buf_len);
+
 	return ret;
 }
 
@@ -984,6 +996,10 @@ int sev_dev_init(struct psp_device *psp)
 	if (!sev)
 		goto e_err;
 
+	sev->cmd_buf = (void *)devm_get_free_pages(dev, GFP_KERNEL, 0);
+	if (!sev->cmd_buf)
+		goto e_sev;
+
 	psp->sev_data = sev;
 
 	sev->dev = dev;
@@ -995,7 +1011,7 @@ int sev_dev_init(struct psp_device *psp)
 	if (!sev->vdata) {
 		ret = -ENODEV;
 		dev_err(dev, "sev: missing driver data\n");
-		goto e_sev;
+		goto e_buf;
 	}
 
 	psp_set_sev_irq_handler(psp, sev_irq_handler, sev);
@@ -1010,6 +1026,8 @@ int sev_dev_init(struct psp_device *psp)
 
 e_irq:
 	psp_clear_sev_irq_handler(psp);
+e_buf:
+	devm_free_pages(dev, (unsigned long)sev->cmd_buf);
 e_sev:
 	devm_kfree(dev, sev);
 e_err:
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index dd5c4fe82914..e1572f408577 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -52,6 +52,8 @@ struct sev_device {
 	u8 api_major;
 	u8 api_minor;
 	u8 build;
+
+	void *cmd_buf;
 };
 
 int sev_dev_init(struct psp_device *psp);
-- 
2.31.0.208.g409f899ff0-goog

[PATCH v2 5/8] crypto: ccp: Use the stack for small SEV command buffers

[Sean Christopherson]

For commands with small input/output buffers, use the local stack to
"allocate" the structures used to communicate with the PSP.   Now that
__sev_do_cmd_locked() gracefully handles vmalloc'd buffers, there's no
reason to avoid using the stack, e.g. CONFIG_VMAP_STACK=y will just work.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 122 ++++++++++++++---------------------
 1 file changed, 47 insertions(+), 75 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 4aedbdaffe90..bb0d6de071e6 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -396,7 +396,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_pek_csr input;
-	struct sev_data_pek_csr *data;
+	struct sev_data_pek_csr data;
 	void __user *input_address;
 	void *blob = NULL;
 	int ret;
@@ -407,29 +407,24 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
 	/* userspace wants to query CSR length */
-	if (!input.address || !input.length)
+	if (!input.address || !input.length) {
+		data.address = 0;
+		data.len = 0;
 		goto cmd;
+	}
 
 	/* allocate a physically contiguous buffer to store the CSR blob */
 	input_address = (void __user *)input.address;
-	if (input.length > SEV_FW_BLOB_MAX_SIZE) {
-		ret = -EFAULT;
-		goto e_free;
-	}
+	if (input.length > SEV_FW_BLOB_MAX_SIZE)
+		return -EFAULT;
 
 	blob = kmalloc(input.length, GFP_KERNEL);
-	if (!blob) {
-		ret = -ENOMEM;
-		goto e_free;
-	}
+	if (!blob)
+		return -ENOMEM;
 
-	data->address = __psp_pa(blob);
-	data->len = input.length;
+	data.address = __psp_pa(blob);
+	data.len = input.length;
 
 cmd:
 	if (sev->state == SEV_STATE_UNINIT) {
@@ -438,10 +433,10 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 			goto e_free_blob;
 	}
 
-	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, data, &argp->error);
+	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
 
 	 /* If we query the CSR length, FW responded with expected data. */
-	input.length = data->len;
+	input.length = data.len;
 
 	if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
 		ret = -EFAULT;
@@ -455,8 +450,6 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 
 e_free_blob:
 	kfree(blob);
-e_free:
-	kfree(data);
 	return ret;
 }
 
@@ -588,7 +581,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_pek_cert_import input;
-	struct sev_data_pek_cert_import *data;
+	struct sev_data_pek_cert_import data;
 	void *pek_blob, *oca_blob;
 	int ret;
 
@@ -598,19 +591,14 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
 	/* copy PEK certificate blobs from userspace */
 	pek_blob = psp_copy_user_blob(input.pek_cert_address, input.pek_cert_len);
-	if (IS_ERR(pek_blob)) {
-		ret = PTR_ERR(pek_blob);
-		goto e_free;
-	}
+	if (IS_ERR(pek_blob))
+		return PTR_ERR(pek_blob);
 
-	data->pek_cert_address = __psp_pa(pek_blob);
-	data->pek_cert_len = input.pek_cert_len;
+	data.reserved = 0;
+	data.pek_cert_address = __psp_pa(pek_blob);
+	data.pek_cert_len = input.pek_cert_len;
 
 	/* copy PEK certificate blobs from userspace */
 	oca_blob = psp_copy_user_blob(input.oca_cert_address, input.oca_cert_len);
@@ -619,8 +607,8 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 		goto e_free_pek;
 	}
 
-	data->oca_cert_address = __psp_pa(oca_blob);
-	data->oca_cert_len = input.oca_cert_len;
+	data.oca_cert_address = __psp_pa(oca_blob);
+	data.oca_cert_len = input.oca_cert_len;
 
 	/* If platform is not in INIT state then transition it to INIT */
 	if (sev->state != SEV_STATE_INIT) {
@@ -629,21 +617,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 			goto e_free_oca;
 	}
 
-	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
+	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
 
 e_free_oca:
 	kfree(oca_blob);
 e_free_pek:
 	kfree(pek_blob);
-e_free:
-	kfree(data);
 	return ret;
 }
 
 static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 {
 	struct sev_user_data_get_id2 input;
-	struct sev_data_get_id *data;
+	struct sev_data_get_id data;
 	void __user *input_address;
 	void *id_blob = NULL;
 	int ret;
@@ -657,28 +643,25 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 
 	input_address = (void __user *)input.address;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
 	if (input.address && input.length) {
 		id_blob = kmalloc(input.length, GFP_KERNEL);
-		if (!id_blob) {
-			kfree(data);
+		if (!id_blob)
 			return -ENOMEM;
-		}
 
-		data->address = __psp_pa(id_blob);
-		data->len = input.length;
+		data.address = __psp_pa(id_blob);
+		data.len = input.length;
+	} else {
+		data.address = 0;
+		data.len = 0;
 	}
 
-	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, data, &argp->error);
+	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, &data, &argp->error);
 
 	/*
 	 * Firmware will return the length of the ID value (either the minimum
 	 * required length or the actual length written), return it to the user.
 	 */
-	input.length = data->len;
+	input.length = data.len;
 
 	if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
 		ret = -EFAULT;
@@ -686,7 +669,7 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 	}
 
 	if (id_blob) {
-		if (copy_to_user(input_address, id_blob, data->len)) {
+		if (copy_to_user(input_address, id_blob, data.len)) {
 			ret = -EFAULT;
 			goto e_free;
 		}
@@ -694,7 +677,6 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 
 e_free:
 	kfree(id_blob);
-	kfree(data);
 
 	return ret;
 }
@@ -744,7 +726,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_pdh_cert_export input;
 	void *pdh_blob = NULL, *cert_blob = NULL;
-	struct sev_data_pdh_cert_export *data;
+	struct sev_data_pdh_cert_export data;
 	void __user *input_cert_chain_address;
 	void __user *input_pdh_cert_address;
 	int ret;
@@ -762,9 +744,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
+	memset(&data, 0, sizeof(data));
 
 	/* Userspace wants to query the certificate length. */
 	if (!input.pdh_cert_address ||
@@ -776,25 +756,19 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	input_cert_chain_address = (void __user *)input.cert_chain_address;
 
 	/* Allocate a physically contiguous buffer to store the PDH blob. */
-	if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE) {
-		ret = -EFAULT;
-		goto e_free;
-	}
+	if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)
+		return -EFAULT;
 
 	/* Allocate a physically contiguous buffer to store the cert chain blob. */
-	if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE) {
-		ret = -EFAULT;
-		goto e_free;
-	}
+	if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE)
+		return -EFAULT;
 
 	pdh_blob = kmalloc(input.pdh_cert_len, GFP_KERNEL);
-	if (!pdh_blob) {
-		ret = -ENOMEM;
-		goto e_free;
-	}
+	if (!pdh_blob)
+		return -ENOMEM;
 
-	data->pdh_cert_address = __psp_pa(pdh_blob);
-	data->pdh_cert_len = input.pdh_cert_len;
+	data.pdh_cert_address = __psp_pa(pdh_blob);
+	data.pdh_cert_len = input.pdh_cert_len;
 
 	cert_blob = kmalloc(input.cert_chain_len, GFP_KERNEL);
 	if (!cert_blob) {
@@ -802,15 +776,15 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 		goto e_free_pdh;
 	}
 
-	data->cert_chain_address = __psp_pa(cert_blob);
-	data->cert_chain_len = input.cert_chain_len;
+	data.cert_chain_address = __psp_pa(cert_blob);
+	data.cert_chain_len = input.cert_chain_len;
 
 cmd:
-	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
+	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
 
 	/* If we query the length, FW responded with expected data. */
-	input.cert_chain_len = data->cert_chain_len;
-	input.pdh_cert_len = data->pdh_cert_len;
+	input.cert_chain_len = data.cert_chain_len;
+	input.pdh_cert_len = data.pdh_cert_len;
 
 	if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
 		ret = -EFAULT;
@@ -835,8 +809,6 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	kfree(cert_blob);
 e_free_pdh:
 	kfree(pdh_blob);
-e_free:
-	kfree(data);
 	return ret;
 }
 
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v2 5/8] crypto: ccp: Use the stack for small SEV command buffers

[Christophe Leroy]

Le 07/04/2021 à 00:49, Sean Christopherson a écrit :
> For commands with small input/output buffers, use the local stack to
> "allocate" the structures used to communicate with the PSP.   Now that
> __sev_do_cmd_locked() gracefully handles vmalloc'd buffers, there's no
> reason to avoid using the stack, e.g. CONFIG_VMAP_STACK=y will just work.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   drivers/crypto/ccp/sev-dev.c | 122 ++++++++++++++---------------------
>   1 file changed, 47 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 4aedbdaffe90..bb0d6de071e6 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -396,7 +396,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>   {
>   	struct sev_device *sev = psp_master->sev_data;
>   	struct sev_user_data_pek_csr input;
> -	struct sev_data_pek_csr *data;
> +	struct sev_data_pek_csr data;

struct sev_data_pek_csr data = {0, 0};

>   	void __user *input_address;
>   	void *blob = NULL;
>   	int ret;
> @@ -407,29 +407,24 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>   	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>   		return -EFAULT;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
>   	/* userspace wants to query CSR length */
> -	if (!input.address || !input.length)
> +	if (!input.address || !input.length) {
> +		data.address = 0;
> +		data.len = 0;

With the change proposed above, those two new lines are unneeded.

>   		goto cmd;
> +	}
>   
>   	/* allocate a physically contiguous buffer to store the CSR blob */
>   	input_address = (void __user *)input.address;
> -	if (input.length > SEV_FW_BLOB_MAX_SIZE) {
> -		ret = -EFAULT;
> -		goto e_free;
> -	}
> +	if (input.length > SEV_FW_BLOB_MAX_SIZE)
> +		return -EFAULT;
>   
>   	blob = kmalloc(input.length, GFP_KERNEL);
> -	if (!blob) {
> -		ret = -ENOMEM;
> -		goto e_free;
> -	}
> +	if (!blob)
> +		return -ENOMEM;
>   
> -	data->address = __psp_pa(blob);
> -	data->len = input.length;
> +	data.address = __psp_pa(blob);
> +	data.len = input.length;
>   
>   cmd:
>   	if (sev->state == SEV_STATE_UNINIT) {
> @@ -438,10 +433,10 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>   			goto e_free_blob;
>   	}
>   
> -	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, data, &argp->error);
> +	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
>   
>   	 /* If we query the CSR length, FW responded with expected data. */
> -	input.length = data->len;
> +	input.length = data.len;
>   
>   	if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
>   		ret = -EFAULT;
> @@ -455,8 +450,6 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>   
>   e_free_blob:
>   	kfree(blob);
> -e_free:
> -	kfree(data);
>   	return ret;
>   }
>   
> @@ -588,7 +581,7 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
>   {
>   	struct sev_device *sev = psp_master->sev_data;
>   	struct sev_user_data_pek_cert_import input;
> -	struct sev_data_pek_cert_import *data;
> +	struct sev_data_pek_cert_import data;
>   	void *pek_blob, *oca_blob;
>   	int ret;
>   
> @@ -598,19 +591,14 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
>   	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>   		return -EFAULT;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
>   	/* copy PEK certificate blobs from userspace */
>   	pek_blob = psp_copy_user_blob(input.pek_cert_address, input.pek_cert_len);
> -	if (IS_ERR(pek_blob)) {
> -		ret = PTR_ERR(pek_blob);
> -		goto e_free;
> -	}
> +	if (IS_ERR(pek_blob))
> +		return PTR_ERR(pek_blob);
>   
> -	data->pek_cert_address = __psp_pa(pek_blob);
> -	data->pek_cert_len = input.pek_cert_len;
> +	data.reserved = 0;
> +	data.pek_cert_address = __psp_pa(pek_blob);
> +	data.pek_cert_len = input.pek_cert_len;
>   
>   	/* copy PEK certificate blobs from userspace */
>   	oca_blob = psp_copy_user_blob(input.oca_cert_address, input.oca_cert_len);
> @@ -619,8 +607,8 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
>   		goto e_free_pek;
>   	}
>   
> -	data->oca_cert_address = __psp_pa(oca_blob);
> -	data->oca_cert_len = input.oca_cert_len;
> +	data.oca_cert_address = __psp_pa(oca_blob);
> +	data.oca_cert_len = input.oca_cert_len;
>   
>   	/* If platform is not in INIT state then transition it to INIT */
>   	if (sev->state != SEV_STATE_INIT) {
> @@ -629,21 +617,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
>   			goto e_free_oca;
>   	}
>   
> -	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
> +	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
>   
>   e_free_oca:
>   	kfree(oca_blob);
>   e_free_pek:
>   	kfree(pek_blob);
> -e_free:
> -	kfree(data);
>   	return ret;
>   }
>   
>   static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
>   {
>   	struct sev_user_data_get_id2 input;
> -	struct sev_data_get_id *data;
> +	struct sev_data_get_id data;
>   	void __user *input_address;
>   	void *id_blob = NULL;
>   	int ret;
> @@ -657,28 +643,25 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
>   
>   	input_address = (void __user *)input.address;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -
>   	if (input.address && input.length) {
>   		id_blob = kmalloc(input.length, GFP_KERNEL);
> -		if (!id_blob) {
> -			kfree(data);
> +		if (!id_blob)
>   			return -ENOMEM;
> -		}
>   
> -		data->address = __psp_pa(id_blob);
> -		data->len = input.length;
> +		data.address = __psp_pa(id_blob);
> +		data.len = input.length;
> +	} else {
> +		data.address = 0;
> +		data.len = 0;
>   	}
>   
> -	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, data, &argp->error);
> +	ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, &data, &argp->error);
>   
>   	/*
>   	 * Firmware will return the length of the ID value (either the minimum
>   	 * required length or the actual length written), return it to the user.
>   	 */
> -	input.length = data->len;
> +	input.length = data.len;
>   
>   	if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
>   		ret = -EFAULT;
> @@ -686,7 +669,7 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
>   	}
>   
>   	if (id_blob) {
> -		if (copy_to_user(input_address, id_blob, data->len)) {
> +		if (copy_to_user(input_address, id_blob, data.len)) {
>   			ret = -EFAULT;
>   			goto e_free;
>   		}
> @@ -694,7 +677,6 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
>   
>   e_free:
>   	kfree(id_blob);
> -	kfree(data);
>   
>   	return ret;
>   }
> @@ -744,7 +726,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>   	struct sev_device *sev = psp_master->sev_data;
>   	struct sev_user_data_pdh_cert_export input;
>   	void *pdh_blob = NULL, *cert_blob = NULL;
> -	struct sev_data_pdh_cert_export *data;
> +	struct sev_data_pdh_cert_export data;

struct sev_data_pdh_cert_export data = {0, 0, 0, 0, 0};

>   	void __user *input_cert_chain_address;
>   	void __user *input_pdh_cert_address;
>   	int ret;
> @@ -762,9 +744,7 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>   	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>   		return -EFAULT;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> +	memset(&data, 0, sizeof(data));

You can avoid that memset by initing at declaration, see above.

>   
>   	/* Userspace wants to query the certificate length. */
>   	if (!input.pdh_cert_address ||
> @@ -776,25 +756,19 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>   	input_cert_chain_address = (void __user *)input.cert_chain_address;
>   
>   	/* Allocate a physically contiguous buffer to store the PDH blob. */
> -	if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE) {
> -		ret = -EFAULT;
> -		goto e_free;
> -	}
> +	if (input.pdh_cert_len > SEV_FW_BLOB_MAX_SIZE)
> +		return -EFAULT;
>   
>   	/* Allocate a physically contiguous buffer to store the cert chain blob. */
> -	if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE) {
> -		ret = -EFAULT;
> -		goto e_free;
> -	}
> +	if (input.cert_chain_len > SEV_FW_BLOB_MAX_SIZE)
> +		return -EFAULT;
>   
>   	pdh_blob = kmalloc(input.pdh_cert_len, GFP_KERNEL);
> -	if (!pdh_blob) {
> -		ret = -ENOMEM;
> -		goto e_free;
> -	}
> +	if (!pdh_blob)
> +		return -ENOMEM;
>   
> -	data->pdh_cert_address = __psp_pa(pdh_blob);
> -	data->pdh_cert_len = input.pdh_cert_len;
> +	data.pdh_cert_address = __psp_pa(pdh_blob);
> +	data.pdh_cert_len = input.pdh_cert_len;
>   
>   	cert_blob = kmalloc(input.cert_chain_len, GFP_KERNEL);
>   	if (!cert_blob) {
> @@ -802,15 +776,15 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>   		goto e_free_pdh;
>   	}
>   
> -	data->cert_chain_address = __psp_pa(cert_blob);
> -	data->cert_chain_len = input.cert_chain_len;
> +	data.cert_chain_address = __psp_pa(cert_blob);
> +	data.cert_chain_len = input.cert_chain_len;
>   
>   cmd:
> -	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
> +	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
>   
>   	/* If we query the length, FW responded with expected data. */
> -	input.cert_chain_len = data->cert_chain_len;
> -	input.pdh_cert_len = data->pdh_cert_len;
> +	input.cert_chain_len = data.cert_chain_len;
> +	input.pdh_cert_len = data.pdh_cert_len;
>   
>   	if (copy_to_user((void __user *)argp->data, &input, sizeof(input))) {
>   		ret = -EFAULT;
> @@ -835,8 +809,6 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>   	kfree(cert_blob);
>   e_free_pdh:
>   	kfree(pdh_blob);
> -e_free:
> -	kfree(data);
>   	return ret;
>   }
>   
> 

Re: [PATCH v2 5/8] crypto: ccp: Use the stack for small SEV command buffers

[Paolo Bonzini]

On 07/04/21 00:49, Sean Christopherson wrote:
> For commands with small input/output buffers, use the local stack to
> "allocate" the structures used to communicate with the PSP.   Now that
> __sev_do_cmd_locked() gracefully handles vmalloc'd buffers, there's no
> reason to avoid using the stack, e.g. CONFIG_VMAP_STACK=y will just work.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Squashing this in (inspired by Christophe's review, though not quite
matching his suggestion).

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 0f5644a3b138..246b281b6376 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -408,12 +408,11 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
  	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
  		return -EFAULT;
  
+	memset(&data, 0, sizeof(data));
+
  	/* userspace wants to query CSR length */
-	if (!input.address || !input.length) {
-		data.address = 0;
-		data.len = 0;
+	if (!input.address || !input.length)
  		goto cmd;
-	}
  
  	/* allocate a physically contiguous buffer to store the CSR blob */
  	input_address = (void __user *)input.address;

[PATCH v2 6/8] crypto: ccp: Use the stack and common buffer for status commands

[Sean Christopherson]

Drop the dedicated status_cmd_buf and instead use a local variable for
PLATFORM_STATUS.  Now that the low level helper uses an internal buffer
for all commands, using the stack for the upper layers is safe even when
running with CONFIG_VMAP_STACK=y.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 27 ++++++++++++---------------
 drivers/crypto/ccp/sev-dev.h |  1 -
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index bb0d6de071e6..e54774b0d637 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -315,15 +315,14 @@ static int sev_platform_shutdown(int *error)
 
 static int sev_get_platform_state(int *state, int *error)
 {
-	struct sev_device *sev = psp_master->sev_data;
+	struct sev_user_data_status data;
 	int rc;
 
-	rc = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS,
-				 &sev->status_cmd_buf, error);
+	rc = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, error);
 	if (rc)
 		return rc;
 
-	*state = sev->status_cmd_buf.state;
+	*state = data.state;
 	return rc;
 }
 
@@ -361,15 +360,14 @@ static int sev_ioctl_do_reset(struct sev_issue_cmd *argp, bool writable)
 
 static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 {
-	struct sev_device *sev = psp_master->sev_data;
-	struct sev_user_data_status *data = &sev->status_cmd_buf;
+	struct sev_user_data_status data;
 	int ret;
 
-	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, data, &argp->error);
+	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &data, &argp->error);
 	if (ret)
 		return ret;
 
-	if (copy_to_user((void __user *)argp->data, data, sizeof(*data)))
+	if (copy_to_user((void __user *)argp->data, &data, sizeof(data)))
 		ret = -EFAULT;
 
 	return ret;
@@ -469,21 +467,20 @@ EXPORT_SYMBOL_GPL(psp_copy_user_blob);
 static int sev_get_api_version(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	struct sev_user_data_status *status;
+	struct sev_user_data_status status;
 	int error = 0, ret;
 
-	status = &sev->status_cmd_buf;
-	ret = sev_platform_status(status, &error);
+	ret = sev_platform_status(&status, &error);
 	if (ret) {
 		dev_err(sev->dev,
 			"SEV: failed to get status. Error: %#x\n", error);
 		return 1;
 	}
 
-	sev->api_major = status->api_major;
-	sev->api_minor = status->api_minor;
-	sev->build = status->build;
-	sev->state = status->state;
+	sev->api_major = status.api_major;
+	sev->api_minor = status.api_minor;
+	sev->build = status.build;
+	sev->state = status.state;
 
 	return 0;
 }
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index e1572f408577..0fd21433f627 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -46,7 +46,6 @@ struct sev_device {
 	unsigned int int_rcvd;
 	wait_queue_head_t int_queue;
 	struct sev_misc_dev *misc;
-	struct sev_user_data_status status_cmd_buf;
 	struct sev_data_init init_cmd_buf;
 
 	u8 api_major;
-- 
2.31.0.208.g409f899ff0-goog

[PATCH v2 7/8] crypto: ccp: Use the stack and common buffer for INIT command

[Sean Christopherson]

Drop the dedicated init_cmd_buf and instead use a local variable.  Now
that the low level helper uses an internal buffer for all commands,
using the stack for the upper layers is safe even when running with
CONFIG_VMAP_STACK=y.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 10 ++++++----
 drivers/crypto/ccp/sev-dev.h |  1 -
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index e54774b0d637..9ff28df03030 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -233,6 +233,7 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
 static int __sev_platform_init_locked(int *error)
 {
 	struct psp_device *psp = psp_master;
+	struct sev_data_init data;
 	struct sev_device *sev;
 	int rc = 0;
 
@@ -244,6 +245,7 @@ static int __sev_platform_init_locked(int *error)
 	if (sev->state == SEV_STATE_INIT)
 		return 0;
 
+	memset(&data, 0, sizeof(data));
 	if (sev_es_tmr) {
 		u64 tmr_pa;
 
@@ -253,12 +255,12 @@ static int __sev_platform_init_locked(int *error)
 		 */
 		tmr_pa = __pa(sev_es_tmr);
 
-		sev->init_cmd_buf.flags |= SEV_INIT_FLAGS_SEV_ES;
-		sev->init_cmd_buf.tmr_address = tmr_pa;
-		sev->init_cmd_buf.tmr_len = SEV_ES_TMR_SIZE;
+		data.flags |= SEV_INIT_FLAGS_SEV_ES;
+		data.tmr_address = tmr_pa;
+		data.tmr_len = SEV_ES_TMR_SIZE;
 	}
 
-	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &sev->init_cmd_buf, error);
+	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
 	if (rc)
 		return rc;
 
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
index 0fd21433f627..666c21eb81ab 100644
--- a/drivers/crypto/ccp/sev-dev.h
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -46,7 +46,6 @@ struct sev_device {
 	unsigned int int_rcvd;
 	wait_queue_head_t int_queue;
 	struct sev_misc_dev *misc;
-	struct sev_data_init init_cmd_buf;
 
 	u8 api_major;
 	u8 api_minor;
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v2 7/8] crypto: ccp: Use the stack and common buffer for INIT command

[Christophe Leroy]

Le 07/04/2021 à 00:49, Sean Christopherson a écrit :
> Drop the dedicated init_cmd_buf and instead use a local variable.  Now
> that the low level helper uses an internal buffer for all commands,
> using the stack for the upper layers is safe even when running with
> CONFIG_VMAP_STACK=y.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   drivers/crypto/ccp/sev-dev.c | 10 ++++++----
>   drivers/crypto/ccp/sev-dev.h |  1 -
>   2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e54774b0d637..9ff28df03030 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -233,6 +233,7 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
>   static int __sev_platform_init_locked(int *error)
>   {
>   	struct psp_device *psp = psp_master;
> +	struct sev_data_init data;

struct sev_data_init data = {0, 0, 0, 0};

>   	struct sev_device *sev;
>   	int rc = 0;
>   
> @@ -244,6 +245,7 @@ static int __sev_platform_init_locked(int *error)
>   	if (sev->state == SEV_STATE_INIT)
>   		return 0;
>   
> +	memset(&data, 0, sizeof(data));

Not needed.

>   	if (sev_es_tmr) {
>   		u64 tmr_pa;
>   
> @@ -253,12 +255,12 @@ static int __sev_platform_init_locked(int *error)
>   		 */
>   		tmr_pa = __pa(sev_es_tmr);
>   
> -		sev->init_cmd_buf.flags |= SEV_INIT_FLAGS_SEV_ES;
> -		sev->init_cmd_buf.tmr_address = tmr_pa;
> -		sev->init_cmd_buf.tmr_len = SEV_ES_TMR_SIZE;
> +		data.flags |= SEV_INIT_FLAGS_SEV_ES;
> +		data.tmr_address = tmr_pa;
> +		data.tmr_len = SEV_ES_TMR_SIZE;
>   	}
>   
> -	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &sev->init_cmd_buf, error);
> +	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>   	if (rc)
>   		return rc;
>   
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 0fd21433f627..666c21eb81ab 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -46,7 +46,6 @@ struct sev_device {
>   	unsigned int int_rcvd;
>   	wait_queue_head_t int_queue;
>   	struct sev_misc_dev *misc;
> -	struct sev_data_init init_cmd_buf;
>   
>   	u8 api_major;
>   	u8 api_minor;
> 

Re: [PATCH v2 7/8] crypto: ccp: Use the stack and common buffer for INIT command

[Paolo Bonzini]

On 07/04/21 07:20, Christophe Leroy wrote:
>>
>> +    struct sev_data_init data;
> 
> struct sev_data_init data = {0, 0, 0, 0};

Having to count the number of items is suboptimal.  The alternative 
could be {} (which however is technically not standard C), {0} (a bit 
mysterious, but it works) and memset.  I kept the latter to avoid 
touching the submitter's patch too much.

Paolo

[PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack

[Sean Christopherson]

Use the local stack to "allocate" the structures used to communicate with
the PSP.  The largest struct used by KVM, sev_data_launch_secret, clocks
in at 52 bytes, well within the realm of reasonable stack usage.  The
smallest structs are a mere 4 bytes, i.e. the pointer for the allocation
is larger than the allocation itself.

Now that the PSP driver plays nice with vmalloc pointers, putting the
data on a virtually mapped stack (CONFIG_VMAP_STACK=y) will not cause
explosions.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 262 +++++++++++++++--------------------------
 1 file changed, 96 insertions(+), 166 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5457138c7347..316fd39c7aef 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -150,35 +150,22 @@ static void sev_asid_free(int asid)
 
 static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
 {
-	struct sev_data_decommission *decommission;
-	struct sev_data_deactivate *data;
+	struct sev_data_decommission decommission;
+	struct sev_data_deactivate deactivate;
 
 	if (!handle)
 		return;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return;
-
-	/* deactivate handle */
-	data->handle = handle;
+	deactivate.handle = handle;
 
 	/* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */
 	down_read(&sev_deactivate_lock);
-	sev_guest_deactivate(data, NULL);
+	sev_guest_deactivate(&deactivate, NULL);
 	up_read(&sev_deactivate_lock);
 
-	kfree(data);
-
-	decommission = kzalloc(sizeof(*decommission), GFP_KERNEL);
-	if (!decommission)
-		return;
-
 	/* decommission handle */
-	decommission->handle = handle;
-	sev_guest_decommission(decommission, NULL);
-
-	kfree(decommission);
+	decommission.handle = handle;
+	sev_guest_decommission(&decommission, NULL);
 }
 
 static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
@@ -216,19 +203,14 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 
 static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
 {
-	struct sev_data_activate *data;
+	struct sev_data_activate activate;
 	int asid = sev_get_asid(kvm);
 	int ret;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
-
 	/* activate ASID on the given handle */
-	data->handle = handle;
-	data->asid   = asid;
-	ret = sev_guest_activate(data, error);
-	kfree(data);
+	activate.handle = handle;
+	activate.asid   = asid;
+	ret = sev_guest_activate(&activate, error);
 
 	return ret;
 }
@@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
 static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_start *start;
+	struct sev_data_launch_start start;
 	struct kvm_sev_launch_start params;
 	void *dh_blob, *session_blob;
 	int *error = &argp->error;
@@ -270,20 +252,16 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
 		return -EFAULT;
 
-	start = kzalloc(sizeof(*start), GFP_KERNEL_ACCOUNT);
-	if (!start)
-		return -ENOMEM;
+	memset(&start, 0, sizeof(start));
 
 	dh_blob = NULL;
 	if (params.dh_uaddr) {
 		dh_blob = psp_copy_user_blob(params.dh_uaddr, params.dh_len);
-		if (IS_ERR(dh_blob)) {
-			ret = PTR_ERR(dh_blob);
-			goto e_free;
-		}
+		if (IS_ERR(dh_blob))
+			return PTR_ERR(dh_blob);
 
-		start->dh_cert_address = __sme_set(__pa(dh_blob));
-		start->dh_cert_len = params.dh_len;
+		start.dh_cert_address = __sme_set(__pa(dh_blob));
+		start.dh_cert_len = params.dh_len;
 	}
 
 	session_blob = NULL;
@@ -294,40 +272,38 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
 			goto e_free_dh;
 		}
 
-		start->session_address = __sme_set(__pa(session_blob));
-		start->session_len = params.session_len;
+		start.session_address = __sme_set(__pa(session_blob));
+		start.session_len = params.session_len;
 	}
 
-	start->handle = params.handle;
-	start->policy = params.policy;
+	start.handle = params.handle;
+	start.policy = params.policy;
 
 	/* create memory encryption context */
-	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, start, error);
+	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, &start, error);
 	if (ret)
 		goto e_free_session;
 
 	/* Bind ASID to this guest */
-	ret = sev_bind_asid(kvm, start->handle, error);
+	ret = sev_bind_asid(kvm, start.handle, error);
 	if (ret)
 		goto e_free_session;
 
 	/* return handle to userspace */
-	params.handle = start->handle;
+	params.handle = start.handle;
 	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params))) {
-		sev_unbind_asid(kvm, start->handle);
+		sev_unbind_asid(kvm, start.handle);
 		ret = -EFAULT;
 		goto e_free_session;
 	}
 
-	sev->handle = start->handle;
+	sev->handle = start.handle;
 	sev->fd = argp->sev_fd;
 
 e_free_session:
 	kfree(session_blob);
 e_free_dh:
 	kfree(dh_blob);
-e_free:
-	kfree(start);
 	return ret;
 }
 
@@ -446,7 +422,7 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	unsigned long vaddr, vaddr_end, next_vaddr, npages, pages, size, i;
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 	struct kvm_sev_launch_update_data params;
-	struct sev_data_launch_update_data *data;
+	struct sev_data_launch_update_data data;
 	struct page **inpages;
 	int ret;
 
@@ -456,20 +432,14 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
-
 	vaddr = params.uaddr;
 	size = params.len;
 	vaddr_end = vaddr + size;
 
 	/* Lock the user memory. */
 	inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);
-	if (IS_ERR(inpages)) {
-		ret = PTR_ERR(inpages);
-		goto e_free;
-	}
+	if (IS_ERR(inpages))
+		return PTR_ERR(inpages);
 
 	/*
 	 * Flush (on non-coherent CPUs) before LAUNCH_UPDATE encrypts pages in
@@ -477,6 +447,9 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	 */
 	sev_clflush_pages(inpages, npages);
 
+	data.reserved = 0;
+	data.handle = sev->handle;
+
 	for (i = 0; vaddr < vaddr_end; vaddr = next_vaddr, i += pages) {
 		int offset, len;
 
@@ -491,10 +464,9 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 
 		len = min_t(size_t, ((pages * PAGE_SIZE) - offset), size);
 
-		data->handle = sev->handle;
-		data->len = len;
-		data->address = __sme_page_pa(inpages[i]) + offset;
-		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, data, &argp->error);
+		data.len = len;
+		data.address = __sme_page_pa(inpages[i]) + offset;
+		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, &data, &argp->error);
 		if (ret)
 			goto e_unpin;
 
@@ -510,8 +482,6 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 	/* unlock the user pages */
 	sev_unpin_memory(kvm, inpages, npages);
-e_free:
-	kfree(data);
 	return ret;
 }
 
@@ -563,16 +533,14 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_update_vmsa *vmsa;
+	struct sev_data_launch_update_vmsa vmsa;
 	struct kvm_vcpu *vcpu;
 	int i, ret;
 
 	if (!sev_es_guest(kvm))
 		return -ENOTTY;
 
-	vmsa = kzalloc(sizeof(*vmsa), GFP_KERNEL);
-	if (!vmsa)
-		return -ENOMEM;
+	vmsa.reserved = 0;
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		struct vcpu_svm *svm = to_svm(vcpu);
@@ -580,7 +548,7 @@ static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		/* Perform some pre-encryption checks against the VMSA */
 		ret = sev_es_sync_vmsa(svm);
 		if (ret)
-			goto e_free;
+			return ret;
 
 		/*
 		 * The LAUNCH_UPDATE_VMSA command will perform in-place
@@ -590,27 +558,25 @@ static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		 */
 		clflush_cache_range(svm->vmsa, PAGE_SIZE);
 
-		vmsa->handle = sev->handle;
-		vmsa->address = __sme_pa(svm->vmsa);
-		vmsa->len = PAGE_SIZE;
-		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, vmsa,
+		vmsa.handle = sev->handle;
+		vmsa.address = __sme_pa(svm->vmsa);
+		vmsa.len = PAGE_SIZE;
+		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa,
 				    &argp->error);
 		if (ret)
-			goto e_free;
+			return ret;
 
 		svm->vcpu.arch.guest_state_protected = true;
 	}
 
-e_free:
-	kfree(vmsa);
-	return ret;
+	return 0;
 }
 
 static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	void __user *measure = (void __user *)(uintptr_t)argp->data;
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_measure *data;
+	struct sev_data_launch_measure data;
 	struct kvm_sev_launch_measure params;
 	void __user *p = NULL;
 	void *blob = NULL;
@@ -622,9 +588,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (copy_from_user(&params, measure, sizeof(params)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
+	memset(&data, 0, sizeof(data));
 
 	/* User wants to query the blob length */
 	if (!params.len)
@@ -632,23 +596,20 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 
 	p = (void __user *)(uintptr_t)params.uaddr;
 	if (p) {
-		if (params.len > SEV_FW_BLOB_MAX_SIZE) {
-			ret = -EINVAL;
-			goto e_free;
-		}
+		if (params.len > SEV_FW_BLOB_MAX_SIZE)
+			return -EINVAL;
 
-		ret = -ENOMEM;
 		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
 		if (!blob)
-			goto e_free;
+			return -ENOMEM;
 
-		data->address = __psp_pa(blob);
-		data->len = params.len;
+		data.address = __psp_pa(blob);
+		data.len = params.len;
 	}
 
 cmd:
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, data, &argp->error);
+	data.handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, &data, &argp->error);
 
 	/*
 	 * If we query the session length, FW responded with expected data.
@@ -665,63 +626,50 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 
 done:
-	params.len = data->len;
+	params.len = data.len;
 	if (copy_to_user(measure, &params, sizeof(params)))
 		ret = -EFAULT;
 e_free_blob:
 	kfree(blob);
-e_free:
-	kfree(data);
 	return ret;
 }
 
 static int sev_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_finish *data;
-	int ret;
+	struct sev_data_launch_finish data;
 
 	if (!sev_guest(kvm))
 		return -ENOTTY;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
-
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, data, &argp->error);
-
-	kfree(data);
-	return ret;
+	data.handle = sev->handle;
+	return sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, &data, &argp->error);
 }
 
 static int sev_guest_status(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 	struct kvm_sev_guest_status params;
-	struct sev_data_guest_status *data;
+	struct sev_data_guest_status data;
 	int ret;
 
 	if (!sev_guest(kvm))
 		return -ENOTTY;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
+	memset(&data, 0, sizeof(data));
 
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, data, &argp->error);
+	data.handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, &data, &argp->error);
 	if (ret)
-		goto e_free;
+		return ret;
 
-	params.policy = data->policy;
-	params.state = data->state;
-	params.handle = data->handle;
+	params.policy = data.policy;
+	params.state = data.state;
+	params.handle = data.handle;
 
 	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
 		ret = -EFAULT;
-e_free:
-	kfree(data);
+
 	return ret;
 }
 
@@ -730,23 +678,17 @@ static int __sev_issue_dbg_cmd(struct kvm *kvm, unsigned long src,
 			       int *error, bool enc)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_dbg *data;
-	int ret;
+	struct sev_data_dbg data;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
+	data.reserved = 0;
+	data.handle = sev->handle;
+	data.dst_addr = dst;
+	data.src_addr = src;
+	data.len = size;
 
-	data->handle = sev->handle;
-	data->dst_addr = dst;
-	data->src_addr = src;
-	data->len = size;
-
-	ret = sev_issue_cmd(kvm,
-			    enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
-			    data, error);
-	kfree(data);
-	return ret;
+	return sev_issue_cmd(kvm,
+			     enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
+			     &data, error);
 }
 
 static int __sev_dbg_decrypt(struct kvm *kvm, unsigned long src_paddr,
@@ -966,7 +908,7 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
 static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_launch_secret *data;
+	struct sev_data_launch_secret data;
 	struct kvm_sev_launch_secret params;
 	struct page **pages;
 	void *blob, *hdr;
@@ -998,41 +940,36 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		goto e_unpin_memory;
 	}
 
-	ret = -ENOMEM;
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		goto e_unpin_memory;
+	memset(&data, 0, sizeof(data));
 
 	offset = params.guest_uaddr & (PAGE_SIZE - 1);
-	data->guest_address = __sme_page_pa(pages[0]) + offset;
-	data->guest_len = params.guest_len;
+	data.guest_address = __sme_page_pa(pages[0]) + offset;
+	data.guest_len = params.guest_len;
 
 	blob = psp_copy_user_blob(params.trans_uaddr, params.trans_len);
 	if (IS_ERR(blob)) {
 		ret = PTR_ERR(blob);
-		goto e_free;
+		goto e_unpin_memory;
 	}
 
-	data->trans_address = __psp_pa(blob);
-	data->trans_len = params.trans_len;
+	data.trans_address = __psp_pa(blob);
+	data.trans_len = params.trans_len;
 
 	hdr = psp_copy_user_blob(params.hdr_uaddr, params.hdr_len);
 	if (IS_ERR(hdr)) {
 		ret = PTR_ERR(hdr);
 		goto e_free_blob;
 	}
-	data->hdr_address = __psp_pa(hdr);
-	data->hdr_len = params.hdr_len;
+	data.hdr_address = __psp_pa(hdr);
+	data.hdr_len = params.hdr_len;
 
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, data, &argp->error);
+	data.handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, &data, &argp->error);
 
 	kfree(hdr);
 
 e_free_blob:
 	kfree(blob);
-e_free:
-	kfree(data);
 e_unpin_memory:
 	/* content of memory is updated, mark pages dirty */
 	for (i = 0; i < n; i++) {
@@ -1047,7 +984,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
 	void __user *report = (void __user *)(uintptr_t)argp->data;
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_attestation_report *data;
+	struct sev_data_attestation_report data;
 	struct kvm_sev_attestation_report params;
 	void __user *p;
 	void *blob = NULL;
@@ -1059,9 +996,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
 		return -EFAULT;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
-	if (!data)
-		return -ENOMEM;
+	memset(&data, 0, sizeof(data));
 
 	/* User wants to query the blob length */
 	if (!params.len)
@@ -1069,23 +1004,20 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 
 	p = (void __user *)(uintptr_t)params.uaddr;
 	if (p) {
-		if (params.len > SEV_FW_BLOB_MAX_SIZE) {
-			ret = -EINVAL;
-			goto e_free;
-		}
+		if (params.len > SEV_FW_BLOB_MAX_SIZE)
+			return -EINVAL;
 
-		ret = -ENOMEM;
 		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
 		if (!blob)
-			goto e_free;
+			return -ENOMEM;
 
-		data->address = __psp_pa(blob);
-		data->len = params.len;
-		memcpy(data->mnonce, params.mnonce, sizeof(params.mnonce));
+		data.address = __psp_pa(blob);
+		data.len = params.len;
+		memcpy(data.mnonce, params.mnonce, sizeof(params.mnonce));
 	}
 cmd:
-	data->handle = sev->handle;
-	ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, data, &argp->error);
+	data.handle = sev->handle;
+	ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, &data, &argp->error);
 	/*
 	 * If we query the session length, FW responded with expected data.
 	 */
@@ -1101,13 +1033,11 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	}
 
 done:
-	params.len = data->len;
+	params.len = data.len;
 	if (copy_to_user(report, &params, sizeof(params)))
 		ret = -EFAULT;
 e_free_blob:
 	kfree(blob);
-e_free:
-	kfree(data);
 	return ret;
 }
 
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack

[Christophe Leroy]

Le 07/04/2021 à 00:49, Sean Christopherson a écrit :
> Use the local stack to "allocate" the structures used to communicate with
> the PSP.  The largest struct used by KVM, sev_data_launch_secret, clocks
> in at 52 bytes, well within the realm of reasonable stack usage.  The
> smallest structs are a mere 4 bytes, i.e. the pointer for the allocation
> is larger than the allocation itself.
> 
> Now that the PSP driver plays nice with vmalloc pointers, putting the
> data on a virtually mapped stack (CONFIG_VMAP_STACK=y) will not cause
> explosions.
> 
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/sev.c | 262 +++++++++++++++--------------------------
>   1 file changed, 96 insertions(+), 166 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5457138c7347..316fd39c7aef 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -150,35 +150,22 @@ static void sev_asid_free(int asid)
>   
>   static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
>   {
> -	struct sev_data_decommission *decommission;
> -	struct sev_data_deactivate *data;
> +	struct sev_data_decommission decommission;
> +	struct sev_data_deactivate deactivate;
>   
>   	if (!handle)
>   		return;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return;
> -
> -	/* deactivate handle */
> -	data->handle = handle;
> +	deactivate.handle = handle;
>   
>   	/* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */
>   	down_read(&sev_deactivate_lock);
> -	sev_guest_deactivate(data, NULL);
> +	sev_guest_deactivate(&deactivate, NULL);
>   	up_read(&sev_deactivate_lock);
>   
> -	kfree(data);
> -
> -	decommission = kzalloc(sizeof(*decommission), GFP_KERNEL);
> -	if (!decommission)
> -		return;
> -
>   	/* decommission handle */
> -	decommission->handle = handle;
> -	sev_guest_decommission(decommission, NULL);
> -
> -	kfree(decommission);
> +	decommission.handle = handle;
> +	sev_guest_decommission(&decommission, NULL);
>   }
>   
>   static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> @@ -216,19 +203,14 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   
>   static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error)
>   {
> -	struct sev_data_activate *data;
> +	struct sev_data_activate activate;
>   	int asid = sev_get_asid(kvm);
>   	int ret;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
> -	if (!data)
> -		return -ENOMEM;
> -
>   	/* activate ASID on the given handle */
> -	data->handle = handle;
> -	data->asid   = asid;
> -	ret = sev_guest_activate(data, error);
> -	kfree(data);
> +	activate.handle = handle;
> +	activate.asid   = asid;
> +	ret = sev_guest_activate(&activate, error);
>   
>   	return ret;
>   }
> @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
>   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_launch_start *start;
> +	struct sev_data_launch_start start;

struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};

>   	struct kvm_sev_launch_start params;
>   	void *dh_blob, *session_blob;
>   	int *error = &argp->error;
> @@ -270,20 +252,16 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
>   		return -EFAULT;
>   
> -	start = kzalloc(sizeof(*start), GFP_KERNEL_ACCOUNT);
> -	if (!start)
> -		return -ENOMEM;
> +	memset(&start, 0, sizeof(start));

Not needed.

>   
>   	dh_blob = NULL;
>   	if (params.dh_uaddr) {
>   		dh_blob = psp_copy_user_blob(params.dh_uaddr, params.dh_len);
> -		if (IS_ERR(dh_blob)) {
> -			ret = PTR_ERR(dh_blob);
> -			goto e_free;
> -		}
> +		if (IS_ERR(dh_blob))
> +			return PTR_ERR(dh_blob);
>   
> -		start->dh_cert_address = __sme_set(__pa(dh_blob));
> -		start->dh_cert_len = params.dh_len;
> +		start.dh_cert_address = __sme_set(__pa(dh_blob));
> +		start.dh_cert_len = params.dh_len;
>   	}
>   
>   	session_blob = NULL;
> @@ -294,40 +272,38 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   			goto e_free_dh;
>   		}
>   
> -		start->session_address = __sme_set(__pa(session_blob));
> -		start->session_len = params.session_len;
> +		start.session_address = __sme_set(__pa(session_blob));
> +		start.session_len = params.session_len;
>   	}
>   
> -	start->handle = params.handle;
> -	start->policy = params.policy;
> +	start.handle = params.handle;
> +	start.policy = params.policy;
>   
>   	/* create memory encryption context */
> -	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, start, error);
> +	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_LAUNCH_START, &start, error);
>   	if (ret)
>   		goto e_free_session;
>   
>   	/* Bind ASID to this guest */
> -	ret = sev_bind_asid(kvm, start->handle, error);
> +	ret = sev_bind_asid(kvm, start.handle, error);
>   	if (ret)
>   		goto e_free_session;
>   
>   	/* return handle to userspace */
> -	params.handle = start->handle;
> +	params.handle = start.handle;
>   	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params))) {
> -		sev_unbind_asid(kvm, start->handle);
> +		sev_unbind_asid(kvm, start.handle);
>   		ret = -EFAULT;
>   		goto e_free_session;
>   	}
>   
> -	sev->handle = start->handle;
> +	sev->handle = start.handle;
>   	sev->fd = argp->sev_fd;
>   
>   e_free_session:
>   	kfree(session_blob);
>   e_free_dh:
>   	kfree(dh_blob);
> -e_free:
> -	kfree(start);
>   	return ret;
>   }
>   
> @@ -446,7 +422,7 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	unsigned long vaddr, vaddr_end, next_vaddr, npages, pages, size, i;
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>   	struct kvm_sev_launch_update_data params;
> -	struct sev_data_launch_update_data *data;
> +	struct sev_data_launch_update_data data;
>   	struct page **inpages;
>   	int ret;
>   
> @@ -456,20 +432,14 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
>   		return -EFAULT;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
> -	if (!data)
> -		return -ENOMEM;
> -
>   	vaddr = params.uaddr;
>   	size = params.len;
>   	vaddr_end = vaddr + size;
>   
>   	/* Lock the user memory. */
>   	inpages = sev_pin_memory(kvm, vaddr, size, &npages, 1);
> -	if (IS_ERR(inpages)) {
> -		ret = PTR_ERR(inpages);
> -		goto e_free;
> -	}
> +	if (IS_ERR(inpages))
> +		return PTR_ERR(inpages);
>   
>   	/*
>   	 * Flush (on non-coherent CPUs) before LAUNCH_UPDATE encrypts pages in
> @@ -477,6 +447,9 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	 */
>   	sev_clflush_pages(inpages, npages);
>   
> +	data.reserved = 0;
> +	data.handle = sev->handle;
> +
>   	for (i = 0; vaddr < vaddr_end; vaddr = next_vaddr, i += pages) {
>   		int offset, len;
>   
> @@ -491,10 +464,9 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   
>   		len = min_t(size_t, ((pages * PAGE_SIZE) - offset), size);
>   
> -		data->handle = sev->handle;
> -		data->len = len;
> -		data->address = __sme_page_pa(inpages[i]) + offset;
> -		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, data, &argp->error);
> +		data.len = len;
> +		data.address = __sme_page_pa(inpages[i]) + offset;
> +		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_DATA, &data, &argp->error);
>   		if (ret)
>   			goto e_unpin;
>   
> @@ -510,8 +482,6 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	}
>   	/* unlock the user pages */
>   	sev_unpin_memory(kvm, inpages, npages);
> -e_free:
> -	kfree(data);
>   	return ret;
>   }
>   
> @@ -563,16 +533,14 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>   static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_launch_update_vmsa *vmsa;
> +	struct sev_data_launch_update_vmsa vmsa;
>   	struct kvm_vcpu *vcpu;
>   	int i, ret;
>   
>   	if (!sev_es_guest(kvm))
>   		return -ENOTTY;
>   
> -	vmsa = kzalloc(sizeof(*vmsa), GFP_KERNEL);
> -	if (!vmsa)
> -		return -ENOMEM;
> +	vmsa.reserved = 0;
>   
>   	kvm_for_each_vcpu(i, vcpu, kvm) {
>   		struct vcpu_svm *svm = to_svm(vcpu);
> @@ -580,7 +548,7 @@ static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   		/* Perform some pre-encryption checks against the VMSA */
>   		ret = sev_es_sync_vmsa(svm);
>   		if (ret)
> -			goto e_free;
> +			return ret;
>   
>   		/*
>   		 * The LAUNCH_UPDATE_VMSA command will perform in-place
> @@ -590,27 +558,25 @@ static int sev_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   		 */
>   		clflush_cache_range(svm->vmsa, PAGE_SIZE);
>   
> -		vmsa->handle = sev->handle;
> -		vmsa->address = __sme_pa(svm->vmsa);
> -		vmsa->len = PAGE_SIZE;
> -		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, vmsa,
> +		vmsa.handle = sev->handle;
> +		vmsa.address = __sme_pa(svm->vmsa);
> +		vmsa.len = PAGE_SIZE;
> +		ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_VMSA, &vmsa,
>   				    &argp->error);
>   		if (ret)
> -			goto e_free;
> +			return ret;
>   
>   		svm->vcpu.arch.guest_state_protected = true;
>   	}
>   
> -e_free:
> -	kfree(vmsa);
> -	return ret;
> +	return 0;
>   }
>   
>   static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
>   	void __user *measure = (void __user *)(uintptr_t)argp->data;
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_launch_measure *data;
> +	struct sev_data_launch_measure data;

struct sev_data_launch_measure data = {0, 0, 0, 0};

>   	struct kvm_sev_launch_measure params;
>   	void __user *p = NULL;
>   	void *blob = NULL;
> @@ -622,9 +588,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	if (copy_from_user(&params, measure, sizeof(params)))
>   		return -EFAULT;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
> -	if (!data)
> -		return -ENOMEM;
> +	memset(&data, 0, sizeof(data));

Not needed

>   
>   	/* User wants to query the blob length */
>   	if (!params.len)
> @@ -632,23 +596,20 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   
>   	p = (void __user *)(uintptr_t)params.uaddr;
>   	if (p) {
> -		if (params.len > SEV_FW_BLOB_MAX_SIZE) {
> -			ret = -EINVAL;
> -			goto e_free;
> -		}
> +		if (params.len > SEV_FW_BLOB_MAX_SIZE)
> +			return -EINVAL;
>   
> -		ret = -ENOMEM;
>   		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
>   		if (!blob)
> -			goto e_free;
> +			return -ENOMEM;
>   
> -		data->address = __psp_pa(blob);
> -		data->len = params.len;
> +		data.address = __psp_pa(blob);
> +		data.len = params.len;
>   	}
>   
>   cmd:
> -	data->handle = sev->handle;
> -	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, data, &argp->error);
> +	data.handle = sev->handle;
> +	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_MEASURE, &data, &argp->error);
>   
>   	/*
>   	 * If we query the session length, FW responded with expected data.
> @@ -665,63 +626,50 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	}
>   
>   done:
> -	params.len = data->len;
> +	params.len = data.len;
>   	if (copy_to_user(measure, &params, sizeof(params)))
>   		ret = -EFAULT;
>   e_free_blob:
>   	kfree(blob);
> -e_free:
> -	kfree(data);
>   	return ret;
>   }
>   
>   static int sev_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_launch_finish *data;
> -	int ret;
> +	struct sev_data_launch_finish data;
>   
>   	if (!sev_guest(kvm))
>   		return -ENOTTY;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
> -	if (!data)
> -		return -ENOMEM;
> -
> -	data->handle = sev->handle;
> -	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, data, &argp->error);
> -
> -	kfree(data);
> -	return ret;
> +	data.handle = sev->handle;
> +	return sev_issue_cmd(kvm, SEV_CMD_LAUNCH_FINISH, &data, &argp->error);
>   }
>   
>   static int sev_guest_status(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>   	struct kvm_sev_guest_status params;
> -	struct sev_data_guest_status *data;
> +	struct sev_data_guest_status data = {0, 0, 0, 0};
>   	int ret;
>   
>   	if (!sev_guest(kvm))
>   		return -ENOTTY;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
> -	if (!data)
> -		return -ENOMEM;
> +	memset(&data, 0, sizeof(data));

not needed

>   
> -	data->handle = sev->handle;
> -	ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, data, &argp->error);
> +	data.handle = sev->handle;
> +	ret = sev_issue_cmd(kvm, SEV_CMD_GUEST_STATUS, &data, &argp->error);
>   	if (ret)
> -		goto e_free;
> +		return ret;
>   
> -	params.policy = data->policy;
> -	params.state = data->state;
> -	params.handle = data->handle;
> +	params.policy = data.policy;
> +	params.state = data.state;
> +	params.handle = data.handle;
>   
>   	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
>   		ret = -EFAULT;
> -e_free:
> -	kfree(data);
> +
>   	return ret;
>   }
>   
> @@ -730,23 +678,17 @@ static int __sev_issue_dbg_cmd(struct kvm *kvm, unsigned long src,
>   			       int *error, bool enc)
>   {
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_dbg *data;
> -	int ret;
> +	struct sev_data_dbg data;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
> -	if (!data)
> -		return -ENOMEM;
> +	data.reserved = 0;
> +	data.handle = sev->handle;
> +	data.dst_addr = dst;
> +	data.src_addr = src;
> +	data.len = size;
>   
> -	data->handle = sev->handle;
> -	data->dst_addr = dst;
> -	data->src_addr = src;
> -	data->len = size;
> -
> -	ret = sev_issue_cmd(kvm,
> -			    enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
> -			    data, error);
> -	kfree(data);
> -	return ret;
> +	return sev_issue_cmd(kvm,
> +			     enc ? SEV_CMD_DBG_ENCRYPT : SEV_CMD_DBG_DECRYPT,
> +			     &data, error);
>   }
>   
>   static int __sev_dbg_decrypt(struct kvm *kvm, unsigned long src_paddr,
> @@ -966,7 +908,7 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp, bool dec)
>   static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_launch_secret *data;
> +	struct sev_data_launch_secret data;
>   	struct kvm_sev_launch_secret params;
>   	struct page **pages;
>   	void *blob, *hdr;
> @@ -998,41 +940,36 @@ static int sev_launch_secret(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   		goto e_unpin_memory;
>   	}
>   
> -	ret = -ENOMEM;
> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
> -	if (!data)
> -		goto e_unpin_memory;
> +	memset(&data, 0, sizeof(data));
>   
>   	offset = params.guest_uaddr & (PAGE_SIZE - 1);
> -	data->guest_address = __sme_page_pa(pages[0]) + offset;
> -	data->guest_len = params.guest_len;
> +	data.guest_address = __sme_page_pa(pages[0]) + offset;
> +	data.guest_len = params.guest_len;
>   
>   	blob = psp_copy_user_blob(params.trans_uaddr, params.trans_len);
>   	if (IS_ERR(blob)) {
>   		ret = PTR_ERR(blob);
> -		goto e_free;
> +		goto e_unpin_memory;
>   	}
>   
> -	data->trans_address = __psp_pa(blob);
> -	data->trans_len = params.trans_len;
> +	data.trans_address = __psp_pa(blob);
> +	data.trans_len = params.trans_len;
>   
>   	hdr = psp_copy_user_blob(params.hdr_uaddr, params.hdr_len);
>   	if (IS_ERR(hdr)) {
>   		ret = PTR_ERR(hdr);
>   		goto e_free_blob;
>   	}
> -	data->hdr_address = __psp_pa(hdr);
> -	data->hdr_len = params.hdr_len;
> +	data.hdr_address = __psp_pa(hdr);
> +	data.hdr_len = params.hdr_len;
>   
> -	data->handle = sev->handle;
> -	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, data, &argp->error);
> +	data.handle = sev->handle;
> +	ret = sev_issue_cmd(kvm, SEV_CMD_LAUNCH_UPDATE_SECRET, &data, &argp->error);
>   
>   	kfree(hdr);
>   
>   e_free_blob:
>   	kfree(blob);
> -e_free:
> -	kfree(data);
>   e_unpin_memory:
>   	/* content of memory is updated, mark pages dirty */
>   	for (i = 0; i < n; i++) {
> @@ -1047,7 +984,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   {
>   	void __user *report = (void __user *)(uintptr_t)argp->data;
>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	struct sev_data_attestation_report *data;
> +	struct sev_data_attestation_report data;
>   	struct kvm_sev_attestation_report params;
>   	void __user *p;
>   	void *blob = NULL;
> @@ -1059,9 +996,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
>   		return -EFAULT;
>   
> -	data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT);
> -	if (!data)
> -		return -ENOMEM;
> +	memset(&data, 0, sizeof(data));
>   
>   	/* User wants to query the blob length */
>   	if (!params.len)
> @@ -1069,23 +1004,20 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   
>   	p = (void __user *)(uintptr_t)params.uaddr;
>   	if (p) {
> -		if (params.len > SEV_FW_BLOB_MAX_SIZE) {
> -			ret = -EINVAL;
> -			goto e_free;
> -		}
> +		if (params.len > SEV_FW_BLOB_MAX_SIZE)
> +			return -EINVAL;
>   
> -		ret = -ENOMEM;
>   		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
>   		if (!blob)
> -			goto e_free;
> +			return -ENOMEM;
>   
> -		data->address = __psp_pa(blob);
> -		data->len = params.len;
> -		memcpy(data->mnonce, params.mnonce, sizeof(params.mnonce));
> +		data.address = __psp_pa(blob);
> +		data.len = params.len;
> +		memcpy(data.mnonce, params.mnonce, sizeof(params.mnonce));
>   	}
>   cmd:
> -	data->handle = sev->handle;
> -	ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, data, &argp->error);
> +	data.handle = sev->handle;
> +	ret = sev_issue_cmd(kvm, SEV_CMD_ATTESTATION_REPORT, &data, &argp->error);
>   	/*
>   	 * If we query the session length, FW responded with expected data.
>   	 */
> @@ -1101,13 +1033,11 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   	}
>   
>   done:
> -	params.len = data->len;
> +	params.len = data.len;
>   	if (copy_to_user(report, &params, sizeof(params)))
>   		ret = -EFAULT;
>   e_free_blob:
>   	kfree(blob);
> -e_free:
> -	kfree(data);
>   	return ret;
>   }
>   
> 

Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack

[Borislav Petkov]

First of all, I'd strongly suggest you trim your emails when you reply -
that would be much appreciated.

On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:
> > @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
> >   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >   {
> >   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > -	struct sev_data_launch_start *start;
> > +	struct sev_data_launch_start start;
> 
> struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};

I don't know how this is any better than using memset...

Also, you can do

	... start = { };

which is certainly the only other alternative to memset, AFAIK.

But whatever you do, you need to look at the resulting asm the compiler
generates. So let's do that:

Your version:

# arch/x86/kvm/svm/sev.c:261:   struct sev_data_launch_start _tmp = {0, 0, 0, 0, 0, 0, 0};
        movq    $0, 104(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 112(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 120(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 128(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movl    $0, 136(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]


my version:

# arch/x86/kvm/svm/sev.c:261:   struct sev_data_launch_start _tmp = {};
        movq    $0, 104(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 112(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 120(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movq    $0, 128(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]
        movl    $0, 136(%rsp)   #, MEM[(struct sev_data_launch_start *)_561]


the memset version:

# arch/x86/kvm/svm/sev.c:269: 	memset(&_tmp, 0, sizeof(_tmp));
#NO_APP
	movq	$0, 104(%rsp)	#, MEM <char[1:36]> [(void *)_561]
	movq	$0, 112(%rsp)	#, MEM <char[1:36]> [(void *)_561]
	movq	$0, 120(%rsp)	#, MEM <char[1:36]> [(void *)_561]
	movq	$0, 128(%rsp)	#, MEM <char[1:36]> [(void *)_561]
	movl	$0, 136(%rsp)	#, MEM <char[1:36]> [(void *)_561]

Ok?

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack

[Sean Christopherson]

On Wed, Apr 07, 2021, Borislav Petkov wrote:
> First of all, I'd strongly suggest you trim your emails when you reply -
> that would be much appreciated.
> 
> On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:
> > > @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
> > >   static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >   {
> > >   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > -	struct sev_data_launch_start *start;
> > > +	struct sev_data_launch_start start;
> > 
> > struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};
> 
> I don't know how this is any better than using memset...
> 
> Also, you can do
> 
> 	... start = { };
> 
> which is certainly the only other alternative to memset, AFAIK.
> 
> But whatever you do, you need to look at the resulting asm the compiler
> generates. So let's do that:

I'm ok with Boris' version, I'm not a fan of having to count zeros.  I used
memset() to defer initialization until after the various sanity checks, and
out of habit.

Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack

[Christophe Leroy]

Le 07/04/2021 à 19:05, Sean Christopherson a écrit :
> On Wed, Apr 07, 2021, Borislav Petkov wrote:
>> First of all, I'd strongly suggest you trim your emails when you reply -
>> that would be much appreciated.
>>
>> On Wed, Apr 07, 2021 at 07:24:54AM +0200, Christophe Leroy wrote:
>>>> @@ -258,7 +240,7 @@ static int sev_issue_cmd(struct kvm *kvm, int id, void *data, int *error)
>>>>    static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>>    {
>>>>    	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>> -	struct sev_data_launch_start *start;
>>>> +	struct sev_data_launch_start start;
>>>
>>> struct sev_data_launch_start start = {0, 0, 0, 0, 0, 0, 0};
>>
>> I don't know how this is any better than using memset...
>>
>> Also, you can do
>>
>> 	... start = { };
>>
>> which is certainly the only other alternative to memset, AFAIK.
>>
>> But whatever you do, you need to look at the resulting asm the compiler
>> generates. So let's do that:
> 
> I'm ok with Boris' version, I'm not a fan of having to count zeros.  I used
> memset() to defer initialization until after the various sanity checks, and
> out of habit.
> 

Yes I also like Boris' version 	... start = { };  better than mine.

Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack

[Borislav Petkov]

On Wed, Apr 07, 2021 at 05:05:07PM +0000, Sean Christopherson wrote:
> I used memset() to defer initialization until after the various sanity
> checks,

I'd actually vote for that too - I don't like doing stuff which is not
going to be used. I.e., don't change what you have.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Re: [PATCH v2 8/8] KVM: SVM: Allocate SEV command structures on local stack

[Paolo Bonzini]

On 07/04/21 19:34, Borislav Petkov wrote:
> On Wed, Apr 07, 2021 at 05:05:07PM +0000, Sean Christopherson wrote:
>> I used memset() to defer initialization until after the various sanity
>> checks,
> 
> I'd actually vote for that too - I don't like doing stuff which is not
> going to be used. I.e., don't change what you have.

It's three votes for that then. :)

Sean, I squashed in this change

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index ec4c01807272..a4d0ca8c4710 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1039,21 +1039,14 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
  static int sev_send_cancel(struct kvm *kvm, struct kvm_sev_cmd *argp)
  {
  	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	struct sev_data_send_cancel *data;
+	struct sev_data_send_cancel 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_CANCEL, data, &argp->error);
-
-	kfree(data);
-	return ret;
+	data.handle = sev->handle;
+	return sev_issue_cmd(kvm, SEV_CMD_SEND_CANCEL, &data, &argp->error);
  }
  
  int svm_mem_enc_op(struct kvm *kvm, void __user *argp)

to handle SV_CMD_SEND_CANCEL which I merged before this series.

Paolo

Re: [PATCH v2 0/8] ccp: KVM: SVM: Use stack for SEV command buffers

[Brijesh Singh]

On 4/6/21 5:49 PM, Sean Christopherson wrote:
> This series teaches __sev_do_cmd_locked() to gracefully handle vmalloc'd
> command buffers by copying _all_ incoming data pointers to an internal
> buffer before sending the command to the PSP.  The SEV driver and KVM are
> then converted to use the stack for all command buffers.
>
> Tested everything except sev_ioctl_do_pek_import(), I don't know anywhere
> near enough about the PSP to give it the right input.
>
> v2:
>   - Rebase to kvm/queue, commit f96be2deac9b ("KVM: x86: Support KVM VMs
>     sharing SEV context").
>   - Unconditionally copy @data to the internal buffer. [Christophe, Brijesh]
>   - Allocate a full page for the buffer. [Brijesh]
>   - Drop one set of the "!"s. [Christophe]
>   - Use virt_addr_valid() instead of is_vmalloc_addr() for the temporary
>     patch (definitely feel free to drop the patch if it's not worth
>     backporting). [Christophe]
>   - s/intput/input/. [Tom]
>   - Add a patch to free "sev" if init fails.  This is not strictly
>     necessary (I think; I suck horribly when it comes to the driver
>     framework).   But it felt wrong to not free cmd_buf on failure, and
>     even more wrong to free cmd_buf but not sev.
>
> v1:
>   - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F20210402233702.3291792-1-seanjc%40google.com&data=04%7C01%7Cbrijesh.singh%40amd.com%7C051db746fc2048e06acb08d8f94e527b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637533462083069551%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bbNHBXMO1RWh8i4siTYkv4P92Ph5C7SnAZ3uTPsxgvg%3D&reserved=0
>
> Sean Christopherson (8):
>   crypto: ccp: Free SEV device if SEV init fails
>   crypto: ccp: Detect and reject "invalid" addresses destined for PSP
>   crypto: ccp: Reject SEV commands with mismatching command buffer
>   crypto: ccp: Play nice with vmalloc'd memory for SEV command structs
>   crypto: ccp: Use the stack for small SEV command buffers
>   crypto: ccp: Use the stack and common buffer for status commands
>   crypto: ccp: Use the stack and common buffer for INIT command
>   KVM: SVM: Allocate SEV command structures on local stack
>
>  arch/x86/kvm/svm/sev.c       | 262 +++++++++++++----------------------
>  drivers/crypto/ccp/sev-dev.c | 197 +++++++++++++-------------
>  drivers/crypto/ccp/sev-dev.h |   4 +-
>  3 files changed, 196 insertions(+), 267 deletions(-)
>

Thanks Sean.

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

Re: [PATCH v2 0/8] ccp: KVM: SVM: Use stack for SEV command buffers

[Tom Lendacky]

On 4/6/21 5:49 PM, Sean Christopherson wrote:
> This series teaches __sev_do_cmd_locked() to gracefully handle vmalloc'd
> command buffers by copying _all_ incoming data pointers to an internal
> buffer before sending the command to the PSP.  The SEV driver and KVM are
> then converted to use the stack for all command buffers.
> 
> Tested everything except sev_ioctl_do_pek_import(), I don't know anywhere
> near enough about the PSP to give it the right input.
> 
> v2:
>   - Rebase to kvm/queue, commit f96be2deac9b ("KVM: x86: Support KVM VMs
>     sharing SEV context").
>   - Unconditionally copy @data to the internal buffer. [Christophe, Brijesh]
>   - Allocate a full page for the buffer. [Brijesh]
>   - Drop one set of the "!"s. [Christophe]
>   - Use virt_addr_valid() instead of is_vmalloc_addr() for the temporary
>     patch (definitely feel free to drop the patch if it's not worth
>     backporting). [Christophe]
>   - s/intput/input/. [Tom]
>   - Add a patch to free "sev" if init fails.  This is not strictly
>     necessary (I think; I suck horribly when it comes to the driver
>     framework).   But it felt wrong to not free cmd_buf on failure, and
>     even more wrong to free cmd_buf but not sev.
> 
> v1:
>   - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F20210402233702.3291792-1-seanjc%40google.com&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cecd38fba67954845323908d8f94e5405%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637533462102772796%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SUN8Zp%2Fi%2BiHAjMSe%2Fjwvs9JmXg%2FRvi%2B8j01sLDipPg8%3D&reserved=0
> 
> Sean Christopherson (8):
>   crypto: ccp: Free SEV device if SEV init fails
>   crypto: ccp: Detect and reject "invalid" addresses destined for PSP
>   crypto: ccp: Reject SEV commands with mismatching command buffer
>   crypto: ccp: Play nice with vmalloc'd memory for SEV command structs
>   crypto: ccp: Use the stack for small SEV command buffers
>   crypto: ccp: Use the stack and common buffer for status commands
>   crypto: ccp: Use the stack and common buffer for INIT command
>   KVM: SVM: Allocate SEV command structures on local stack
> 
>  arch/x86/kvm/svm/sev.c       | 262 +++++++++++++----------------------
>  drivers/crypto/ccp/sev-dev.c | 197 +++++++++++++-------------
>  drivers/crypto/ccp/sev-dev.h |   4 +-
>  3 files changed, 196 insertions(+), 267 deletions(-)

For the series:

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> 

Re: [PATCH v2 0/8] ccp: KVM: SVM: Use stack for SEV command buffers

[Paolo Bonzini]

On 07/04/21 20:00, Tom Lendacky wrote:
> For the series:
> 
> Acked-by: Tom Lendacky<thomas.lendacky@amd.com>

Shall I take this as a request (or permission, whatever :)) to merge it 
through the KVM tree?

Paolo

Re: [PATCH v2 0/8] ccp: KVM: SVM: Use stack for SEV command buffers

[Tom Lendacky]

On 4/15/21 11:09 AM, Paolo Bonzini wrote:
> On 07/04/21 20:00, Tom Lendacky wrote:
>> For the series:
>>
>> Acked-by: Tom Lendacky<thomas.lendacky@amd.com>
> 
> Shall I take this as a request (or permission, whatever :)) to merge it
> through the KVM tree?

Adding Herbert. Here's a link to the series:

https://lore.kernel.org/kvm/88eef561-6fd8-a495-0d60-ff688070cc9e@redhat.com/T/#m2bbdd12452970d3bd7d0b1464c22bf2f0227a9f1

I'm not sure how you typically do the cross-tree stuff. Patch 8 has a
requirement on patches 1-7. The arch/x86/kvm/svm/sev.c file tends to have
more activity/changes than drivers/crypto/ccp/sev-dev.{c,h}, so it would
make sense to take it through the KVM tree. But I think you need to verify
that with Herbert.

Thanks,
Tom

> 
> Paolo
> 

Re: [PATCH v2 0/8] ccp: KVM: SVM: Use stack for SEV command buffers

[Herbert Xu]

On Thu, Apr 15, 2021 at 01:15:59PM -0500, Tom Lendacky wrote:
> On 4/15/21 11:09 AM, Paolo Bonzini wrote:
> > On 07/04/21 20:00, Tom Lendacky wrote:
> >> For the series:
> >>
> >> Acked-by: Tom Lendacky<thomas.lendacky@amd.com>
> > 
> > Shall I take this as a request (or permission, whatever :)) to merge it
> > through the KVM tree?
> 
> Adding Herbert. Here's a link to the series:
> 
> https://lore.kernel.org/kvm/88eef561-6fd8-a495-0d60-ff688070cc9e@redhat.com/T/#m2bbdd12452970d3bd7d0b1464c22bf2f0227a9f1
> 
> I'm not sure how you typically do the cross-tree stuff. Patch 8 has a
> requirement on patches 1-7. The arch/x86/kvm/svm/sev.c file tends to have
> more activity/changes than drivers/crypto/ccp/sev-dev.{c,h}, so it would
> make sense to take it through the KVM tree. But I think you need to verify
> that with Herbert.

I don't mind at all.  Paolo you can take this through your tree.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 0/8] ccp: KVM: SVM: Use stack for SEV command buffers

[Paolo Bonzini]

On 07/04/21 00:49, Sean Christopherson wrote:
> This series teaches __sev_do_cmd_locked() to gracefully handle vmalloc'd
> command buffers by copying _all_ incoming data pointers to an internal
> buffer before sending the command to the PSP.  The SEV driver and KVM are
> then converted to use the stack for all command buffers.
> 
> Tested everything except sev_ioctl_do_pek_import(), I don't know anywhere
> near enough about the PSP to give it the right input.
> 
> v2:
>    - Rebase to kvm/queue, commit f96be2deac9b ("KVM: x86: Support KVM VMs
>      sharing SEV context").
>    - Unconditionally copy @data to the internal buffer. [Christophe, Brijesh]
>    - Allocate a full page for the buffer. [Brijesh]
>    - Drop one set of the "!"s. [Christophe]
>    - Use virt_addr_valid() instead of is_vmalloc_addr() for the temporary
>      patch (definitely feel free to drop the patch if it's not worth
>      backporting). [Christophe]
>    - s/intput/input/. [Tom]
>    - Add a patch to free "sev" if init fails.  This is not strictly
>      necessary (I think; I suck horribly when it comes to the driver
>      framework).   But it felt wrong to not free cmd_buf on failure, and
>      even more wrong to free cmd_buf but not sev.
> 
> v1:
>    - https://lkml.kernel.org/r/20210402233702.3291792-1-seanjc@google.com
> 
> Sean Christopherson (8):
>    crypto: ccp: Free SEV device if SEV init fails
>    crypto: ccp: Detect and reject "invalid" addresses destined for PSP
>    crypto: ccp: Reject SEV commands with mismatching command buffer
>    crypto: ccp: Play nice with vmalloc'd memory for SEV command structs
>    crypto: ccp: Use the stack for small SEV command buffers
>    crypto: ccp: Use the stack and common buffer for status commands
>    crypto: ccp: Use the stack and common buffer for INIT command
>    KVM: SVM: Allocate SEV command structures on local stack
> 
>   arch/x86/kvm/svm/sev.c       | 262 +++++++++++++----------------------
>   drivers/crypto/ccp/sev-dev.c | 197 +++++++++++++-------------
>   drivers/crypto/ccp/sev-dev.h |   4 +-
>   3 files changed, 196 insertions(+), 267 deletions(-)
> 

Queued, thanks.

Paolo