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

[Wander Lairson Costa <wander@redhat.com>]

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();
> +}
> +