Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

2017-10-13 Thread Borislav Petkov 
On Thu, Oct 12, 2017 at 11:13:44PM -0500, Brijesh Singh wrote:
> As per the spec, its perfectly legal to pass input.address = 0x0 and
> input.length = 0x0. If userspace wants to query CSR length then it will
> fill all the fields with. In response, FW will return error
> (LENGTH_TO_SMALL) and input.length will be filled with the expected
> length. Several command work very similar (e.g PEK_CSR,
> PEK_CERT_EXPORT). A typical usage from userspace will be:
> 
> - query the length of the blob (call command with all fields set to zero)
> - SEV FW will response with expected length
> - userspace allocate the blob and retries the command. 

Ok, let's make that a clearer and more precise by explicitly checking
the query case:

+   /* Userspace wants to query CSR length */
+   if (!input.address && !input.length)
+   goto cmd;

and commenting why we're doing this.

The rest is cleanups.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e3ee68afd068..e10f507f9a60 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -299,36 +299,37 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
struct sev_user_data_pek_csr input;
struct sev_data_pek_csr *data;
int do_shutdown = 0;
+   void *blob = NULL;
int ret, state;
-   void *blob;
 
-   if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
-  sizeof(struct sev_user_data_pek_csr)))
+   if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
 
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
 
-   /* allocate a temporary physical contigous buffer to store the CSR blob 
*/
-   blob = NULL;
-   if (input.address) {
-   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
-   input.length > SEV_FW_BLOB_MAX_SIZE) {
-   ret = -EFAULT;
-   goto e_free;
-   }
+   /* Userspace wants to query CSR length */
+   if (!input.address && !input.length)
+   goto cmd;
 
-   blob = kmalloc(input.length, GFP_KERNEL);
-   if (!blob) {
-   ret = -ENOMEM;
-   goto e_free;
-   }
+   /* allocate a physically contiguous buffer to store the CSR blob */
+   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+   input.length > SEV_FW_BLOB_MAX_SIZE) {
+   ret = -EFAULT;
+   goto e_free;
+   }
 
-   data->address = __psp_pa(blob);
-   data->len = input.length;
+   blob = kmalloc(input.length, GFP_KERNEL);
+   if (!blob) {
+   ret = -ENOMEM;
+   goto e_free;
}
 
+   data->address = __psp_pa(blob);
+   data->len = input.length;
+
+cmd:
ret = sev_platform_get_state(, >error);
if (ret)
goto e_free_blob;
@@ -349,25 +350,26 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
 
-   ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
+   ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
 
+   /*
+* If we query the CSR length, FW responded with the expected length.
+*/
input.length = data->len;
 
-   /* copy blob to userspace */
-   if (blob &&
-   copy_to_user((void __user *)(uintptr_t)input.address,
-   blob, input.length)) {
-   ret = -EFAULT;
-   goto e_shutdown;
+   if (blob) {
+   if (copy_to_user((void __user *)input.address, blob, 
input.length)) {
+   ret = -EFAULT;
+   goto e_shutdown;
+   }
}
 
-   if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
-sizeof(struct sev_user_data_pek_csr)))
+   if (copy_to_user((void __user *)argp->data, , sizeof(input)))
ret = -EFAULT;
 
 e_shutdown:
if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
 e_free_blob:
kfree(blob);
 e_free:

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
--

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

2017-10-13 Thread Borislav Petkov 
On Thu, Oct 12, 2017 at 09:24:01PM -0500, Brijesh Singh wrote:
> I assume you mean performing the SEV state check before allocating the
> memory for the CSR blob, right ?

I mean, do those first:

if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
return -EFAULT;

if (!input.address)
return -EINVAL;

/* allocate a physically contiguous buffer to store the CSR blob */
if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
input.length > SEV_FW_BLOB_MAX_SIZE)
return -EFAULT;


Because if you allocate the memory first and some of those checks fail,
you allocate in vain to free it immediately after. And you can save
yourself all that.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
--

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

2017-10-12 Thread Brijesh Singh 


On 10/12/17 9:24 PM, Brijesh Singh wrote:
>
> On 10/12/17 2:53 PM, Borislav Petkov wrote:
> ...
>
>> Ok, a couple of things here:
>>
>> * Move the checks first and the allocations second so that you allocate
>> memory only after all checks have been passed and you don't allocate
>> pointlessly.
>
> I assume you mean performing the SEV state check before allocating the
> memory for the CSR blob, right ? In my patches, I typically perform all
> the SW specific checks and allocation before invoking the HW routines.
> Handling the PSP commands will take longer compare to kmalloc() or
> access_ok() etc. If its not a big deal then I would prefer to keep that
> way.
>
>
>> * That:
>>
>> if (state == SEV_STATE_WORKING) {
>> ret = -EBUSY;
>> goto e_free_blob;
>> } else if (state == SEV_STATE_UNINIT) {
>> ret = sev_firmware_init(>error);
>> if (ret)
>> goto e_free_blob;
>> do_shutdown = 1;
>> }
>>
>> is a repeating pattern. Perhaps it should be called
>> sev_firmware_reinit() and called by other functions.
>
>> * The rest is simplifications and streamlining.
>>
>> ---
>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>> index e3ee68afd068..d41f5448a25b 100644
>> --- a/drivers/crypto/ccp/psp-dev.c
>> +++ b/drivers/crypto/ccp/psp-dev.c
>> @@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd 
>> *argp)
>>  int ret, state;
>>  void *blob;
>>  
>> -if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
>> -   sizeof(struct sev_user_data_pek_csr)))
>> +if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
>> +return -EFAULT;
>> +
>> +if (!input.address)
>> +return -EINVAL;
>> +


As per the spec, its perfectly legal to pass input.address = 0x0 and
input.length = 0x0. If userspace wants to query CSR length then it will
fill all the fields with. In response, FW will return error
(LENGTH_TO_SMALL) and input.length will be filled with the expected
length. Several command work very similar (e.g PEK_CSR,
PEK_CERT_EXPORT). A typical usage from userspace will be:

- query the length of the blob (call command with all fields set to zero)
- SEV FW will response with expected length
- userspace allocate the blob and retries the command. 


>> +/* allocate a physically contiguous buffer to store the CSR blob */
>> +if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
>> +input.length > SEV_FW_BLOB_MAX_SIZE)
>>  return -EFAULT;
>>  
>>  data = kzalloc(sizeof(*data), GFP_KERNEL);
>>  if (!data)
>>  return -ENOMEM;
>>  
>> -/* allocate a temporary physical contigous buffer to store the CSR blob 
>> */
>> -blob = NULL;
>> -if (input.address) {
>> -if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
>> -input.length > SEV_FW_BLOB_MAX_SIZE) {
>> -ret = -EFAULT;
>> -goto e_free;
>> -}
>> -
>> -blob = kmalloc(input.length, GFP_KERNEL);
>> -if (!blob) {
>> -ret = -ENOMEM;
>> -goto e_free;
>> -}
>> -
>> -data->address = __psp_pa(blob);
>> -data->len = input.length;
>> +blob = kmalloc(input.length, GFP_KERNEL);
>> +if (!blob) {
>> +ret = -ENOMEM;
>> +goto e_free;
>>  }
>>  
>> +data->address = __psp_pa(blob);
>> +data->len = input.length;
>> +
>>  ret = sev_platform_get_state(, >error);
>>  if (ret)
>>  goto e_free_blob;
>> @@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd 
>> *argp)
>>  do_shutdown = 1;
>>  }
>>  
>> -ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
>> +ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
>>  
>>  input.length = data->len;
>>  
>>  /* copy blob to userspace */
>> -if (blob &&
>> -copy_to_user((void __user *)(uintptr_t)input.address,
>> -blob, input.length)) {
>> +if (copy_to_user((void __user *)input.address, blob, input.length)) {
>>  ret = -EFAULT;
>>  goto e_shutdown;
>>  }
>>  
>> -if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
>> - sizeof(struct sev_user_data_pek_csr)))
>> +if (copy_to_user((void __user *)argp->data, , sizeof(input)))
>>  ret = -EFAULT;
>>  
>>  e_shutdown:
>>  if (do_shutdown)
>> -sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>> +ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>> +
>>  e_free_blob:
>>  kfree(blob);
>>  e_free:
>> @@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int 
>> ioctl, unsigned long arg)
>>  ret = sev_ioctl_pdh_gen();
>>  break;
>>  
>> -case SEV_PEK_CSR: {
>>

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

2017-10-12 Thread Brijesh Singh 


On 10/12/17 2:53 PM, Borislav Petkov wrote:
...

> Ok, a couple of things here:
>
> * Move the checks first and the allocations second so that you allocate
> memory only after all checks have been passed and you don't allocate
> pointlessly.


I assume you mean performing the SEV state check before allocating the
memory for the CSR blob, right ? In my patches, I typically perform all
the SW specific checks and allocation before invoking the HW routines.
Handling the PSP commands will take longer compare to kmalloc() or
access_ok() etc. If its not a big deal then I would prefer to keep that
way.


>
> * That:
>
> if (state == SEV_STATE_WORKING) {
> ret = -EBUSY;
> goto e_free_blob;
> } else if (state == SEV_STATE_UNINIT) {
> ret = sev_firmware_init(>error);
> if (ret)
> goto e_free_blob;
> do_shutdown = 1;
> }
>
> is a repeating pattern. Perhaps it should be called
> sev_firmware_reinit() and called by other functions.


> * The rest is simplifications and streamlining.
>
> ---
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index e3ee68afd068..d41f5448a25b 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
>   int ret, state;
>   void *blob;
>  
> - if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
> -sizeof(struct sev_user_data_pek_csr)))
> + if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
> + return -EFAULT;
> +
> + if (!input.address)
> + return -EINVAL;
> +
> + /* allocate a physically contiguous buffer to store the CSR blob */
> + if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
> + input.length > SEV_FW_BLOB_MAX_SIZE)
>   return -EFAULT;
>  
>   data = kzalloc(sizeof(*data), GFP_KERNEL);
>   if (!data)
>   return -ENOMEM;
>  
> - /* allocate a temporary physical contigous buffer to store the CSR blob 
> */
> - blob = NULL;
> - if (input.address) {
> - if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
> - input.length > SEV_FW_BLOB_MAX_SIZE) {
> - ret = -EFAULT;
> - goto e_free;
> - }
> -
> - blob = kmalloc(input.length, GFP_KERNEL);
> - if (!blob) {
> - ret = -ENOMEM;
> - goto e_free;
> - }
> -
> - data->address = __psp_pa(blob);
> - data->len = input.length;
> + blob = kmalloc(input.length, GFP_KERNEL);
> + if (!blob) {
> + ret = -ENOMEM;
> + goto e_free;
>   }
>  
> + data->address = __psp_pa(blob);
> + data->len = input.length;
> +
>   ret = sev_platform_get_state(, >error);
>   if (ret)
>   goto e_free_blob;
> @@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
>   do_shutdown = 1;
>   }
>  
> - ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
> + ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
>  
>   input.length = data->len;
>  
>   /* copy blob to userspace */
> - if (blob &&
> - copy_to_user((void __user *)(uintptr_t)input.address,
> - blob, input.length)) {
> + if (copy_to_user((void __user *)input.address, blob, input.length)) {
>   ret = -EFAULT;
>   goto e_shutdown;
>   }
>  
> - if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
> -  sizeof(struct sev_user_data_pek_csr)))
> + if (copy_to_user((void __user *)argp->data, , sizeof(input)))
>   ret = -EFAULT;
>  
>  e_shutdown:
>   if (do_shutdown)
> - sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> + ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> +
>  e_free_blob:
>   kfree(blob);
>  e_free:
> @@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int 
> ioctl, unsigned long arg)
>   ret = sev_ioctl_pdh_gen();
>   break;
>  
> - case SEV_PEK_CSR: {
> + case SEV_PEK_CSR:
>   ret = sev_ioctl_pek_csr();
>   break;
> - }
> +
>   default:
>   ret = -EINVAL;
>   goto out;
>

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

2017-10-12 Thread Borislav Petkov 
On Fri, Oct 06, 2017 at 08:06:05PM -0500, Brijesh Singh wrote:
> The SEV_PEK_CSR command can be used to generate a PEK certificate
> signing request. The command is defined in SEV spec section 5.7.
> 
> Cc: Paolo Bonzini 
> Cc: "Radim Krčmář" 
> Cc: Borislav Petkov 
> Cc: Herbert Xu 
> Cc: Gary Hook 
> Cc: Tom Lendacky 
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh 
> ---
>  drivers/crypto/ccp/psp-dev.c | 85 
> 
>  1 file changed, 85 insertions(+)

Ok, a couple of things here:

* Move the checks first and the allocations second so that you allocate
memory only after all checks have been passed and you don't allocate
pointlessly.

* That:

if (state == SEV_STATE_WORKING) {
ret = -EBUSY;
goto e_free_blob;
} else if (state == SEV_STATE_UNINIT) {
ret = sev_firmware_init(>error);
if (ret)
goto e_free_blob;
do_shutdown = 1;
}

is a repeating pattern. Perhaps it should be called
sev_firmware_reinit() and called by other functions.

* The rest is simplifications and streamlining.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e3ee68afd068..d41f5448a25b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
int ret, state;
void *blob;
 
-   if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
-  sizeof(struct sev_user_data_pek_csr)))
+   if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
+   return -EFAULT;
+
+   if (!input.address)
+   return -EINVAL;
+
+   /* allocate a physically contiguous buffer to store the CSR blob */
+   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+   input.length > SEV_FW_BLOB_MAX_SIZE)
return -EFAULT;
 
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
 
-   /* allocate a temporary physical contigous buffer to store the CSR blob 
*/
-   blob = NULL;
-   if (input.address) {
-   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
-   input.length > SEV_FW_BLOB_MAX_SIZE) {
-   ret = -EFAULT;
-   goto e_free;
-   }
-
-   blob = kmalloc(input.length, GFP_KERNEL);
-   if (!blob) {
-   ret = -ENOMEM;
-   goto e_free;
-   }
-
-   data->address = __psp_pa(blob);
-   data->len = input.length;
+   blob = kmalloc(input.length, GFP_KERNEL);
+   if (!blob) {
+   ret = -ENOMEM;
+   goto e_free;
}
 
+   data->address = __psp_pa(blob);
+   data->len = input.length;
+
ret = sev_platform_get_state(, >error);
if (ret)
goto e_free_blob;
@@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
 
-   ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
+   ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
 
input.length = data->len;
 
/* copy blob to userspace */
-   if (blob &&
-   copy_to_user((void __user *)(uintptr_t)input.address,
-   blob, input.length)) {
+   if (copy_to_user((void __user *)input.address, blob, input.length)) {
ret = -EFAULT;
goto e_shutdown;
}
 
-   if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
-sizeof(struct sev_user_data_pek_csr)))
+   if (copy_to_user((void __user *)argp->data, , sizeof(input)))
ret = -EFAULT;
 
 e_shutdown:
if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+
 e_free_blob:
kfree(blob);
 e_free:
@@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
ret = sev_ioctl_pdh_gen();
break;
 
-   case SEV_PEK_CSR: {
+   case SEV_PEK_CSR:
ret = sev_ioctl_pek_csr();
break;
-   }
+
default:
ret = -EINVAL;
goto out;

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
--