Skip to contentSkip to conversation

ccp: KVM: SVM: Use stack for SEV command buffers

PATCH Superseded Sean Christopherson 4 participants 17 messages Apr 2, 2021 – Apr 5, 2021

Subsystems (6)
x86 KVM Crypto API KVM x86 +2 more subsystems

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Sean Christopherson

author maintainer
#0 lore raw

cover ccp: KVM: SVM: Use stack for SEV command buffers

While doing minor KVM cleanup to account various kernel allocations, I noticed that all of the SEV command buffers are allocated via kmalloc(), even for commands whose payloads is smaller than a pointer. After much head scratching, the only reason I could come up with for dynamically allocating the command data is CONFIG_VMAP_STACK=y.

This series teaches __sev_do_cmd_locked() to gracefully handle vmalloc'd command buffers by copying such buffers 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.

The first patch is optional, I included it in case someone wants to backport it to stable kernels. It wouldn't actually fix bugs, but it would make debugging issues a lot easier if they did pop up.

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

Based on kvm/queue, commit f96be2deac9b ("KVM: x86: Support KVM VMs sharing SEV context") to avoid a minor conflict.

Sean Christopherson (5):
crypto: ccp: Detect and reject vmalloc 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
KVM: SVM: Allocate SEV command structures on local stack

 arch/x86/kvm/svm/sev.c       | 262 +++++++++++++----------------------
 drivers/crypto/ccp/sev-dev.c | 161 ++++++++++-----------
 drivers/crypto/ccp/sev-dev.h |   7 +
 3 files changed, 184 insertions(+), 246 deletions(-)
Signature · 1 line
2.31.0.208.g409f899ff0-goog
author maintainer
#1 lore raw

PATCH 1/5 crypto: ccp: Detect and reject vmalloc addresses destined for PSP

Explicitly reject vmalloc'd data as the source for SEV commands that are sent to the PSP. The PSP works with physical addresses, and __pa() will not return the correct address for a vmalloc'd pionter, which at best will cause the command to fail, and at worst lead to system instability.

While it's unlikely that callers will deliberately use vmalloc() 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 <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Fixes 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support")
Signed-off-by Sean Christopherson <[email protected]>
---
 drivers/crypto/ccp/sev-dev.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/crypto/ccp/sev-dev.c +3 -0
index cb9b4c4e371e..6556d220713b 100644
@@ -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(is_vmalloc_addr(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;
Signature · 1 line
2.31.0.208.g409f899ff0-goog
#2 lore raw

Re: [PATCH 1/5] crypto: ccp: Detect and reject vmalloc addresses destined for PSP

Le 03/04/2021 à 01:36, Sean Christopherson a écrit :

Quoted 27 lines

Explicitly reject vmalloc'd data as the source for SEV commands that are sent to the PSP. The PSP works with physical addresses, and __pa() will not return the correct address for a vmalloc'd pionter, which at best will cause the command to fail, and at worst lead to system instability.

While it's unlikely that callers will deliberately use vmalloc() 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 <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Fixes 200664d5237f ("crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support")
Signed-off-by Sean Christopherson <[email protected]>
---
  drivers/crypto/ccp/sev-dev.c | 3 +++
  1 file changed, 3 insertions(+)
drivers/crypto/ccp/sev-dev.c +0 -0
index cb9b4c4e371e..6556d220713b 100644

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 155

Quoted 6 lines
drivers/crypto/ccp/sev-dev.c +3 -0
@@ -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(is_vmalloc_addr(data)))
+		return -EINVAL;
+

I hadn't seen this patch when I commented the 2 other ones, I received it only this night.

As commented in the other patches, is_vmalloc_addr() is not the best way to test that __pa() can be safely used.

For that, you have virt_addr_valid()

Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +0 -0
  	/* 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;
author maintainer
#3 lore raw

PATCH 2/5 crypto: ccp: Reject SEV commands with mismatching command buffer

In reply to 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.

Aside from sanity checking intput, 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 <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
 drivers/crypto/ccp/sev-dev.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/crypto/ccp/sev-dev.c +8 -3
index 6556d220713b..4c513318f16a 100644
@@ -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,7 +151,11 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
 	sev = psp->sev_data;
 
-	if (data && WARN_ON_ONCE(is_vmalloc_addr(data)))
+	buf_len = sev_cmd_buffer_len(cmd);
+	if (WARN_ON_ONCE(!!data != !!buf_len))
+		return -EINVAL;
+
+	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
 		return -EINVAL;
 
 	/* Get the physical address of the command buffer */

@@ -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;
 }
Signature · 1 line
2.31.0.208.g409f899ff0-goog
#4 lore raw

Re: [PATCH 2/5] crypto: ccp: Reject SEV commands with mismatching command buffer

Le 03/04/2021 à 01:36, Sean Christopherson a écrit :

Quoted 34 lines

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.

Aside from sanity checking intput, 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 <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
  drivers/crypto/ccp/sev-dev.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)
drivers/crypto/ccp/sev-dev.c +1 -0
index 6556d220713b..4c513318f16a 100644
@@ -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;

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 158

Quoted 9 lines
drivers/crypto/ccp/sev-dev.c +5 -1
@@ -150,7 +151,11 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
  	sev = psp->sev_data;
 
-	if (data && WARN_ON_ONCE(is_vmalloc_addr(data)))
+	buf_len = sev_cmd_buffer_len(cmd);
+	if (WARN_ON_ONCE(!!data != !!buf_len))
+		return -EINVAL;
+
+	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))

Shouldn't it be !virt_addr_valid() instead of is_vmalloc_addr() ?

Quoted 22 lines
drivers/crypto/ccp/sev-dev.c +2 -2
  		return -EINVAL;
 
  	/* Get the physical address of the command buffer */

@@ -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;
  }
maintainer
#5 lore raw

Re: [PATCH 2/5] crypto: ccp: Reject SEV commands with mismatching command buffer

In reply to Sean Christopherson
Quoted 5 lines · 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.

Aside from sanity checking intput, disallowing a non-null pointer without

s/intput/input/

Quoted 29 lines

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 <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
 drivers/crypto/ccp/sev-dev.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/crypto/ccp/sev-dev.c +1 -0
index 6556d220713b..4c513318f16a 100644
@@ -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;

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 155

Quoted 6 lines
drivers/crypto/ccp/sev-dev.c +2 -1
@@ -150,7 +151,11 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
 	sev = psp->sev_data;
 
-	if (data && WARN_ON_ONCE(is_vmalloc_addr(data)))
+	buf_len = sev_cmd_buffer_len(cmd);
+	if (WARN_ON_ONCE(!!data != !!buf_len))

Seems a bit confusing to me. Can this just be:

	if (WARN_ON_ONCE(data && !buf_len))

Or is this also trying to catch the case where buf_len is non-zero but data is NULL?

Thanks,
Tom

Quoted 25 lines
drivers/crypto/ccp/sev-dev.c +5 -2
+		return -EINVAL;
+
+	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
 		return -EINVAL;
 
 	/* Get the physical address of the command buffer */

@@ -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;
 }
author maintainer
#6 lore raw

Re: [PATCH 2/5] crypto: ccp: Reject SEV commands with mismatching command buffer

Quoted 17 lines · Tom Lendacky
Quoted 16 lines · Sean Christopherson
drivers/crypto/ccp/sev-dev.c +1 -0
index 6556d220713b..4c513318f16a 100644
@@ -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,7 +151,11 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
 	sev = psp->sev_data;
Quoted 7 lines

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 155

Quoted 3 lines
drivers/crypto/ccp/sev-dev.c +2 -1
-	if (data && WARN_ON_ONCE(is_vmalloc_addr(data)))
+	buf_len = sev_cmd_buffer_len(cmd);
+	if (WARN_ON_ONCE(!!data != !!buf_len))

Seems a bit confusing to me. Can this just be:

	if (WARN_ON_ONCE(data && !buf_len))

Or as Christophe pointed out, "!data != !buf_len".

Quoted 2 lines

Or is this also trying to catch the case where buf_len is non-zero but data is NULL?

Ya. It's not necessary to detect "buf_len && !data", but it doesn't incur additional cost. Is there a reason _not_ to disallow that?

maintainer
#7 lore raw

Re: [PATCH 2/5] crypto: ccp: Reject SEV commands with mismatching command buffer

Quoted 26 lines · Sean Christopherson
Quoted 17 lines · Tom Lendacky
Quoted 16 lines · Sean Christopherson
drivers/crypto/ccp/sev-dev.c +1 -0
index 6556d220713b..4c513318f16a 100644
@@ -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,7 +151,11 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
 	sev = psp->sev_data;
Quoted 7 lines

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 155

Quoted 3 lines
drivers/crypto/ccp/sev-dev.c +2 -1
-	if (data && WARN_ON_ONCE(is_vmalloc_addr(data)))
+	buf_len = sev_cmd_buffer_len(cmd);
+	if (WARN_ON_ONCE(!!data != !!buf_len))

Seems a bit confusing to me. Can this just be:

	if (WARN_ON_ONCE(data && !buf_len))
Quoted 7 lines

Or as Christophe pointed out, "!data != !buf_len".

Quoted 2 lines

Or is this also trying to catch the case where buf_len is non-zero but data is NULL?

Ya. It's not necessary to detect "buf_len && !data", but it doesn't incur additional cost. Is there a reason _not_ to disallow that?

Nope, no reason. I was just trying to process all the not signs :)

Thanks,
Tom

Quoted 1 line
author maintainer
#8 lore raw

PATCH 3/5 crypto: ccp: Play nice with vmalloc'd memory for SEV command structs

In reply to Sean Christopherson

Copy vmalloc'd data to an internal buffer instead of rejecting outright so that callers can put SEV command buffers on the stack without running afoul of CONFIG_VMAP_STACK=y. Currently, 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 a flexible array for the buffer, sized to hold the largest known
command. Alternatively, the buffer could be a union of all known
command structs, but that would incur a higher maintenance cost due to the need to update the union for every command in addition to updating the existing sev_cmd_buffer_len().

Align the buffer to an 8-byte boundary, mimicking the alignment that would be provided by the compiler if any of the structs were embedded directly. Note, sizeof() correctly incorporates this alignment.

Cc Brijesh Singh <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
 drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++++++++++++------
 drivers/crypto/ccp/sev-dev.h |  7 +++++++
 2 files changed, 34 insertions(+), 6 deletions(-)
drivers/crypto/ccp/sev-dev.c +27 -6
index 4c513318f16a..6d5882290cfc 100644
@@ -135,13 +135,14 @@ static int sev_cmd_buffer_len(int cmd)
 	return 0;
 }
 
-static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
+static int __sev_do_cmd_locked(int cmd, void *__data, int *psp_ret)
 {
 	struct psp_device *psp = psp_master;
 	struct sev_device *sev;
 	unsigned int phys_lsb, phys_msb;
 	unsigned int reg, ret = 0;
 	int buf_len;
+	void *data;
 
 	if (!psp || !psp->sev_data)
 		return -ENODEV;

@@ -152,11 +153,21 @@ 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))
+	if (WARN_ON_ONCE(!!__data != !!buf_len))
 		return -EINVAL;
 
-	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
-		return -EINVAL;
+	if (__data && is_vmalloc_addr(__data)) {
+		/*
+		 * If the incoming buffer is virtually allocated, copy it to
+		 * the driver's scratch buffer as __pa() will not work for such
+		 * addresses, vmalloc_to_page() is not guaranteed to succeed,
+		 * and vmalloc'd data may not be physically contiguous.
+		 */
+		data = sev->cmd_buf;
+		memcpy(data, __data, buf_len);
+	} else {
+		data = __data;
+	}
 
 	/* Get the physical address of the command buffer */
 	phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;

@@ -204,6 +215,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 && data != __data)
+		memcpy(__data, data, buf_len);
+
 	return ret;
 }
 

@@ -978,9 +996,12 @@ int sev_dev_init(struct psp_device *psp)
 {
 	struct device *dev = psp->dev;
 	struct sev_device *sev;
-	int ret = -ENOMEM;
+	int ret = -ENOMEM, cmd_buf_size = 0, i;
 
-	sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
+	for (i = 0; i < SEV_CMD_MAX; i++)
+		cmd_buf_size = max(cmd_buf_size, sev_cmd_buffer_len(i));
+
+	sev = devm_kzalloc(dev, sizeof(*sev) + cmd_buf_size, GFP_KERNEL);
 	if (!sev)
 		goto e_err;
drivers/crypto/ccp/sev-dev.h +7 -0
index dd5c4fe82914..b43283ce2d73 100644
@@ -52,6 +52,13 @@ struct sev_device {
 	u8 api_major;
 	u8 api_minor;
 	u8 build;
+
+	/*
+	 * Buffer used for incoming commands whose physical address cannot be
+	 * resolved via __pa(), e.g. stack pointers when CONFIG_VMAP_STACK=y.
+	 * Note, alignment isn't strictly required.
+	 */
+	u8 cmd_buf[] __aligned(8);
 };
 
 int sev_dev_init(struct psp_device *psp);
Signature · 1 line
2.31.0.208.g409f899ff0-goog
#9 lore raw

Re: [PATCH 3/5] crypto: ccp: Play nice with vmalloc'd memory for SEV command structs

Le 03/04/2021 à 01:37, Sean Christopherson a écrit :

Quoted 47 lines

Copy vmalloc'd data to an internal buffer instead of rejecting outright so that callers can put SEV command buffers on the stack without running afoul of CONFIG_VMAP_STACK=y. Currently, 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 a flexible array for the buffer, sized to hold the largest known
command. Alternatively, the buffer could be a union of all known
command structs, but that would incur a higher maintenance cost due to the need to update the union for every command in addition to updating the existing sev_cmd_buffer_len().

Align the buffer to an 8-byte boundary, mimicking the alignment that would be provided by the compiler if any of the structs were embedded directly. Note, sizeof() correctly incorporates this alignment.

Cc Brijesh Singh <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
  drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++++++++++++------
  drivers/crypto/ccp/sev-dev.h |  7 +++++++
  2 files changed, 34 insertions(+), 6 deletions(-)
drivers/crypto/ccp/sev-dev.c +2 -1
index 4c513318f16a..6d5882290cfc 100644
@@ -135,13 +135,14 @@ static int sev_cmd_buffer_len(int cmd)
  	return 0;
  }
 
-static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
+static int __sev_do_cmd_locked(int cmd, void *__data, int *psp_ret)
  {
  	struct psp_device *psp = psp_master;
  	struct sev_device *sev;
  	unsigned int phys_lsb, phys_msb;
  	unsigned int reg, ret = 0;
  	int buf_len;
+	void *data;
 
  	if (!psp || !psp->sev_data)
  		return -ENODEV;

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 159

Quoted 10 lines
drivers/crypto/ccp/sev-dev.c +2 -3
@@ -152,11 +153,21 @@ 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))
+	if (WARN_ON_ONCE(!!__data != !!buf_len))
  		return -EINVAL;
 
-	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
-		return -EINVAL;
+	if (__data && is_vmalloc_addr(__data)) {

I think you want to use !virt_addr_valid() here, because not only vmalloc addresses are a problem. For instance, module addresses are a problem as well.

Quoted 62 lines
drivers/crypto/ccp/sev-dev.c +23 -2
+		/*
+		 * If the incoming buffer is virtually allocated, copy it to
+		 * the driver's scratch buffer as __pa() will not work for such
+		 * addresses, vmalloc_to_page() is not guaranteed to succeed,
+		 * and vmalloc'd data may not be physically contiguous.
+		 */
+		data = sev->cmd_buf;
+		memcpy(data, __data, buf_len);
+	} else {
+		data = __data;
+	}
 
  	/* Get the physical address of the command buffer */
  	phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;

@@ -204,6 +215,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 && data != __data)
+		memcpy(__data, data, buf_len);
+
  	return ret;
  }
 

@@ -978,9 +996,12 @@ int sev_dev_init(struct psp_device *psp)
  {
  	struct device *dev = psp->dev;
  	struct sev_device *sev;
-	int ret = -ENOMEM;
+	int ret = -ENOMEM, cmd_buf_size = 0, i;
 
-	sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
+	for (i = 0; i < SEV_CMD_MAX; i++)
+		cmd_buf_size = max(cmd_buf_size, sev_cmd_buffer_len(i));
+
+	sev = devm_kzalloc(dev, sizeof(*sev) + cmd_buf_size, GFP_KERNEL);
  	if (!sev)
  		goto e_err;
drivers/crypto/ccp/sev-dev.h +7 -0
index dd5c4fe82914..b43283ce2d73 100644
@@ -52,6 +52,13 @@ struct sev_device {
  	u8 api_major;
  	u8 api_minor;
  	u8 build;
+
+	/*
+	 * Buffer used for incoming commands whose physical address cannot be
+	 * resolved via __pa(), e.g. stack pointers when CONFIG_VMAP_STACK=y.
+	 * Note, alignment isn't strictly required.
+	 */
+	u8 cmd_buf[] __aligned(8);
  };
 
  int sev_dev_init(struct psp_device *psp);
#10 lore raw

Re: [PATCH 3/5] crypto: ccp: Play nice with vmalloc'd memory for SEV command structs

In reply to Sean Christopherson

Le 03/04/2021 à 01:37, Sean Christopherson a écrit :

Quoted 47 lines

Copy vmalloc'd data to an internal buffer instead of rejecting outright so that callers can put SEV command buffers on the stack without running afoul of CONFIG_VMAP_STACK=y. Currently, 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 a flexible array for the buffer, sized to hold the largest known
command. Alternatively, the buffer could be a union of all known
command structs, but that would incur a higher maintenance cost due to the need to update the union for every command in addition to updating the existing sev_cmd_buffer_len().

Align the buffer to an 8-byte boundary, mimicking the alignment that would be provided by the compiler if any of the structs were embedded directly. Note, sizeof() correctly incorporates this alignment.

Cc Brijesh Singh <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
  drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++++++++++++------
  drivers/crypto/ccp/sev-dev.h |  7 +++++++
  2 files changed, 34 insertions(+), 6 deletions(-)
drivers/crypto/ccp/sev-dev.c +2 -1
index 4c513318f16a..6d5882290cfc 100644
@@ -135,13 +135,14 @@ static int sev_cmd_buffer_len(int cmd)
  	return 0;
  }
 
-static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
+static int __sev_do_cmd_locked(int cmd, void *__data, int *psp_ret)
  {
  	struct psp_device *psp = psp_master;
  	struct sev_device *sev;
  	unsigned int phys_lsb, phys_msb;
  	unsigned int reg, ret = 0;
  	int buf_len;
+	void *data;
 
  	if (!psp || !psp->sev_data)
  		return -ENODEV;

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 156

Quoted 5 lines
drivers/crypto/ccp/sev-dev.c +1 -1
@@ -152,11 +153,21 @@ 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))
+	if (WARN_ON_ONCE(!!__data != !!buf_len))

Why do you need a double !! ?
I think !__data != !buf_len should be enough.

Quoted 20 lines
drivers/crypto/ccp/sev-dev.c +12 -2
  		return -EINVAL;
 
-	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
-		return -EINVAL;
+	if (__data && is_vmalloc_addr(__data)) {
+		/*
+		 * If the incoming buffer is virtually allocated, copy it to
+		 * the driver's scratch buffer as __pa() will not work for such
+		 * addresses, vmalloc_to_page() is not guaranteed to succeed,
+		 * and vmalloc'd data may not be physically contiguous.
+		 */
+		data = sev->cmd_buf;
+		memcpy(data, __data, buf_len);
+	} else {
+		data = __data;
+	}
 
  	/* Get the physical address of the command buffer */
  	phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 222

Quoted 8 lines
drivers/crypto/ccp/sev-dev.c +5 -0
@@ -204,6 +215,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 && data != __data)

IIUC, when __data is NULL, data is also NULL, so this double test is useless.

Checking data != __data should be enough

Quoted 39 lines
drivers/crypto/ccp/sev-dev.c +7 -2
+		memcpy(__data, data, buf_len);
+
  	return ret;
  }
 

@@ -978,9 +996,12 @@ int sev_dev_init(struct psp_device *psp)
  {
  	struct device *dev = psp->dev;
  	struct sev_device *sev;
-	int ret = -ENOMEM;
+	int ret = -ENOMEM, cmd_buf_size = 0, i;
 
-	sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
+	for (i = 0; i < SEV_CMD_MAX; i++)
+		cmd_buf_size = max(cmd_buf_size, sev_cmd_buffer_len(i));
+
+	sev = devm_kzalloc(dev, sizeof(*sev) + cmd_buf_size, GFP_KERNEL);
  	if (!sev)
  		goto e_err;
drivers/crypto/ccp/sev-dev.h +7 -0
index dd5c4fe82914..b43283ce2d73 100644
@@ -52,6 +52,13 @@ struct sev_device {
  	u8 api_major;
  	u8 api_minor;
  	u8 build;
+
+	/*
+	 * Buffer used for incoming commands whose physical address cannot be
+	 * resolved via __pa(), e.g. stack pointers when CONFIG_VMAP_STACK=y.
+	 * Note, alignment isn't strictly required.
+	 */
+	u8 cmd_buf[] __aligned(8);
  };
 
  int sev_dev_init(struct psp_device *psp);
#11 lore raw

Re: [PATCH 3/5] crypto: ccp: Play nice with vmalloc'd memory for SEV command structs

In reply to Sean Christopherson

Le 03/04/2021 à 01:37, Sean Christopherson a écrit :

Quoted 58 lines

Copy vmalloc'd data to an internal buffer instead of rejecting outright so that callers can put SEV command buffers on the stack without running afoul of CONFIG_VMAP_STACK=y. Currently, 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 a flexible array for the buffer, sized to hold the largest known
command. Alternatively, the buffer could be a union of all known
command structs, but that would incur a higher maintenance cost due to the need to update the union for every command in addition to updating the existing sev_cmd_buffer_len().

Align the buffer to an 8-byte boundary, mimicking the alignment that would be provided by the compiler if any of the structs were embedded directly. Note, sizeof() correctly incorporates this alignment.

Cc Brijesh Singh <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Tom Lendacky <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
  drivers/crypto/ccp/sev-dev.c | 33 +++++++++++++++++++++++++++------
  drivers/crypto/ccp/sev-dev.h |  7 +++++++
  2 files changed, 34 insertions(+), 6 deletions(-)
drivers/crypto/ccp/sev-dev.c +5 -4
index 4c513318f16a..6d5882290cfc 100644
@@ -135,13 +135,14 @@ static int sev_cmd_buffer_len(int cmd)
  	return 0;
  }
 
-static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
+static int __sev_do_cmd_locked(int cmd, void *__data, int *psp_ret)
  {
  	struct psp_device *psp = psp_master;
  	struct sev_device *sev;
  	unsigned int phys_lsb, phys_msb;
  	unsigned int reg, ret = 0;
  	int buf_len;
+	void *data;
 
  	if (!psp || !psp->sev_data)
  		return -ENODEV;

@@ -152,11 +153,21 @@ 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))
+	if (WARN_ON_ONCE(!!__data != !!buf_len))
  		return -EINVAL;
 
-	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
-		return -EINVAL;
+	if (__data && is_vmalloc_addr(__data)) {
+		/*

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 170

Quoted 10 lines
drivers/crypto/ccp/sev-dev.c +10 -0
+		 * If the incoming buffer is virtually allocated, copy it to
+		 * the driver's scratch buffer as __pa() will not work for such
+		 * addresses, vmalloc_to_page() is not guaranteed to succeed,
+		 * and vmalloc'd data may not be physically contiguous.
+		 */
+		data = sev->cmd_buf;
+		memcpy(data, __data, buf_len);
+	} else {
+		data = __data;
+	}

I don't know how big commands are, but if they are small, it would probably be more efficient to inconditionnally copy them to the buffer rather then doing the test.

Quoted 51 lines
drivers/crypto/ccp/sev-dev.c +12 -2
  	/* Get the physical address of the command buffer */
  	phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;

@@ -204,6 +215,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 && data != __data)
+		memcpy(__data, data, buf_len);
+
  	return ret;
  }
 

@@ -978,9 +996,12 @@ int sev_dev_init(struct psp_device *psp)
  {
  	struct device *dev = psp->dev;
  	struct sev_device *sev;
-	int ret = -ENOMEM;
+	int ret = -ENOMEM, cmd_buf_size = 0, i;
 
-	sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
+	for (i = 0; i < SEV_CMD_MAX; i++)
+		cmd_buf_size = max(cmd_buf_size, sev_cmd_buffer_len(i));
+
+	sev = devm_kzalloc(dev, sizeof(*sev) + cmd_buf_size, GFP_KERNEL);
  	if (!sev)
  		goto e_err;
drivers/crypto/ccp/sev-dev.h +7 -0
index dd5c4fe82914..b43283ce2d73 100644
@@ -52,6 +52,13 @@ struct sev_device {
  	u8 api_major;
  	u8 api_minor;
  	u8 build;
+
+	/*
+	 * Buffer used for incoming commands whose physical address cannot be
+	 * resolved via __pa(), e.g. stack pointers when CONFIG_VMAP_STACK=y.
+	 * Note, alignment isn't strictly required.
+	 */
+	u8 cmd_buf[] __aligned(8);
  };
 
  int sev_dev_init(struct psp_device *psp);
author maintainer
#12 lore raw

Re: [PATCH 3/5] crypto: ccp: Play nice with vmalloc'd memory for SEV command structs

Quoted 18 lines · Christophe Leroy

Le 03/04/2021 à 01:37, Sean Christopherson a écrit :

Quoted 16 lines
drivers/crypto/ccp/sev-dev.c +9 -3
@@ -152,11 +153,21 @@ 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))
+	if (WARN_ON_ONCE(!!__data != !!buf_len))
  		return -EINVAL;
-	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
-		return -EINVAL;
+	if (__data && is_vmalloc_addr(__data)) {
+		/*
+		 * If the incoming buffer is virtually allocated, copy it to
+		 * the driver's scratch buffer as __pa() will not work for such
+		 * addresses, vmalloc_to_page() is not guaranteed to succeed,
+		 * and vmalloc'd data may not be physically contiguous.
+		 */
+		data = sev->cmd_buf;
Quoted 8 lines

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 170

Quoted 4 lines
drivers/crypto/ccp/sev-dev.c +4 -0
+		memcpy(data, __data, buf_len);
+	} else {
+		data = __data;
+	}

I don't know how big commands are, but if they are small, it would probably be more efficient to inconditionnally copy them to the buffer rather then doing the test.

Brijesh, I assume SNP support will need to copy the commands unconditionally? If yes, it probably makes sense to do so now and avoid vmalloc dependencies completely. And I think that would allow for the removal of status_cmd_buf and init_cmd_buf, or is there another reason those dedicated buffers exist?

#13 lore raw

Re: [PATCH 3/5] crypto: ccp: Play nice with vmalloc'd memory for SEV command structs

Quoted 25 lines · Sean Christopherson
Quoted 16 lines · Christophe Leroy

Le 03/04/2021 à 01:37, Sean Christopherson a écrit :

Quoted 15 lines
drivers/crypto/ccp/sev-dev.c +8 -3
@@ -152,11 +153,21 @@ 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))
+	if (WARN_ON_ONCE(!!__data != !!buf_len))
  		return -EINVAL;
-	if (WARN_ON_ONCE(data && is_vmalloc_addr(data)))
-		return -EINVAL;
+	if (__data && is_vmalloc_addr(__data)) {
+		/*
+		 * If the incoming buffer is virtually allocated, copy it to
+		 * the driver's scratch buffer as __pa() will not work for such
+		 * addresses, vmalloc_to_page() is not guaranteed to succeed,
+		 * and vmalloc'd data may not be physically contiguous.
+		 */
Quoted 8 lines

drivers/crypto/ccp/sev-dev.c static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret) · line 170

Quoted 5 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+		data = sev->cmd_buf;
+		memcpy(data, __data, buf_len);
+	} else {
+		data = __data;
+	}

I don't know how big commands are, but if they are small, it would probably be more efficient to inconditionnally copy them to the buffer rather then doing the test.

Quoted 4 lines

Brijesh, I assume SNP support will need to copy the commands unconditionally? If yes, it probably makes sense to do so now and avoid vmalloc dependencies completely. And I think that would allow for the removal of status_cmd_buf and init_cmd_buf, or is there another reason those dedicated buffers exist?

Yes, we need to copy the commands unconditionally for the SNP support. It makes sense to avoid the vmalloc dependencies. I can't think of any reason why we would need the status_cmd_buf and init_cmd_buf after those changes.

author maintainer
#14 lore raw

PATCH 4/5 crypto: ccp: Use the stack for small SEV command buffers

In reply to 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 <[email protected]>
---
 drivers/crypto/ccp/sev-dev.c | 122 ++++++++++++++---------------------
 1 file changed, 47 insertions(+), 75 deletions(-)
drivers/crypto/ccp/sev-dev.c +47 -75
index 6d5882290cfc..6a380d483fce 100644
@@ -402,7 +402,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;

@@ -413,29 +413,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) {

@@ -444,10 +439,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;

@@ -461,8 +456,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;
 }
 

@@ -594,7 +587,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;
 

@@ -604,19 +597,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);

@@ -625,8 +613,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) {

@@ -635,21 +623,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;

@@ -663,28 +649,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;

@@ -692,7 +675,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;
 		}

@@ -700,7 +683,6 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 
 e_free:
 	kfree(id_blob);
-	kfree(data);
 
 	return ret;
 }

@@ -750,7 +732,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;

@@ -768,9 +750,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 ||

@@ -782,25 +762,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) {

@@ -808,15 +782,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;

@@ -841,8 +815,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;
 }
Signature · 1 line
2.31.0.208.g409f899ff0-goog
author maintainer
#15 lore raw

PATCH 5/5 KVM: SVM: Allocate SEV command structures on local stack

In reply to 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 <[email protected]>
Cc Tom Lendacky <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
 arch/x86/kvm/svm/sev.c | 262 +++++++++++++++--------------------------
 1 file changed, 96 insertions(+), 166 deletions(-)
arch/x86/kvm/svm/sev.c +96 -166
index 5457138c7347..316fd39c7aef 100644
@@ -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;
 }
Signature · 1 line
2.31.0.208.g409f899ff0-goog
#16 lore raw
In reply to Sean Christopherson

Hi Sean,

Quoted 10 lines · Sean Christopherson

While doing minor KVM cleanup to account various kernel allocations, I noticed that all of the SEV command buffers are allocated via kmalloc(), even for commands whose payloads is smaller than a pointer. After much head scratching, the only reason I could come up with for dynamically allocating the command data is CONFIG_VMAP_STACK=y.

This series teaches __sev_do_cmd_locked() to gracefully handle vmalloc'd command buffers by copying such buffers 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.

Thanks for the series. Post SNP series, I was going to move all the command buffer allocation to the stack. You are ahead of me :). I can certainly build upon your series.

The behavior of the SEV-legacy command is changed when SNP firmware is in the INIT state. All the legacy commands that cause a firmware to write to memory must be in the firmware state before issuing the command. One of my patch in the SNP series is using an internal memory before sending the command to the PSP.

Looking forward to the SNP support, may I ask you to remove the vmalloc'd buffer check and use a page for the internal buffer ? In SNP series, I can simply transition the internal page to firmware state before issuing the command.

Quoted 22 lines

The first patch is optional, I included it in case someone wants to backport it to stable kernels. It wouldn't actually fix bugs, but it would make debugging issues a lot easier if they did pop up.

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

Based on kvm/queue, commit f96be2deac9b ("KVM: x86: Support KVM VMs sharing SEV context") to avoid a minor conflict.

Sean Christopherson (5):
crypto: ccp: Detect and reject vmalloc 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
KVM: SVM: Allocate SEV command structures on local stack

 arch/x86/kvm/svm/sev.c       | 262 +++++++++++++----------------------
 drivers/crypto/ccp/sev-dev.c | 161 ++++++++++-----------
 drivers/crypto/ccp/sev-dev.h |   7 +
 3 files changed, 184 insertions(+), 246 deletions(-)