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

[Sathyanarayanan Kuppuswamy]

On 4/28/22 10:58 AM, Wander Lairson Costa wrote:
> 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.

Yes. I will change it to non-zero check.

> > +		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.

Since we are already in error path and setting ret to -EIO I did not
handle the pur_user() error case.

> > +		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?

Good catch. I will fix it in next version.

> > +		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?

IMO, since GetQuote TDVMCALL is successful, we can copy the buffer
back to the user. My idea is to let the attestation agent handle the
error in GetQuote header (like IN_FLIGHT scenario).

Maybe I should remove quote_hdr->status check in kernel and let user
agent handle it.

Isaku/Kai, any comments?

> > +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?

Yes. I will fix this in next version.

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

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer