Hi All,
Intel's Trust Domain Extensions (TDX) protect guest VMs from malicious
hosts and some physical attacks. VM guest with TDX support is called
as TD Guest.
In TD Guest, the attestation process is used to verify the
trustworthiness of TD guest to the 3rd party servers. Such attestation
process is required by 3rd party servers before sending sensitive
information to TD guests. One usage example is to get encryption keys
from the key server for mounting the encrypted rootfs or secondary drive.
Following patches adds the attestation support to TDX guest which
includes attestation user interface driver, user agent example, and
related hypercall support.
In this series, only following patches are in arch/x86 and are
intended for x86 maintainers review.
* x86/tdx: Add TDREPORT TDX Module call support
* x86/tdx: Add GetQuote TDX hypercall support
* x86/tdx: Add SetupEventNotifyInterrupt TDX hypercall support
Patch titled "platform/x86: intel_tdx_attest: Add TDX Guest attestation
interface driver" adds the attestation driver support. This is supposed
to be reviewed by platform-x86 maintainers.
Also, patch titled "tools/tdx: Add a sample attestation user app" adds
a testing app for attestation feature which needs review from
bpf@vger.kernel.org.
This series is the continuation of the following TDX patch series which
added basic TDX guest support.
[set 1] - https://lore.kernel.org/patchwork/project/lkml/list/?series=505232
[set 2] - https://lore.kernel.org/patchwork/project/lkml/list/?series=506230
[set 3] - https://lore.kernel.org/patchwork/project/lkml/list/?series=506231
[set 4] - https://lore.kernel.org/patchwork/project/lkml/list/?series=506232
[set 5] - https://lore.kernel.org/patchwork/project/lkml/list/?series=506233
Also please note that this series alone is not necessarily fully
functional.
You can find TDX related documents in the following link.
https://software.intel.com/content/www/br/pt/develop/articles/intel-trust-domain-extensions.html
Changes since v1:
* Included platform-x86 and test tool maintainers in recipient list.
* Fixed commit log and comments in attestation driver as per Han's comments.
Kuppuswamy Sathyanarayanan (6):
x86/tdx: Add TDREPORT TDX Module call support
x86/tdx: Add GetQuote TDX hypercall support
x86/tdx: Add SetupEventNotifyInterrupt TDX hypercall support
x86/tdx: Add TDX Guest event notify interrupt vector support
platform/x86: intel_tdx_attest: Add TDX Guest attestation interface
driver
tools/tdx: Add a sample attestation user app
arch/x86/include/asm/hardirq.h | 1 +
arch/x86/include/asm/idtentry.h | 4 +
arch/x86/include/asm/irq_vectors.h | 7 +-
arch/x86/include/asm/tdx.h | 6 +
arch/x86/kernel/irq.c | 7 +
arch/x86/kernel/tdx.c | 129 +++++++++++++
drivers/platform/x86/Kconfig | 9 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_tdx_attest.c | 171 +++++++++++++++++
include/uapi/misc/tdx.h | 37 ++++
tools/Makefile | 13 +-
tools/tdx/Makefile | 19 ++
tools/tdx/attest/.gitignore | 2 +
tools/tdx/attest/Makefile | 24 +++
tools/tdx/attest/tdx-attest-test.c | 232 ++++++++++++++++++++++++
15 files changed, 655 insertions(+), 7 deletions(-)
create mode 100644 drivers/platform/x86/intel_tdx_attest.c
create mode 100644 include/uapi/misc/tdx.h
create mode 100644 tools/tdx/Makefile
create mode 100644 tools/tdx/attest/.gitignore
create mode 100644 tools/tdx/attest/Makefile
create mode 100644 tools/tdx/attest/tdx-attest-test.c
--
2.25.1
The second stage in the attestation process is for the guest to
request the VMM generate and sign a quote based on the TDREPORT
acquired earlier.
Add tdx_hcall_get_quote() helper function to implement the GetQuote
hypercall.
More details about the GetQuote TDVMCALL are in the Guest-Host
Communication Interface (GHCI) Specification, sec 3.3, titled
"TDG.VP.VMCALL<GetQuote>".
This will be used by the TD attestation driver in follow-on patches.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
arch/x86/include/asm/tdx.h | 2 ++
arch/x86/kernel/tdx.c | 30 ++++++++++++++++++++++++++++++
2 files changed, 32 insertions(+)
The TDX Guest-Host Communication Interface (GHCI) includes a module
call (TDREPORT TDCALL) that a guest can make to acquire a copy of the
attestation data that it needs to verify its trustworthiness.
Add a wrapper function tdx_mcall_tdreport() that makes the module
call to get this data.
See GHCI section 2.4.5 "TDCALL [TDG.MR.REPORT] leaf" for additional
details.
[Xiaoyao: Proposed error code fix]
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
arch/x86/include/asm/tdx.h | 2 ++
arch/x86/kernel/tdx.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
@@ -96,6 +96,8 @@ extern int tdx_hcall_gpa_intent(phys_addr_t gpa, int numpages,booltdg_filter_enabled(void);+inttdx_mcall_tdreport(u64data,u64reportdata);+/**TosupportI/Oportaccessindecompressororearlykernelinit*code,since#VEexceptionhandlercannotbeused,useparavirt
SetupEventNotifyInterrupt TDX hypercall is used by guest TD to specify
which interrupt vector to use as an event-notify vector to the VMM.
Such registered vector is also used by Host VMM to notify about
completion of GetQuote requests to the Guest TD.
Add tdx_hcall_set_notify_intr() helper function to implement the
SetupEventNotifyInterrupt hypercall.
This will be used by the TD quote driver.
Details about the SetupEventNotifyInterrupt TDVMCALL can be found in
TDX Guest-Host Communication Interface (GHCI) Specification, sec 3.5
"TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
arch/x86/kernel/tdx.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
@@ -202,6 +203,30 @@ int tdx_hcall_get_quote(u64 data)}EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);+/*+*tdx_hcall_set_notify_intr()-SetupEventNotifyInterruptVector.+*+*@vector:Vectoraddresstobeusedfornotification.+*+*return0onsuccessorfailureerrornumber.+*/+inttdx_hcall_set_notify_intr(u8vector)+{+u64ret;++/* Minimum vector value allowed is 32 */+if(vector<32)+return-EINVAL;++ret=_trace_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR,vector,0,0,0,+NULL);++if(ret==TDCALL_INVALID_OPERAND)+return-EINVAL;++return0;+}+staticvoidtdg_get_info(void){u64ret;
Allocate 0xec IRQ vector address for TDX guest to receive the event
completion notification from VMM. Since this vector address will be
sent to VMM via hypercall, allocate a fixed address and move
LOCAL_TIMER_VECTOR vector address by 1 byte. Also add related IDT
handler to process the notification event.
It will be mainly used by attestation driver to receive Quote event
completion notification from host.
Add support to track the notification event status via /proc/interrupts.
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
arch/x86/include/asm/hardirq.h | 1 +
arch/x86/include/asm/idtentry.h | 4 +++
arch/x86/include/asm/irq_vectors.h | 7 ++++-
arch/x86/include/asm/tdx.h | 2 ++
arch/x86/kernel/irq.c | 7 +++++
arch/x86/kernel/tdx.c | 41 ++++++++++++++++++++++++++++++
6 files changed, 61 insertions(+), 1 deletion(-)
@@ -0,0 +1,24 @@+# SPDX-License-Identifier: GPL-2.0+# Makefile for vm tools+#+VAR_CFLAGS:=$(shellpkg-config--cflagslibtracefs2>/dev/null)+VAR_LDLIBS:=$(shellpkg-config--libslibtracefs2>/dev/null)++TARGETS=tdx-attest-test+CFLAGS=-static-Wall-Wextra-g-O2$(VAR_CFLAGS)+LDFLAGS=-lpthread$(VAR_LDLIBS)++all:$(TARGETS)++%:%.c+$(CC)$(CFLAGS)-o$@$<$(LDFLAGS)++clean:+$(RM)tdx-attest-test++prefix?=/usr/local+sbindir?=${prefix}/sbin++install:all+install-d$(DESTDIR)$(sbindir)+install-m755-p$(TARGETS)$(DESTDIR)$(sbindir)
TDX guest supports encrypted disk as root or secondary drives.
Decryption keys required to access such drives are usually maintained
by 3rd party key servers. Attestation is required by 3rd party key
servers to get the key for an encrypted disk volume, or possibly other
encrypted services. Attestation is used to prove to the key server that
the TD guest is running in a valid TD and the kernel and virtual BIOS
and other environment are secure.
During the boot process various components before the kernel accumulate
hashes in the TDX module, which can then combined into a report. This
would typically include a hash of the bios, bios configuration, boot
loader, command line, kernel, initrd. After checking the hashes the
key server will securely release the keys.
The actual details of the attestation protocol depend on the particular
key server configuration, but some parts are common and need to
communicate with the TDX module.
This communication is implemented in the attestation driver.
The supported steps are:
1. TD guest generates the TDREPORT that contains version information
about the Intel TDX module, measurement of the TD, along with a
TD-specified nonce.
2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
which is used by the host to generate a quote via quoting
enclave (QE).
3. Quote generation completion notification is sent to TD OS via
callback interrupt vector configured by TD using
SetupEventNotifyInterrupt hypercall.
4. After receiving the generated TDQUOTE, a remote verifier can be
used to verify the quote and confirm the trustworthiness of the
TD.
Attestation agent uses IOCTLs implemented by the attestation driver to
complete the various steps of the attestation process.
Also note that, explicit access permissions are not enforced in this
driver because the quote and measurements are not a secret. However
the access permissions of the device node can be used to set any
desired access policy. The udev default is usually root access
only.
TDX_CMD_GEN_QUOTE IOCTL can be used to create an computation on the
host, but TDX assumes that the host is able to deal with malicious
guest flooding it anyways.
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 <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
drivers/platform/x86/Kconfig | 9 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_tdx_attest.c | 171 ++++++++++++++++++++++++
include/uapi/misc/tdx.h | 37 +++++
4 files changed, 218 insertions(+)
create mode 100644 drivers/platform/x86/intel_tdx_attest.c
create mode 100644 include/uapi/misc/tdx.h
From: Xiaoyao Li <hidden> Date: 2021-07-08 08:16:35
On 7/8/2021 4:42 AM, Kuppuswamy Sathyanarayanan wrote:
quoted hunk
The TDX Guest-Host Communication Interface (GHCI) includes a module
call (TDREPORT TDCALL) that a guest can make to acquire a copy of the
attestation data that it needs to verify its trustworthiness.
Add a wrapper function tdx_mcall_tdreport() that makes the module
call to get this data.
See GHCI section 2.4.5 "TDCALL [TDG.MR.REPORT] leaf" for additional
details.
[Xiaoyao: Proposed error code fix]
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
arch/x86/include/asm/tdx.h | 2 ++
arch/x86/kernel/tdx.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
@@ -96,6 +96,8 @@ extern int tdx_hcall_gpa_intent(phys_addr_t gpa, int numpages,booltdg_filter_enabled(void);+inttdx_mcall_tdreport(u64data,u64reportdata);+/**TosupportI/Oportaccessindecompressororearlykernelinit*code,since#VEexceptionhandlercannotbeused,useparavirt
Sorry I guess I didn't state it clearly during internal review.
I suggest something like this
if (ret != TDCALL_SUCCESS) {
if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
return -EINVAL;
else if (TDCALL_RETURN_CODE(ret) == TDCALL_OPERAND_BUSY)
return -EBUSY;
else
return -EFAULT; //I'm not sure if -EFAULT is proper.
}
Sorry I guess I didn't state it clearly during internal review.
I suggest something like this
if (ret != TDCALL_SUCCESS) {
if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
return -EINVAL;
else if (TDCALL_RETURN_CODE(ret) == TDCALL_OPERAND_BUSY)
return -EBUSY;
else
return -EFAULT; //I'm not sure if -EFAULT is proper.
}
As per current spec, TDCALL_INVALID_OPERAND, TDCALL_OPERAND_BUSY and
0 are the only possible return values. So I have checked for failure case
in if condition and returned success by default. Any reason for specifically
checking for success code ?
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
From: Hans de Goede <hidden> Date: 2021-07-08 14:20:36
Hi,
On 7/8/21 4:07 PM, Kuppuswamy, Sathyanarayanan wrote:
On 7/8/21 1:16 AM, Xiaoyao Li wrote:
quoted
Sorry I guess I didn't state it clearly during internal review.
I suggest something like this
if (ret != TDCALL_SUCCESS) {
if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
return -EINVAL;
else if (TDCALL_RETURN_CODE(ret) == TDCALL_OPERAND_BUSY)
return -EBUSY;
else
return -EFAULT; //I'm not sure if -EFAULT is proper.
}
As per current spec, TDCALL_INVALID_OPERAND, TDCALL_OPERAND_BUSY and
0 are the only possible return values. So I have checked for failure case
in if condition and returned success by default. Any reason for specifically
checking for success code ?
Yes, new error codes might be introduced and you might forget to
update this (or other) checks.
Checking for errors really MUST always be done by checking for
ret != success (typically ret != 0 or ret < 0).
Only checking for known error codes means that if somehow an
unknown error code gets thrown this gets treated as success,
which is not acceptable behavior.
Regards,
Hans
Yes, new error codes might be introduced and you might forget to
update this (or other) checks.
Checking for errors really MUST always be done by checking for
ret != success (typically ret != 0 or ret < 0).
Only checking for known error codes means that if somehow an
unknown error code gets thrown this gets treated as success,
which is not acceptable behavior.
Got it. I will include this change in next version.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
From: Andy Lutomirski <luto@kernel.org> Date: 2021-07-08 22:21:34
On 7/7/21 1:42 PM, Kuppuswamy Sathyanarayanan wrote:
quoted hunk
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 <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
drivers/platform/x86/Kconfig | 9 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_tdx_attest.c | 171 ++++++++++++++++++++++++
include/uapi/misc/tdx.h | 37 +++++
4 files changed, 218 insertions(+)
create mode 100644 drivers/platform/x86/intel_tdx_attest.c
create mode 100644 include/uapi/misc/tdx.h
@@ -0,0 +1,171 @@+// SPDX-License-Identifier: GPL-2.0+/*+*intel_tdx_attest.c-TDXguestattestationinterfacedriver.+*+*Implementsuserinterfacetotriggerattestationprocessand+*readtheTDQuoteresult.+*+*Copyright(C)2020IntelCorporation+*+*Author:+*KuppuswamySathyanarayanan<sathyanarayanan.kuppuswamy@linux.intel.com>+*/++#define pr_fmt(fmt) "x86/tdx: attest: " fmt++#include<linux/module.h>+#include<linux/miscdevice.h>+#include<linux/uaccess.h>+#include<linux/fs.h>+#include<linux/mm.h>+#include<linux/slab.h>+#include<linux/set_memory.h>+#include<linux/io.h>+#include<asm/apic.h>+#include<asm/tdx.h>+#include<asm/irq_vectors.h>+#include<uapi/misc/tdx.h>++#define VERSION "1.0"++/* Used in Quote memory allocation */+#define QUOTE_SIZE (2 * PAGE_SIZE)++/* Mutex to synchronize attestation requests */+staticDEFINE_MUTEX(attestation_lock);+/* Completion object to track attestation status */+staticDECLARE_COMPLETION(attestation_done);++staticvoidattestation_callback_handler(void)+{+complete(&attestation_done);+}++staticlongtdg_attest_ioctl(structfile*file,unsignedintcmd,+unsignedlongarg)+{+u64data=virt_to_phys(file->private_data);
quoted hunk
+ 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.
quoted hunk
++ /* Generate TDREPORT_STRUCT */+ if (tdx_mcall_tdreport(data, virt_to_phys(reportdata))) {+ ret = -EIO;+ break;+ }
This does the hypercall.
quoted hunk
++ 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?
quoted hunk
+ 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?)
quoted hunk
++ /* 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.
quoted hunk
++ 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.
From: Dave Hansen <hidden> Date: 2021-07-08 22:35:33
On 7/8/21 3:21 PM, Andy Lutomirski wrote:
quoted
+ 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?)
Expensive and permanently fractures the direct map.
I'm struggling to figure out why the direct map is even touched here.
Why not just use a vmalloc area mapping? You really just need *a*
decrypted mapping to the page. You don't need to make *every* mapping
to the page decrypted.
On 7/7/21 1:42 PM, Kuppuswamy Sathyanarayanan wrote:
quoted
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 <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
quoted
+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.
quoted
+static long tdg_attest_ioctl(struct file *file, unsigned int cmd,+ unsigned long arg)+{+ u64 data = virt_to_phys(file->private_data);
quoted
+ 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.
quoted
++ /* Generate TDREPORT_STRUCT */+ if (tdx_mcall_tdreport(data, virt_to_phys(reportdata))) {+ ret = -EIO;+ break;+ }
This does the hypercall.
quoted
++ 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.
quoted
+ 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.
quoted
++ /* 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.
quoted
++ 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
From: Dan Williams <hidden> Date: 2021-07-08 23:36:39
On Wed, Jul 7, 2021 at 1:43 PM Kuppuswamy Sathyanarayanan
[off-list ref] wrote:
quoted hunk
TDX guest supports encrypted disk as root or secondary drives.
Decryption keys required to access such drives are usually maintained
by 3rd party key servers. Attestation is required by 3rd party key
servers to get the key for an encrypted disk volume, or possibly other
encrypted services. Attestation is used to prove to the key server that
the TD guest is running in a valid TD and the kernel and virtual BIOS
and other environment are secure.
During the boot process various components before the kernel accumulate
hashes in the TDX module, which can then combined into a report. This
would typically include a hash of the bios, bios configuration, boot
loader, command line, kernel, initrd. After checking the hashes the
key server will securely release the keys.
The actual details of the attestation protocol depend on the particular
key server configuration, but some parts are common and need to
communicate with the TDX module.
This communication is implemented in the attestation driver.
The supported steps are:
1. TD guest generates the TDREPORT that contains version information
about the Intel TDX module, measurement of the TD, along with a
TD-specified nonce.
2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
which is used by the host to generate a quote via quoting
enclave (QE).
3. Quote generation completion notification is sent to TD OS via
callback interrupt vector configured by TD using
SetupEventNotifyInterrupt hypercall.
4. After receiving the generated TDQUOTE, a remote verifier can be
used to verify the quote and confirm the trustworthiness of the
TD.
Attestation agent uses IOCTLs implemented by the attestation driver to
complete the various steps of the attestation process.
Also note that, explicit access permissions are not enforced in this
driver because the quote and measurements are not a secret. However
the access permissions of the device node can be used to set any
desired access policy. The udev default is usually root access
only.
TDX_CMD_GEN_QUOTE IOCTL can be used to create an computation on the
host, but TDX assumes that the host is able to deal with malicious
guest flooding it anyways.
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 <redacted>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
drivers/platform/x86/Kconfig | 9 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_tdx_attest.c | 171 ++++++++++++++++++++++++
include/uapi/misc/tdx.h | 37 +++++
4 files changed, 218 insertions(+)
create mode 100644 drivers/platform/x86/intel_tdx_attest.c
create mode 100644 include/uapi/misc/tdx.h
Individual module versions are typically useless as the kernel version
is sufficient.
quoted hunk
++/* Used in Quote memory allocation */+#define QUOTE_SIZE (2 * PAGE_SIZE)++/* Mutex to synchronize attestation requests */+static DEFINE_MUTEX(attestation_lock);+/* Completion object to track attestation status */+static DECLARE_COMPLETION(attestation_done);++static void attestation_callback_handler(void)+{+ complete(&attestation_done);+}++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;+ }++ /* Generate TDREPORT_STRUCT */+ if (tdx_mcall_tdreport(data, virt_to_phys(reportdata))) {+ ret = -EIO;+ break;+ }++ if (copy_to_user(argp, file->private_data, TDX_TDREPORT_LEN))+ ret = -EFAULT;+ 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;++ /* 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));
This wants a get_nr_pages() helper.
quoted hunk
++ 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);
Why is this ioctl not part of the driver that registered the interrupt
handler for this callback in the first instance? I've never seen this
style of cross-driver communication before.
Why does this driver abandon all semblance of type-safety and use
->private_data directly? This also seems an easy way to consume
memory, just keep opening this device over and over again.
AFAICS this buffer is only used ephemerally. I see no reason it needs
to be allocated once per open file. Unless you need several threads to
be running the attestation process in parallel just allocate a single
buffer at module init (statically defined or on the heap) and use a
lock to enforce only one user of this buffer at a time. That would
also solve your direct-map fracturing problem.
All that said, this new user ABI for passing blobs in and out of the
kernel is something that the keyutils API already does. Did you
consider add_key() / request_key() for this case? That would also be
the natural path for the end step of requesting the drive decrypt key.
I.e. a chain of key payloads starting with establishing the
attestation blob.
+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);
Why is this ioctl not part of the driver that registered the interrupt
We cannot club them because they are not functionally related. Even notification
is a separate common feature supported by TDX and configured using
SetupEventNotifyInterrupt hypercall. It is not related to TDX attestation.
Attestation just uses event notification interface to get the quote
completion event.
handler for this callback in the first instance? I've never seen this
style of cross-driver communication before.
This is similar to x86_platform_ipi_callback() acrn_setup_intr_handler()
use cases.
Why does this driver abandon all semblance of type-safety and use
->private_data directly? This also seems an easy way to consume
memory, just keep opening this device over and over again.
AFAICS this buffer is only used ephemerally. I see no reason it needs
to be allocated once per open file. Unless you need several threads to
be running the attestation process in parallel just allocate a single
buffer at module init (statically defined or on the heap) and use a
lock to enforce only one user of this buffer at a time. That would
also solve your direct-map fracturing problem.
Theoretically attestation requests can be sent in parallel. I have
allocated the memory in open() call mainly for this reason. But current
TDX ABI specification does not clearly specify this possibility and I am
not sure whether TDX KVM supports it. Let me confirm about it again with
TDX KVM owner. If such model is not currently supported, then I will move
the memory allocation to init code.
All that said, this new user ABI for passing blobs in and out of the
kernel is something that the keyutils API already does. Did you
consider add_key() / request_key() for this case? That would also be
the natural path for the end step of requesting the drive decrypt key.
I.e. a chain of key payloads starting with establishing the
attestation blob.
I am not sure whether we can use keyutil interface for attestation. AFAIK,
there are other use cases for attestation other than getting keys for
encrypted drives.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
From: Dan Williams <hidden> Date: 2021-07-09 00:20:40
On Thu, Jul 8, 2021 at 4:57 PM Kuppuswamy, Sathyanarayanan
[off-list ref] wrote:
On 7/8/21 4:36 PM, Dan Williams wrote:
quoted
quoted
+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);
Why is this ioctl not part of the driver that registered the interrupt
We cannot club them because they are not functionally related. Even notification
is a separate common feature supported by TDX and configured using
SetupEventNotifyInterrupt hypercall. It is not related to TDX attestation.
Attestation just uses event notification interface to get the quote
completion event.
quoted
handler for this callback in the first instance? I've never seen this
style of cross-driver communication before.
This is similar to x86_platform_ipi_callback() acrn_setup_intr_handler()
use cases.
Those appear to be for core functionality, not one off drivers. Where
is the code that does the SetupEventNotifyInterrupt, is it a driver?
Why does this driver abandon all semblance of type-safety and use
->private_data directly? This also seems an easy way to consume
memory, just keep opening this device over and over again.
AFAICS this buffer is only used ephemerally. I see no reason it needs
to be allocated once per open file. Unless you need several threads to
be running the attestation process in parallel just allocate a single
buffer at module init (statically defined or on the heap) and use a
lock to enforce only one user of this buffer at a time. That would
also solve your direct-map fracturing problem.
Theoretically attestation requests can be sent in parallel. I have
allocated the memory in open() call mainly for this reason. But current
TDX ABI specification does not clearly specify this possibility and I am
not sure whether TDX KVM supports it. Let me confirm about it again with
TDX KVM owner. If such model is not currently supported, then I will move
the memory allocation to init code.
If you have a lock would TDX KVM even notice that its parallel
requests are being handled serially? I.e. even if they said "yes,
multiple requests may happen in parallel", until it becomes an actual
latency problem in practice it's not clear that this generous use of
resources is justified.
Scratch that... this driver already has the attestation_lock! So, it's
already the case that only one thread can be attesting at a time. The
per-file buffer is unecessary.
quoted
All that said, this new user ABI for passing blobs in and out of the
kernel is something that the keyutils API already does. Did you
consider add_key() / request_key() for this case? That would also be
the natural path for the end step of requesting the drive decrypt key.
I.e. a chain of key payloads starting with establishing the
attestation blob.
I am not sure whether we can use keyutil interface for attestation. AFAIK,
there are other use cases for attestation other than getting keys for
encrypted drives.
keyutils supports generating and passing blobs into and out of the
kernel with a handle associated to those blobs. This driver adds a TDX
way to pass blobs into and out of the kernel. If Linux grows other
TDX-like attestation requirements in the future (e.g. PCI SPDM) should
each of those invent their own user ABI for passing blobs around?
If you have a lock would TDX KVM even notice that its parallel
requests are being handled serially? I.e. even if they said "yes,
multiple requests may happen in parallel", until it becomes an actual
latency problem in practice it's not clear that this generous use of
resources is justified.
The worst case usage is 2 pages * file descriptor. There are lots of other ways to use that much and more memory for each file descriptor.
Scratch that... this driver already has the attestation_lock! So, it's
already the case that only one thread can be attesting at a time. The
per-file buffer is unecessary.
But then you couldn't free the buffer. So it would be leaked forever for likely only one attestation.
Not sure what problem you're trying to solve here.
keyutils supports generating and passing blobs into and out of the
kernel with a handle associated to those blobs. This driver adds a TDX
way to pass blobs into and out of the kernel. If Linux grows other
TDX-like attestation requirements in the future (e.g. PCI SPDM) should
each of those invent their own user ABI for passing blobs around?
The TDX blobs are different than any blobs that keyutils supports today. The TDX operations are different too.
TDREPORT doesn't even involve any keys, it's just attestation reports.
keyutils today nothing related to attestation.
I just don't see any commonality. If there was commonality it would be more with the TPM interface, but TDX attestation is different enough that it also isn't feasible to directly convert it into TPM operation (apart from standard TPM being a beast that you better avoid as much as possible anyways)
-Andi
Expensive and permanently fractures the direct map.
I'm struggling to figure out why the direct map is even touched here.
I think Sathya did it this way because the TD interface requires a physical address.
Why not just use a vmalloc area mapping? You really just need *a*
decrypted mapping to the page. You don't need to make *every* mapping
to the page decrypted.
Yes it would be possible to use vmap() on the page and only set the vmap encrypted by passing the right flags directly.
That would avoid breaking up the direct mapping.
-Andi
From: Dan Williams <hidden> Date: 2021-07-09 01:37:45
On Thu, Jul 8, 2021 at 5:36 PM Andi Kleen [off-list ref] wrote:
On 7/8/2021 5:20 PM, Dan Williams wrote:
quoted
If you have a lock would TDX KVM even notice that its parallel
requests are being handled serially? I.e. even if they said "yes,
multiple requests may happen in parallel", until it becomes an actual
latency problem in practice it's not clear that this generous use of
resources is justified.
The worst case usage is 2 pages * file descriptor. There are lots of
other ways to use that much and more memory for each file descriptor.
quoted
Scratch that... this driver already has the attestation_lock! So, it's
already the case that only one thread can be attesting at a time. The
per-file buffer is unecessary.
But then you couldn't free the buffer. So it would be leaked forever for
likely only one attestation.
Not sure what problem you're trying to solve here.
One allocation for the life of the driver that can have its direct map
permissions changed rather than an allocation per-file descriptor and
fragmenting the direct map.
quoted
keyutils supports generating and passing blobs into and out of the
kernel with a handle associated to those blobs. This driver adds a TDX
way to pass blobs into and out of the kernel. If Linux grows other
TDX-like attestation requirements in the future (e.g. PCI SPDM) should
each of those invent their own user ABI for passing blobs around?
The TDX blobs are different than any blobs that keyutils supports today.
The TDX operations are different too.
TDREPORT doesn't even involve any keys, it's just attestation reports.
keyutils today nothing related to attestation.
I just don't see any commonality. If there was commonality it would be
more with the TPM interface, but TDX attestation is different enough
that it also isn't feasible to directly convert it into TPM operation
(apart from standard TPM being a beast that you better avoid as much as
possible anyways)
Ok. I'll leave that alone for TDX, but I still have my eyes on
keyutils for aspects of PCI SPDM.
One allocation for the life of the driver that can have its direct map
permissions changed rather than an allocation per-file descriptor and
fragmenting the direct map.
The vmap() approach discussed in another mail will solve that.
-Andi
From: Dan Williams <hidden> Date: 2021-07-09 02:04:57
On Thu, Jul 8, 2021 at 6:44 PM Andi Kleen [off-list ref] wrote:
quoted
One allocation for the life of the driver that can have its direct map
permissions changed rather than an allocation per-file descriptor and
fragmenting the direct map.
The vmap() approach discussed in another mail will solve that.
Ok, not my first choice for how to handle the allocation side of that,
but not broken.
I'd still feel better if there was an actual data structure assigned
to file->private_data rather than using that 'void *' pointer directly
and casting throughout the driver.
Ok, not my first choice for how to handle the allocation side of that,
but not broken.
I'd still feel better if there was an actual data structure assigned
to file->private_data rather than using that 'void *' pointer directly
and casting throughout the driver.
We can add a data structure if we have more member requirements. Currently
we only need to pass the memory pointer. But after moving memory allocation
to init code (even for vmap), we may not need to use this private pointer.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Expensive and permanently fractures the direct map.
I'm struggling to figure out why the direct map is even touched here.
I think Sathya did it this way because the TD interface requires a physical address.
quoted
Why not just use a vmalloc area mapping? You really just need *a*
decrypted mapping to the page. You don't need to make *every* mapping
to the page decrypted.
Yes it would be possible to use vmap() on the page and only set the vmap encrypted by passing the right flags directly.
Is it alright to have non coherent mappings? If yes, any documentation reference for it?
That would avoid breaking up the direct mapping.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
From: Dave Hansen <hidden> Date: 2021-07-13 00:44:35
On 7/12/21 5:33 PM, Kuppuswamy, Sathyanarayanan wrote:
On 7/8/21 5:38 PM, Andi Kleen wrote:
quoted
quoted
Expensive and permanently fractures the direct map.
I'm struggling to figure out why the direct map is even touched
here.
I think Sathya did it this way because the TD interface requires a
physical address.
quoted
Why not just use a vmalloc area mapping? You really just need
*a* decrypted mapping to the page. You don't need to make
*every* mapping to the page decrypted.
Yes it would be possible to use vmap() on the page and only set
the vmap encrypted by passing the right flags directly.
Is it alright to have non coherent mappings? If yes, any
documentation reference for it?
Do you mean non-cache-coherent mappings? I'm not sure what that has to
do with creating "unencrypted" (shared) mappings.
Are you asking exactly which arguments to pass to vmap() or to vmap_pfn()?
@@ -0,0 +1,24 @@+# SPDX-License-Identifier: GPL-2.0+# Makefile for vm tools+#+VAR_CFLAGS:=$(shellpkg-config--cflagslibtracefs2>/dev/null)+VAR_LDLIBS:=$(shellpkg-config--libslibtracefs2>/dev/null)++TARGETS=tdx-attest-test+CFLAGS=-static-Wall-Wextra-g-O2$(VAR_CFLAGS)+LDFLAGS=-lpthread$(VAR_LDLIBS)++all:$(TARGETS)++%:%.c+$(CC)$(CFLAGS)-o$@$<$(LDFLAGS)++clean:+$(RM)tdx-attest-test++prefix?=/usr/local+sbindir?=${prefix}/sbin++install:all+install-d$(DESTDIR)$(sbindir)+install-m755-p$(TARGETS)$(DESTDIR)$(sbindir)
In tdg_attest_ioctl() TDX_CMD_GEN_QUOTE case is calling
tdx_mcall_tdreport() same as TDX_CMD_GET_TDREPORT case. Then what is
the point of calling get_tdreport() here? Do you mean to call
gen_report_data()?
In tdg_attest_ioctl() TDX_CMD_GEN_QUOTE case is calling
tdx_mcall_tdreport() same as TDX_CMD_GET_TDREPORT case. Then what is
the point of calling get_tdreport() here? Do you mean to call
gen_report_data()?
Yes, I also noticed this issue and fixed the attestation driver to
to get TDREPORT data as input to get TDQUOTE.
I will be posting the fixed version of attestation driver today.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer