On Thu, Oct 19, 2017 at 09:33:53PM -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 <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 69 
> ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)

Improvements-by: Borislav Petkov <b...@suse.de>

> +static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
> +{
> +     struct sev_user_data_pek_csr input;
> +     struct sev_data_pek_csr *data;
> +     void *blob = NULL;
> +     int ret, err;
> +
> +     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)
> +             goto cmd;
> +
> +     /* 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;
> +     }
> +
> +     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_init(NULL, &argp->error);
> +     if (ret)
> +             goto e_free_blob;
> +
> +     ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->error);

This is the same issue: nothing is handling the case where sev_do_cmd()
here fails. If it doesn't matter, then you should simply do:

        sev_do_cmd(SEV_CMD_PEK_CSR, data, &argp->error);

along with explaining why we don't care about the command failing. But
simply writing into ret to overwrite it later, looks strange.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Reply via email to