All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Kuppuswamy, Sathyanarayanan"  <sathyanarayanan.kuppuswamy@linux.intel.com>
To: Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <mgross@linux.intel.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>
Cc: Peter H Anvin <hpa@zytor.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Tony Luck <tony.luck@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	Kirill Shutemov <kirill.shutemov@linux.intel.com>,
	Sean Christopherson <seanjc@google.com>,
	Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	platform-driver-x86@vger.kernel.org, bpf@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver
Date: Thu, 8 Jul 2021 16:34:22 -0700	[thread overview]
Message-ID: <02b82025-fcb9-d5bc-fb10-52cec17e8588@linux.intel.com> (raw)
In-Reply-To: <06c85c19-e16c-3121-ed47-075cfa779b67@kernel.org>

Hi,

On 7/8/21 3:21 PM, Andy Lutomirski wrote:
> On 7/7/21 1:42 PM, Kuppuswamy Sathyanarayanan wrote:
> 
>> The interaction with the TDX module is like a RPM protocol here. There
>> are several operations (get tdreport, get quote) that need to input a
>> blob, and then output another blob. It was considered to use a sysfs
>> interface for this, but it doesn't fit well into the standard sysfs
>> model for configuring values. It would be possible to do read/write on
>> files, but it would need multiple file descriptors, which would be
>> somewhat messy. ioctls seems to be the best fitting and simplest model
>> here. There is one ioctl per operation, that takes the input blob and
>> returns the output blob, and as well as auxiliary ioctls to return the
>> blob lengths. The ioctls are documented in the header file.
>>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---


>> +config INTEL_TDX_ATTESTATION
>> +	tristate "Intel TDX attestation driver"
>> +	depends on INTEL_TDX_GUEST
>> +	help
>> +	  The TDX attestation driver provides IOCTL or MMAP interfaces to
>> +	  the user to request TDREPORT from the TDX module or request quote
>> +	  from VMM. It is mainly used to get secure disk decryption keys from
>> +	  the key server.
> 
> What's the MMAP interface

Initially this driver supported MMAP to allow user directly get the quote data
without internal copies. But it was later removed based on internal review
comments to simplify the driver interfaces. During the cleanup I somehow missed its
reference in Kconfig file. Sorry, I will fix it in next version.


>> +static long tdg_attest_ioctl(struct file *file, unsigned int cmd,
>> +			     unsigned long arg)
>> +{
>> +	u64 data = virt_to_phys(file->private_data);
> 
> 
>> +	void __user *argp = (void __user *)arg;
>> +	u8 *reportdata;
>> +	long ret = 0;
>> +
>> +	mutex_lock(&attestation_lock);
>> +
>> +	reportdata = kzalloc(TDX_TDREPORT_LEN, GFP_KERNEL);
>> +	if (!reportdata) {
>> +		mutex_unlock(&attestation_lock);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_TDREPORT:
>> +		if (copy_from_user(reportdata, argp, TDX_REPORT_DATA_LEN)) {
>> +			ret = -EFAULT;
>> +			break;
>> +		}
> 
> This copies from user memory to reportdata.
> 
>> +
>> +		/* Generate TDREPORT_STRUCT */
>> +		if (tdx_mcall_tdreport(data, virt_to_phys(reportdata))) {
>> +			ret = -EIO;
>> +			break;
>> +		}
> 
> This does the hypercall.
> 
>> +
>> +		if (copy_to_user(argp, file->private_data, TDX_TDREPORT_LEN))
>> +			ret = -EFAULT;
> 
> This copies from private_data to user memory.  How did the report get to
> private_data?

Report data is copied by previous TDX module call. The pointer we pass to it is the
physical address of file->private_data.


> 
>> +		break;
>> +	case TDX_CMD_GEN_QUOTE:
>> +		if (copy_from_user(reportdata, argp, TDX_REPORT_DATA_LEN)) {
>> +			ret = -EFAULT;
>> +			break;
>> +		}
>> +
>> +		/* Generate TDREPORT_STRUCT */
>> +		if (tdx_mcall_tdreport(data, virt_to_phys(reportdata))) {
>> +			ret = -EIO;
>> +			break;
>> +		}
>> +
>> +		ret = set_memory_decrypted((unsigned long)file->private_data,
>> +					   1UL << get_order(QUOTE_SIZE));
>> +		if (ret)
>> +			break;
> 
> Now private_data is decrypted.  (And this operation is *expensive*.  Why
> is it done at ioctl time?)

We are trying to use the same buffer in both tdx_mcall_*() and tdx_hcall_*()
functions to avoid unnecessary data copies. Since we cannot pass decrypted page
address to TDX module call, we have decrypted it in IOCTL after doing the
TDX module call.

We can move this decrypted/encrypted mapping code to open()/close() functions.
But in that case, after the TDX module call, we need to separately copy the report
data to the quote buffer before making the GetQuote hypercall.

> 
>> +
>> +		/* Submit GetQuote Request */
>> +		if (tdx_hcall_get_quote(data)) {
>> +			ret = -EIO;
>> +			goto done;
>> +		}
>> +
>> +		/* Wait for attestation completion */
>> +		wait_for_completion_interruptible(&attestation_done);
>> +
>> +		if (copy_to_user(argp, file->private_data, QUOTE_SIZE))
>> +			ret = -EFAULT;
>> +done:
>> +		ret = set_memory_encrypted((unsigned long)file->private_data,
>> +					   1UL << get_order(QUOTE_SIZE));
> 
> And this is, again, quite expensive.

Same as above.

> 
>> +
>> +		break;
>> +	case TDX_CMD_GET_QUOTE_SIZE:
>> +		if (put_user(QUOTE_SIZE, (u64 __user *)argp))
>> +			ret = -EFAULT;
>> +
>> +		break;
>> +	default:
>> +		pr_err("cmd %d not supported\n", cmd);
>> +		break;
>> +	}
>> +
>> +	mutex_unlock(&attestation_lock);
>> +
>> +	kfree(reportdata);
>> +
>> +	return ret;
>> +}
>> +
>> +static int tdg_attest_open(struct inode *inode, struct file *file)
>> +{
>> +	/*
>> +	 * Currently tdg_event_notify_handler is only used in attestation
>> +	 * driver. But, WRITE_ONCE is used as benign data race notice.
>> +	 */
>> +	WRITE_ONCE(tdg_event_notify_handler, attestation_callback_handler);
>> +
>> +	file->private_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
>> +						      get_order(QUOTE_SIZE));
> 
> This allocation has negligible cost compared to changing memory to
> decrypted.
> 
> Shouldn't you allocate a buffer once at driver load time or even at boot
> and just keep reusing it as needed?  You could have a few pages of
> shared memory for the specific purposes of hypercalls, and you could
> check them out and release them when you need some.

If you are fine with additional data copy cost, I can move the decrypted/
encrypted mapping code to open/close() functions.

> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

  parent reply	other threads:[~2021-07-08 23:34 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-07 20:42 [PATCH v2 0/6] Add TDX Guest Support (Attestation support) Kuppuswamy Sathyanarayanan
2021-07-07 20:42 ` [PATCH v2 1/6] x86/tdx: Add TDREPORT TDX Module call support Kuppuswamy Sathyanarayanan
2021-07-08  8:16   ` Xiaoyao Li
2021-07-08 14:07     ` Kuppuswamy, Sathyanarayanan
2021-07-08 14:20       ` Hans de Goede
2021-07-08 17:06         ` Kuppuswamy, Sathyanarayanan
2021-07-07 20:42 ` [PATCH v2 2/6] x86/tdx: Add GetQuote TDX hypercall support Kuppuswamy Sathyanarayanan
2021-07-07 20:42 ` [PATCH v2 3/6] x86/tdx: Add SetupEventNotifyInterrupt " Kuppuswamy Sathyanarayanan
2021-07-07 20:42 ` [PATCH v2 4/6] x86/tdx: Add TDX Guest event notify interrupt vector support Kuppuswamy Sathyanarayanan
2021-07-07 20:42 ` [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver Kuppuswamy Sathyanarayanan
2021-07-08 22:21   ` Andy Lutomirski
2021-07-08 22:35     ` Dave Hansen
2021-07-09  0:38       ` Andi Kleen
2021-07-13  0:33         ` Kuppuswamy, Sathyanarayanan
2021-07-13  0:44           ` Dave Hansen
2021-07-08 23:34     ` Kuppuswamy, Sathyanarayanan [this message]
2021-07-08 23:36   ` Dan Williams
2021-07-08 23:57     ` Kuppuswamy, Sathyanarayanan
2021-07-09  0:20       ` Dan Williams
2021-07-09  0:36         ` Andi Kleen
2021-07-09  1:37           ` Dan Williams
2021-07-09  1:44             ` Andi Kleen
2021-07-09  2:04               ` Dan Williams
2021-07-09  2:43                 ` Kuppuswamy, Sathyanarayanan
2021-07-07 20:42 ` [PATCH v2 6/6] tools/tdx: Add a sample attestation user app Kuppuswamy Sathyanarayanan
2021-07-15  8:36   ` Mian Yousaf Kaukab
2021-07-15 15:19     ` Kuppuswamy, Sathyanarayanan
2022-03-30 22:17 [PATCH v2 0/6] Add TDX Guest Attestation support Kuppuswamy Sathyanarayanan
2022-03-30 22:18 ` [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver Kuppuswamy Sathyanarayanan
2022-04-04 10:07   ` Hans de Goede
2022-04-04 19:56     ` Sathyanarayanan Kuppuswamy
2022-04-11 14:38       ` Hans de Goede
2022-04-04 10:09   ` Hans de Goede
2022-04-04 10:11     ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=02b82025-fcb9-d5bc-fb10-52cec17e8588@linux.intel.com \
    --to=sathyanarayanan.kuppuswamy@linux.intel.com \
    --cc=ak@linux.intel.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bp@alien8.de \
    --cc=bpf@vger.kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=daniel@iogearbox.net \
    --cc=dave.hansen@intel.com \
    --cc=hdegoede@redhat.com \
    --cc=hpa@zytor.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=knsathya@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mgross@linux.intel.com \
    --cc=mingo@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=seanjc@google.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.