Date:   Mon, 2 May 2022 09:02:27 -0700
From:   Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     Kai Huang <kai.huang@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org
Cc:     "H . Peter Anvin" <hpa@...or.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Wander Lairson Costa <wander@...hat.com>,
        Isaku Yamahata <isaku.yamahata@...il.com>,
        marcelo.cerri@...onical.com, tim.gardner@...onical.com,
        khalid.elmously@...onical.com, philip.cox@...onical.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] x86/tdx: Add Quote generation support

Hi Kai,

On 5/1/22 10:01 PM, Kai Huang wrote:
> 
>> +
>> +static long tdx_get_quote(void __user *argp)
>> +{
>> +	struct tdx_quote_req quote_req;
>> +	long ret = 0;
>> +	int order;
>> +
>> +	/* Hold lock to serialize GetQuote requests */
>> +	mutex_lock(&quote_lock);
>> +
>> +	reinit_completion(&req_compl);
>> +
>> +	/* Copy GetQuote request struct from user buffer */
>> +	if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req))) {
>> +		ret = -EFAULT;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Make sure the length & timeout is valid */
>> +	if (!quote_req.len || !quote_req.timeout) {
>> +		ret = -EINVAL;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Get order for Quote buffer page allocation */
>> +	order = get_order(quote_req.len);
>> +
>> +	/*
>> +	 * Allocate buffer to get TD Quote from the VMM.
>> +	 * Size needs to be 4KB aligned (which is already
>> +	 * met in page allocation).
>> +	 */
>> +	tdquote = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
>> +	if (!tdquote) {
>> +		ret = -ENOMEM;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/*
>> +	 * Since this buffer will be shared with the VMM via GetQuote
>> +	 * hypercall, decrypt it.
>> +	 */
>> +	ret = set_memory_decrypted((unsigned long)tdquote, 1UL << order);
>> +	if (ret)
>> +		goto quote_failed;
>> +
>> +	/* Copy TDREPORT from user buffer to kernel Quote buffer */
>> +	if (copy_from_user(tdquote, (void __user *)quote_req.buf, quote_req.len)) {
>> +		ret = -EFAULT;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Submit GetQuote Request */
>> +	ret = tdx_get_quote_hypercall(tdquote, (1ULL << order) * PAGE_SIZE);
>> +	if (ret) {
>> +		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
>> +		ret = -EIO;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Wait for attestation completion */
>> +	ret = wait_for_completion_interruptible(&req_compl);
>> +	if (ret <= 0) {
>> +		ret = -EIO;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Copy output data back to user buffer */
>> +	if (copy_to_user((void __user *)quote_req.buf, tdquote, quote_req.len))
>> +		ret = -EFAULT;
>> +
>> +quote_failed:
>> +	if (tdquote)
>> +		free_pages((unsigned long)tdquote, order);
> 
> The buffer is freed w/o being converted back to private.  How can you prevent
> the buffer from being allocated by kernel and used as private pages again?

Yes. It needs set_memory_encrypted() call here. I will fix this in next
version.

> 
> Also, the  buffer may be still used by VMM when timeout (IN_FLIGHT), how can
> this even work?

We will never reach here for IN_FLIGHT case. We will block in
wait_for_completion_interruptible() till the status changes to success
or failure.

> 
>> +	tdquote = NULL;
>> +	mutex_unlock(&quote_lock);
>> +	return ret;
>> +}
>> +
>> +static void attestation_callback_handler(void)
>> +{
>> +	struct tdx_quote_hdr *quote_hdr;
>> +
>> +	quote_hdr = (struct tdx_quote_hdr *) tdquote;
>> +
>> +	/* Check for spurious callback IRQ case */
>> +	if (!tdquote || quote_hdr->status == GET_QUOTE_IN_FLIGHT)
>> +		return;
> 
> I don't get the logic.  Please explain.

I am trying to handle spurious IRQ case here. If we receive a callback
IRQ from VMM before even we allocate tdquote or post the GetQuote
request, accessing quote_hdr->status will lead to NULL pointer
exception. So I have added check for valid quote buffer (tdquote !=
NULL)

Second condition (quote_hdr->status == GET_QUOTE_IN_FLIGHT)) makes
sure we don't mark the current quote request complete until the
Quote buffer status changes to GET_QUOTE_SUCCESS, GET_QUOTE_ERROR or
GET_QUOTE_SERVICE_UNAVAILABLE.

> 
>> +
>> +	complete(&req_compl);
>> +}
>> +
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Powered by blists - more mailing lists