Re: [PATCH v4 3/3] x86/tdx: Add Quote generation support

Wander Lairson Costa
2022-04-28T17:59:01Z

On Fri, Apr 22, 2022 at 04:34:18PM -0700, Kuppuswamy Sathyanarayanan wrote:

[snip]

> +static long tdx_get_tdquote(void __user *argp)
> +{
> + struct tdx_quote_hdr *quote_hdr;
> + struct tdx_quote_req quote_req;
> + void *quote_buf = NULL;
> + dma_addr_t handle;
> + long ret = 0, err;
> + u64 quote_buf_len;
> +
> + mutex_lock(&quote_lock);
> +
> + reinit_completion(&req_compl);
> +
> + /* Copy Quote request struct from user buffer */
> + if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
> + return -EFAULT;
> +
> + /* Make sure the length & timeout is valid */
> + if (quote_req.len <= 0 || quote_req.timeout <= 0)

len and timeout are unsigned values, so they will never be negative.

> + return -EINVAL;
> +
> + /* Align with page size to meet 4K alignment */
> + quote_buf_len = PAGE_ALIGN(quote_req.len);
> +
> + /*
> + * Allocate DMA buffer to get TDQUOTE data from the VMM.
> + * dma_alloc_coherent() API internally marks allocated
> + * memory as shared with VMM. So explicit shared mapping is
> + * not required.
> + */
> + quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
> + GFP_KERNEL | __GFP_ZERO);
> + if (!quote_buf) {
> + ret = -ENOMEM;
> + goto quote_failed;
> + }
> +
> + /* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
> + if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
> + ret = -EFAULT;
> + goto quote_failed;
> + }
> +
> + /* Submit GetQuote Request */
> + err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
> + if (err) {
> + /* if failed, copy hypercall error code to user buffer */
> + ret = put_user(err, (long __user *)argp);

The assigment to ret is unused.

> + ret = -EIO;
> + goto quote_failed;
> + }
> +
> + /* Wait for attestation completion */
> + ret = wait_for_completion_interruptible_timeout(
> + &req_compl,
> + msecs_to_jiffies(quote_req.timeout));
> + if (ret <= 0) {
> + ret = -EIO;
> + goto quote_failed;
> + }
> +
> + /* Copy generated Quote data back to user buffer */
> + if (copy_to_user((void __user *)quote_req.buf, quote_buf, quote_buf_len)) {

Shouldn't we use quote_req.len instead of quote_buf_len here?

> + ret = -EFAULT;
> + goto quote_failed;
> + }
> +
> + quote_hdr = (struct tdx_quote_hdr *)quote_buf;
> +
> + /* Make sure quote generation is successful */
> + if (!quote_hdr->status)
> + ret = 0;
> + else
> + ret = -EIO;
> +

Shouldn't copy_to_user be called after checking the status?

> +quote_failed:
> + if (quote_buf)
> + dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);
> +
> + mutex_unlock(&quote_lock);
> +
> + return ret;
> +}
> +
> +static void attestation_callback_handler(void)
> +{
> + complete(&req_compl);
> +}
> +
> static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> case TDX_CMD_GET_TDREPORT:
> ret = tdx_get_tdreport(argp);
> break;
> + case TDX_CMD_GEN_QUOTE:
> + ret = tdx_get_tdquote(argp);
> + break;
> default:
> pr_err("cmd %d not supported\n", cmd);
> break;
> @@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
> .llseek = no_llseek,
> };
>
> +/* Helper function to cleanup attestation related allocations */
> +static void _tdx_attest_remove(void)
> +{
> + misc_deregister(&miscdev);

Won't misc_deregister be called even if misc_register fails?

> +
> + tdx_remove_ev_notify_handler();
> +}
> +