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 


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;

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