Messages in this thread | | | Date | Thu, 28 Apr 2022 14:58:42 -0300 | From | Wander Lairson Costa <> | Subject | Re: [PATCH v4 3/3] x86/tdx: Add Quote generation support |
| |
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("e_lock); > + > + reinit_completion(&req_compl); > + > + /* Copy Quote request struct from user buffer */ > + if (copy_from_user("e_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("e_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(); > +} > +
| |