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;
>
