lkml.org 
[lkml]   [2017]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command
From
Date


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(&argp->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(&input, (void __user *)(uintptr_t)argp->data,
> - sizeof(struct sev_user_data_pek_csr)))
> + if (copy_from_user(&input, (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(&state, &argp->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, &argp->error);
> + ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->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, &input,
> - sizeof(struct sev_user_data_pek_csr)))
> + if (copy_to_user((void __user *)argp->data, &input, 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(&input);
> break;
>
> - case SEV_PEK_CSR: {
> + case SEV_PEK_CSR:
> ret = sev_ioctl_pek_csr(&input);
> break;
> - }
> +
> default:
> ret = -EINVAL;
> goto out;
>

\
 
 \ /
  Last update: 2017-10-13 04:25    [W:0.166 / U:19.316 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site