[PATCH v2 0/6] Add TDX Guest Support (Attestation support)

[PATCH v2 0/6] Add TDX Guest Support (Attestation support)

[Kuppuswamy Sathyanarayanan]

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

[PATCH v2 1/6] x86/tdx: Add TDREPORT TDX Module call support

[Kuppuswamy Sathyanarayanan]

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 <ak@linux.intel.com>
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(+)

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 48927fac9e12..4f1b5c14a09b 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -96,6 +96,8 @@ extern int tdx_hcall_gpa_intent(phys_addr_t gpa, int numpages,
 
 bool tdg_filter_enabled(void);
 
+int tdx_mcall_tdreport(u64 data, u64 reportdata);
+
 /*
  * To support I/O port access in decompressor or early kernel init
  * code, since #VE exception handler cannot be used, use paravirt
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index f76af7661046..0f797803f4c8 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -23,6 +23,7 @@
 /* TDX Module call Leaf IDs */
 #define TDINFO				1
 #define TDGETVEINFO			3
+#define TDREPORT			4
 #define TDACCEPTPAGE			6
 
 /* TDX hypercall Leaf IDs */
@@ -30,6 +31,11 @@
 
 /* TDX Module call error codes */
 #define TDX_PAGE_ALREADY_ACCEPTED       0x8000000000000001
+#define TDCALL_RETURN_CODE_MASK		0xFFFFFFFF00000000
+#define TDCALL_OPERAND_BUSY		0x8000020000000000
+#define TDCALL_INVALID_OPERAND		0x8000000000000000
+#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
+
 
 #define VE_IS_IO_OUT(exit_qual)		(((exit_qual) & 8) ? 0 : 1)
 #define VE_GET_IO_SIZE(exit_qual)	(((exit_qual) & 7) + 1)
@@ -139,6 +145,33 @@ static bool tdg_perfmon_enabled(void)
 	return td_info.attributes & BIT(63);
 }
 
+/*
+ * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
+ *
+ * @data        : Physical address of 1024B aligned data to store
+ *                TDREPORT_STRUCT.
+ * @reportdata  : Physical address of 64B aligned report data
+ *
+ * return 0 on success or failure error number.
+ */
+int tdx_mcall_tdreport(u64 data, u64 reportdata)
+{
+	u64 ret;
+
+	if (!data || !reportdata || !prot_guest_has(PR_GUEST_TDX))
+		return -EINVAL;
+
+	ret = __trace_tdx_module_call(TDREPORT, data, reportdata, 0, 0, NULL);
+
+	if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
+		return -EINVAL;
+	else if (TDCALL_RETURN_CODE(ret) == TDCALL_OPERAND_BUSY)
+		return -EBUSY;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
+
 static void tdg_get_info(void)
 {
 	u64 ret;
-- 
2.25.1

[PATCH v2 2/6] x86/tdx: Add GetQuote TDX hypercall support

[Kuppuswamy Sathyanarayanan]

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 <ak@linux.intel.com>
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(+)

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 4f1b5c14a09b..1599aa4850e5 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -98,6 +98,8 @@ bool tdg_filter_enabled(void);
 
 int tdx_mcall_tdreport(u64 data, u64 reportdata);
 
+int tdx_hcall_get_quote(u64 data);
+
 /*
  * To support I/O port access in decompressor or early kernel init
  * code, since #VE exception handler cannot be used, use paravirt
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index 0f797803f4c8..eb3a90051604 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -28,6 +28,7 @@
 
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
+#define TDVMCALL_GET_QUOTE		0x10002
 
 /* TDX Module call error codes */
 #define TDX_PAGE_ALREADY_ACCEPTED       0x8000000000000001
@@ -36,6 +37,9 @@
 #define TDCALL_INVALID_OPERAND		0x8000000000000000
 #define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
 
+/* TDX hypercall error codes */
+#define TDVMCALL_INVALID_OPERAND	0x8000000000000000
+#define TDVMCALL_TDREPORT_FAILED	0x8000000000000001
 
 #define VE_IS_IO_OUT(exit_qual)		(((exit_qual) & 8) ? 0 : 1)
 #define VE_GET_IO_SIZE(exit_qual)	(((exit_qual) & 7) + 1)
@@ -172,6 +176,32 @@ int tdx_mcall_tdreport(u64 data, u64 reportdata)
 }
 EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
 
+/*
+ * tdx_hcall_get_quote() - Generate TDQUOTE using TDREPORT_STRUCT.
+ *
+ * @data        : Physical address of 4KB GPA memory which contains
+ *                TDREPORT_STRUCT.
+ *
+ * return 0 on success or failure error number.
+ */
+int tdx_hcall_get_quote(u64 data)
+{
+	u64 ret;
+
+	if (!data || !prot_guest_has(PR_GUEST_TDX))
+		return -EINVAL;
+
+	ret = _trace_tdx_hypercall(TDVMCALL_GET_QUOTE, data, 0, 0, 0, NULL);
+
+	if (ret == TDVMCALL_INVALID_OPERAND)
+		return -EINVAL;
+	else if (ret == TDVMCALL_TDREPORT_FAILED)
+		return -EBUSY;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
+
 static void tdg_get_info(void)
 {
 	u64 ret;
-- 
2.25.1

[PATCH v2 3/6] x86/tdx: Add SetupEventNotifyInterrupt TDX hypercall support

[Kuppuswamy Sathyanarayanan]

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 <ak@linux.intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/kernel/tdx.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index eb3a90051604..fcb0ed70ea19 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -29,6 +29,7 @@
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
 #define TDVMCALL_GET_QUOTE		0x10002
+#define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
 
 /* TDX Module call error codes */
 #define TDX_PAGE_ALREADY_ACCEPTED       0x8000000000000001
@@ -202,6 +203,30 @@ int tdx_hcall_get_quote(u64 data)
 }
 EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
 
+/*
+ * tdx_hcall_set_notify_intr() - Setup Event Notify Interrupt Vector.
+ *
+ * @vector        : Vector address to be used for notification.
+ *
+ * return 0 on success or failure error number.
+ */
+int tdx_hcall_set_notify_intr(u8 vector)
+{
+	u64 ret;
+
+	/* 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;
+
+	return 0;
+}
+
 static void tdg_get_info(void)
 {
 	u64 ret;
-- 
2.25.1

[PATCH v2 4/6] x86/tdx: Add TDX Guest event notify interrupt vector support

[Kuppuswamy Sathyanarayanan]

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 <ak@linux.intel.com>
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(-)

diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
index 07d79fa9c5c6..40d0534e7d82 100644
--- a/arch/x86/include/asm/hardirq.h
+++ b/arch/x86/include/asm/hardirq.h
@@ -46,6 +46,7 @@ typedef struct {
 #endif
 #if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)
 	unsigned int tdg_ve_count;
+	unsigned int irq_tdg_event_notify_count;
 #endif
 } ____cacheline_aligned irq_cpustat_t;
 
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index d3c779abbc78..fad1b0110c88 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -702,6 +702,10 @@ DECLARE_IDTENTRY_SYSVEC(HYPERVISOR_CALLBACK_VECTOR,	sysvec_xen_hvm_callback);
 DECLARE_IDTENTRY_SYSVEC(HYPERVISOR_CALLBACK_VECTOR,	sysvec_kvm_asyncpf_interrupt);
 #endif
 
+#ifdef CONFIG_INTEL_TDX_GUEST
+DECLARE_IDTENTRY_SYSVEC(TDX_GUEST_EVENT_NOTIFY_VECTOR,	sysvec_tdg_event_notify);
+#endif
+
 #undef X86_TRAP_OTHER
 
 #endif
diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
index 889f8b1b5b7f..a1550f237ef6 100644
--- a/arch/x86/include/asm/irq_vectors.h
+++ b/arch/x86/include/asm/irq_vectors.h
@@ -104,7 +104,12 @@
 #define HYPERV_STIMER0_VECTOR		0xed
 #endif
 
-#define LOCAL_TIMER_VECTOR		0xec
+#if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)
+/* Vector on which TDX Guest event notification is delivered */
+#define TDX_GUEST_EVENT_NOTIFY_VECTOR	0xec
+#endif
+
+#define LOCAL_TIMER_VECTOR		0xeb
 
 #define NR_VECTORS			 256
 
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 1599aa4850e5..a7ebc6e448d7 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -100,6 +100,8 @@ int tdx_mcall_tdreport(u64 data, u64 reportdata);
 
 int tdx_hcall_get_quote(u64 data);
 
+extern void (*tdg_event_notify_handler)(void);
+
 /*
  * To support I/O port access in decompressor or early kernel init
  * code, since #VE exception handler cannot be used, use paravirt
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 669869bd46ec..a4fe53c8c18f 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -182,11 +182,18 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 			   irq_stats(j)->kvm_posted_intr_wakeup_ipis);
 	seq_puts(p, "  Posted-interrupt wakeup event\n");
 #endif
+
 #if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)
 	seq_printf(p, "%*s: ", prec, "TGV");
 	for_each_online_cpu(j)
 		seq_printf(p, "%10u ", irq_stats(j)->tdg_ve_count);
 	seq_puts(p, "  TDX Guest VE event\n");
+
+	seq_printf(p, "%*s: ", prec, "TGN");
+	for_each_online_cpu(j)
+		seq_printf(p, "%10u ",
+			   irq_stats(j)->irq_tdg_event_notify_count);
+	seq_puts(p, "  TDX Guest event notification\n");
 #endif
 	return 0;
 }
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index fcb0ed70ea19..f08fd6ae44b3 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -11,6 +11,11 @@
 #include <asm/tdx.h>
 #include <asm/cmdline.h>
 #include <asm/i8259.h>
+#include <asm/apic.h>
+#include <asm/idtentry.h>
+#include <asm/irq_regs.h>
+#include <asm/desc.h>
+#include <asm/idtentry.h>
 #include <asm/vmx.h>
 #include <asm/insn.h>
 #include <asm/insn-eval.h>
@@ -52,6 +57,14 @@ static struct {
 	unsigned long attributes;
 } td_info __ro_after_init;
 
+/*
+ * Currently it will be used only by the attestation
+ * driver. So, race condition with read/write operation
+ * is not considered.
+ */
+void (*tdg_event_notify_handler)(void);
+EXPORT_SYMBOL_GPL(tdg_event_notify_handler);
+
 /*
  * Wrapper for standard use of __tdx_hypercall with BUG_ON() check
  * for TDCALL error.
@@ -150,6 +163,28 @@ static bool tdg_perfmon_enabled(void)
 	return td_info.attributes & BIT(63);
 }
 
+/* TDX guest event notification handler */
+DEFINE_IDTENTRY_SYSVEC(sysvec_tdg_event_notify)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	inc_irq_stat(irq_tdg_event_notify_count);
+
+	if (tdg_event_notify_handler)
+		tdg_event_notify_handler();
+
+	/*
+	 * The hypervisor requires that the APIC EOI should be acked.
+	 * If the APIC EOI is not acked, the APIC ISR bit for the
+	 * TDX_GUEST_EVENT_NOTIFY_VECTOR will not be cleared and then it
+	 * will block the interrupt whose vector is lower than
+	 * TDX_GUEST_EVENT_NOTIFY_VECTOR.
+	 */
+	ack_APIC_irq();
+
+	set_irq_regs(old_regs);
+}
+
 /*
  * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
  *
@@ -712,5 +747,11 @@ void __init tdx_early_init(void)
 		lock_kernel_down("TDX guest init", lockdown_reason);
 	}
 
+	alloc_intr_gate(TDX_GUEST_EVENT_NOTIFY_VECTOR,
+			asm_sysvec_tdg_event_notify);
+
+	if (tdx_hcall_set_notify_intr(TDX_GUEST_EVENT_NOTIFY_VECTOR))
+		pr_warn("Setting event notification interrupt failed\n");
+
 	pr_info("Guest initialized\n");
 }
-- 
2.25.1

[PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kuppuswamy Sathyanarayanan]

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 <ak@linux.intel.com>
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

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 60592fb88e7a..7d01c473aef6 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1301,6 +1301,15 @@ config INTEL_SCU_IPC_UTIL
 	  low level access for debug work and updating the firmware. Say
 	  N unless you will be doing this on an Intel MID platform.
 
+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.
+
 config INTEL_TELEMETRY
 	tristate "Intel SoC Telemetry Driver"
 	depends on X86_64
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index dcc8cdb95b4d..83439990ae47 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -138,6 +138,7 @@ obj-$(CONFIG_INTEL_SCU_PCI)		+= intel_scu_pcidrv.o
 obj-$(CONFIG_INTEL_SCU_PLATFORM)	+= intel_scu_pltdrv.o
 obj-$(CONFIG_INTEL_SCU_WDT)		+= intel_scu_wdt.o
 obj-$(CONFIG_INTEL_SCU_IPC_UTIL)	+= intel_scu_ipcutil.o
+obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
 obj-$(CONFIG_INTEL_TELEMETRY)		+= intel_telemetry_core.o \
 					   intel_telemetry_pltdrv.o \
 					   intel_telemetry_debugfs.o
diff --git a/drivers/platform/x86/intel_tdx_attest.c b/drivers/platform/x86/intel_tdx_attest.c
new file mode 100644
index 000000000000..a0225d053851
--- /dev/null
+++ b/drivers/platform/x86/intel_tdx_attest.c
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel_tdx_attest.c - TDX guest attestation interface driver.
+ *
+ * Implements user interface to trigger attestation process and
+ * read the TD Quote result.
+ *
+ * Copyright (C) 2020 Intel Corporation
+ *
+ * Author:
+ *     Kuppuswamy Sathyanarayanan <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 */
+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));
+
+		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));
+
+	return !file->private_data ? -ENOMEM : 0;
+}
+
+static int tdg_attest_release(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, NULL);
+	free_pages((unsigned long)file->private_data, get_order(QUOTE_SIZE));
+	file->private_data = NULL;
+
+	return 0;
+}
+
+static const struct file_operations tdg_attest_fops = {
+	.owner		= THIS_MODULE,
+	.open		= tdg_attest_open,
+	.release	= tdg_attest_release,
+	.unlocked_ioctl	= tdg_attest_ioctl,
+	.llseek		= no_llseek,
+};
+
+static struct miscdevice tdg_attest_device = {
+	.minor          = MISC_DYNAMIC_MINOR,
+	.name           = "tdx-attest",
+	.fops           = &tdg_attest_fops,
+};
+module_misc_device(tdg_attest_device);
+
+MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
+MODULE_DESCRIPTION("TDX attestation driver ver " VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/misc/tdx.h b/include/uapi/misc/tdx.h
new file mode 100644
index 000000000000..59e6561c0892
--- /dev/null
+++ b/include/uapi/misc/tdx.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_MISC_TDX_H
+#define _UAPI_MISC_TDX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
+#define TDX_REPORT_DATA_LEN		64
+
+/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
+#define TDX_TDREPORT_LEN		1024
+
+/*
+ * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
+ * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
+ * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
+ * TDREPORT data is copied to the user buffer.
+ */
+#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
+
+/*
+ * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
+ * should pass report data of size TDX_REPORT_DATA_LEN bytes via user input
+ * buffer of quote size. Once IOCTL is successful quote data is copied back to
+ * the user buffer.
+ */
+#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
+
+/*
+ * TDX_CMD_GET_QUOTE_SIZE IOCTL is used to get the TD Quote size info in bytes.
+ * This will be used for determining the input buffer allocation size when
+ * using TDX_CMD_GEN_QUOTE IOCTL.
+ */
+#define TDX_CMD_GET_QUOTE_SIZE		_IOR('T', 0x03, __u64)
+
+#endif /* _UAPI_MISC_TDX_H */
-- 
2.25.1

[PATCH v2 6/6] tools/tdx: Add a sample attestation user app

[Kuppuswamy Sathyanarayanan]

This application uses the misc device /dev/tdx-attest to get TDREPORT
from the TDX Module or request quote from the VMM.

It tests following attestation features:

  - Get report using TDX_CMD_GET_TDREPORT IOCTL.
  - Using report data request quote from VMM using TDX_CMD_GEN_QUOTE IOCTL.
  - Get the quote size using TDX_CMD_GET_QUOTE_SIZE IOCTL.

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>
---
 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 +++++++++++++++++++++++++++++
 5 files changed, 284 insertions(+), 6 deletions(-)
 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

diff --git a/tools/Makefile b/tools/Makefile
index 7e9d34ddd74c..5d68084511cb 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -30,6 +30,7 @@ help:
 	@echo '  selftests              - various kernel selftests'
 	@echo '  bootconfig             - boot config tool'
 	@echo '  spi                    - spi tools'
+	@echo '  tdx                    - TDX related test tools'
 	@echo '  tmon                   - thermal monitoring and tuning tool'
 	@echo '  tracing                - misc tracing tools'
 	@echo '  turbostat              - Intel CPU idle stats and freq reporting tool'
@@ -65,7 +66,7 @@ acpi: FORCE
 cpupower: FORCE
 	$(call descend,power/$@)
 
-cgroup firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging tracing: FORCE
+cgroup firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging tracing tdx: FORCE
 	$(call descend,$@)
 
 bpf/%: FORCE
@@ -104,7 +105,7 @@ all: acpi cgroup cpupower gpio hv firewire liblockdep \
 		perf selftests bootconfig spi turbostat usb \
 		virtio vm bpf x86_energy_perf_policy \
 		tmon freefall iio objtool kvm_stat wmi \
-		pci debugging tracing
+		pci debugging tracing tdx
 
 acpi_install:
 	$(call descend,power/$(@:_install=),install)
@@ -112,7 +113,7 @@ acpi_install:
 cpupower_install:
 	$(call descend,power/$(@:_install=),install)
 
-cgroup_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install:
+cgroup_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install tdx_install:
 	$(call descend,$(@:_install=),install)
 
 liblockdep_install:
@@ -139,7 +140,7 @@ install: acpi_install cgroup_install cpupower_install gpio_install \
 		virtio_install vm_install bpf_install x86_energy_perf_policy_install \
 		tmon_install freefall_install objtool_install kvm_stat_install \
 		wmi_install pci_install debugging_install intel-speed-select_install \
-		tracing_install
+		tracing_install tdx_install
 
 acpi_clean:
 	$(call descend,power/acpi,clean)
@@ -147,7 +148,7 @@ acpi_clean:
 cpupower_clean:
 	$(call descend,power/cpupower,clean)
 
-cgroup_clean hv_clean firewire_clean bootconfig_clean spi_clean usb_clean virtio_clean vm_clean wmi_clean bpf_clean iio_clean gpio_clean objtool_clean leds_clean pci_clean firmware_clean debugging_clean tracing_clean:
+cgroup_clean hv_clean firewire_clean bootconfig_clean spi_clean usb_clean virtio_clean vm_clean wmi_clean bpf_clean iio_clean gpio_clean objtool_clean leds_clean pci_clean firmware_clean debugging_clean tracing_clean tdx_clean:
 	$(call descend,$(@:_clean=),clean)
 
 liblockdep_clean:
@@ -186,6 +187,6 @@ clean: acpi_clean cgroup_clean cpupower_clean hv_clean firewire_clean \
 		vm_clean bpf_clean iio_clean x86_energy_perf_policy_clean tmon_clean \
 		freefall_clean build_clean libbpf_clean libsubcmd_clean liblockdep_clean \
 		gpio_clean objtool_clean leds_clean wmi_clean pci_clean firmware_clean debugging_clean \
-		intel-speed-select_clean tracing_clean
+		intel-speed-select_clean tracing_clean tdx_clean
 
 .PHONY: FORCE
diff --git a/tools/tdx/Makefile b/tools/tdx/Makefile
new file mode 100644
index 000000000000..e2564557d463
--- /dev/null
+++ b/tools/tdx/Makefile
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0
+include ../scripts/Makefile.include
+
+all: attest
+
+clean: attest_clean
+
+install: attest_install
+
+attest:
+	$(call descend,attest)
+
+attest_install:
+	$(call descend,attest,install)
+
+attest_clean:
+	$(call descend,attest,clean)
+
+.PHONY: all install clean attest latency_install latency_clean
diff --git a/tools/tdx/attest/.gitignore b/tools/tdx/attest/.gitignore
new file mode 100644
index 000000000000..5f819a8a6c49
--- /dev/null
+++ b/tools/tdx/attest/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+tdx-attest-test
diff --git a/tools/tdx/attest/Makefile b/tools/tdx/attest/Makefile
new file mode 100644
index 000000000000..bf47ba718386
--- /dev/null
+++ b/tools/tdx/attest/Makefile
@@ -0,0 +1,24 @@
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for vm tools
+#
+VAR_CFLAGS := $(shell pkg-config --cflags libtracefs 2>/dev/null)
+VAR_LDLIBS := $(shell pkg-config --libs libtracefs 2>/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 -m 755 -p $(TARGETS) $(DESTDIR)$(sbindir)
diff --git a/tools/tdx/attest/tdx-attest-test.c b/tools/tdx/attest/tdx-attest-test.c
new file mode 100644
index 000000000000..7634ec6a084c
--- /dev/null
+++ b/tools/tdx/attest/tdx-attest-test.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * tdx-attest-test.c - utility to test TDX attestation feature.
+ *
+ * Copyright (C) 2020 - 2021 Intel Corporation. All rights reserved.
+ *
+ * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
+ *
+ */
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <stdio.h>
+#include <ctype.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <getopt.h>
+#include <stdint.h> /* uintmax_t */
+#include <sys/mman.h>
+#include <unistd.h> /* sysconf */
+#include <time.h>
+
+#include "../../../include/uapi/misc/tdx.h"
+
+#define devname		"/dev/tdx-attest"
+
+#define HEX_DUMP_SIZE	16
+#define MAX_ROW_SIZE	70
+
+#define ATTESTATION_TEST_BIN_VERSION "0.1"
+
+struct tdx_attest_args {
+	bool is_dump_data;
+	bool is_get_tdreport;
+	bool is_get_quote_size;
+	bool is_gen_quote;
+	bool debug_mode;
+	char *out_file;
+};
+
+static void print_hex_dump(const char *title, const char *prefix_str,
+			   const void *buf, int len)
+{
+	const __u8 *ptr = buf;
+	int i, rowsize = HEX_DUMP_SIZE;
+
+	if (!len || !buf)
+		return;
+
+	printf("\t\t%s", title);
+
+	for (i = 0; i < len; i++) {
+		if (!(i % rowsize))
+			printf("\n%s%.8x:", prefix_str, i);
+		printf(" %.2x", ptr[i])
+	}
+
+	printf("\n");
+}
+
+static void gen_report_data(__u8 *report_data, bool dump_data)
+{
+	int i;
+
+	srand(time(NULL));
+
+	for (i = 0; i < TDX_REPORT_DATA_LEN; i++)
+		report_data[i] = rand();
+
+	if (dump_data)
+		print_hex_dump("\n\t\tTDX report data\n", " ",
+			       report_data, TDX_REPORT_DATA_LEN);
+}
+
+static int get_tdreport(int devfd, bool dump_data, __u8 *report_data)
+{
+	__u8 tdrdata[TDX_TDREPORT_LEN] = {0};
+	int ret;
+
+	if (!report_data)
+		report_data = tdrdata;
+
+	gen_report_data(report_data, dump_data);
+
+	ret = ioctl(devfd, TDX_CMD_GET_TDREPORT, report_data);
+	if (ret) {
+		printf("TDX_CMD_GET_TDREPORT ioctl() %d failed\n", ret);
+		return -EIO;
+	}
+
+	if (dump_data)
+		print_hex_dump("\n\t\tTDX tdreport data\n", " ", report_data,
+			       TDX_TDREPORT_LEN);
+
+	return 0;
+}
+
+static __u64 get_quote_size(int devfd)
+{
+	int ret;
+	__u64 quote_size;
+
+	ret = ioctl(devfd, TDX_CMD_GET_QUOTE_SIZE, &quote_size);
+	if (ret) {
+		printf("TDX_CMD_GET_QUOTE_SIZE ioctl() %d failed\n", ret);
+		return -EIO;
+	}
+
+	printf("Quote size: %lld\n", quote_size);
+
+	return quote_size;
+}
+
+static int gen_quote(int devfd, bool dump_data)
+{
+	__u8 *quote_data;
+	__u64 quote_size;
+	int ret;
+
+	quote_size = get_quote_size(devfd);
+
+	quote_data = malloc(sizeof(char) * quote_size);
+	if (!quote_data) {
+		printf("%s queue data alloc failed\n", devname);
+		return -ENOMEM;
+	}
+
+	ret = get_tdreport(devfd, dump_data, quote_data);
+	if (ret) {
+		printf("TDX_CMD_GET_TDREPORT ioctl() %d failed\n", ret);
+		goto done;
+	}
+
+	ret = ioctl(devfd, TDX_CMD_GEN_QUOTE, quote_data);
+	if (ret) {
+		printf("TDX_CMD_GEN_QUOTE ioctl() %d failed\n", ret);
+		goto done;
+	}
+
+	print_hex_dump("\n\t\tTDX Quote MMIO data\n", " ", quote_data,
+		       quote_size);
+
+done:
+	free(quote_data);
+
+	return ret;
+}
+
+static void usage(void)
+{
+	puts("\nUsage:\n");
+	puts("tdx_attest [options] \n");
+
+	puts("Attestation device test utility.");
+
+	puts("\nOptions:\n");
+	puts(" -d, --dump                Dump tdreport/tdquote data");
+	puts(" -r, --get-tdreport        Get TDREPORT data");
+	puts(" -g, --gen-quote           Generate TDQUOTE");
+	puts(" -s, --get-quote-size      Get TDQUOTE size");
+}
+
+int main(int argc, char **argv)
+{
+	int ret, devfd;
+	struct tdx_attest_args args = {0};
+
+	static const struct option longopts[] = {
+		{ "dump",           no_argument,       NULL, 'd' },
+		{ "get-tdreport",   required_argument, NULL, 'r' },
+		{ "gen-quote",      required_argument, NULL, 'g' },
+		{ "gen-quote-size", required_argument, NULL, 's' },
+		{ "version",        no_argument,       NULL, 'V' },
+		{ NULL,             0, NULL, 0 }
+	};
+
+	while ((ret = getopt_long(argc, argv, "hdrgsV", longopts,
+				  NULL)) != -1) {
+		switch (ret) {
+		case 'd':
+			args.is_dump_data = true;
+			break;
+		case 'r':
+			args.is_get_tdreport = true;
+			break;
+		case 'g':
+			args.is_gen_quote = true;
+			break;
+		case 's':
+			args.is_get_quote_size = true;
+			break;
+		case 'h':
+			usage();
+			return 0;
+		case 'V':
+			printf("Version: %s\n", ATTESTATION_TEST_BIN_VERSION);
+			return 0;
+		default:
+			printf("Invalid options\n");
+			usage();
+			return -EINVAL;
+		}
+	}
+
+	devfd = open(devname, O_RDWR | O_SYNC);
+	if (devfd < 0) {
+		printf("%s open() failed\n", devname);
+		return -ENODEV;
+	}
+
+	if (args.is_get_quote_size)
+		get_quote_size(devfd);
+
+	if (args.is_get_tdreport)
+		get_tdreport(devfd, args.is_dump_data, NULL);
+
+	if (args.is_gen_quote)
+		gen_quote(devfd, args.is_dump_data);
+
+	close(devfd);
+
+	return 0;
+}
-- 
2.25.1

Re: [PATCH v2 1/6] x86/tdx: Add TDREPORT TDX Module call support

[Xiaoyao Li]

On 7/8/2021 4:42 AM, Kuppuswamy Sathyanarayanan wrote:
> 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 <ak@linux.intel.com>
> 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(+)
> 
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 48927fac9e12..4f1b5c14a09b 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -96,6 +96,8 @@ extern int tdx_hcall_gpa_intent(phys_addr_t gpa, int numpages,
>   
>   bool tdg_filter_enabled(void);
>   
> +int tdx_mcall_tdreport(u64 data, u64 reportdata);
> +
>   /*
>    * To support I/O port access in decompressor or early kernel init
>    * code, since #VE exception handler cannot be used, use paravirt
> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
> index f76af7661046..0f797803f4c8 100644
> --- a/arch/x86/kernel/tdx.c
> +++ b/arch/x86/kernel/tdx.c
> @@ -23,6 +23,7 @@
>   /* TDX Module call Leaf IDs */
>   #define TDINFO				1
>   #define TDGETVEINFO			3
> +#define TDREPORT			4
>   #define TDACCEPTPAGE			6
>   
>   /* TDX hypercall Leaf IDs */
> @@ -30,6 +31,11 @@
>   
>   /* TDX Module call error codes */
>   #define TDX_PAGE_ALREADY_ACCEPTED       0x8000000000000001
> +#define TDCALL_RETURN_CODE_MASK		0xFFFFFFFF00000000
> +#define TDCALL_OPERAND_BUSY		0x8000020000000000
> +#define TDCALL_INVALID_OPERAND		0x8000000000000000
> +#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
> +
>   
>   #define VE_IS_IO_OUT(exit_qual)		(((exit_qual) & 8) ? 0 : 1)
>   #define VE_GET_IO_SIZE(exit_qual)	(((exit_qual) & 7) + 1)
> @@ -139,6 +145,33 @@ static bool tdg_perfmon_enabled(void)
>   	return td_info.attributes & BIT(63);
>   }
>   
> +/*
> + * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
> + *
> + * @data        : Physical address of 1024B aligned data to store
> + *                TDREPORT_STRUCT.
> + * @reportdata  : Physical address of 64B aligned report data
> + *
> + * return 0 on success or failure error number.
> + */
> +int tdx_mcall_tdreport(u64 data, u64 reportdata)
> +{
> +	u64 ret;
> +
> +	if (!data || !reportdata || !prot_guest_has(PR_GUEST_TDX))
> +		return -EINVAL;
> +
> +	ret = __trace_tdx_module_call(TDREPORT, data, reportdata, 0, 0, NULL);
> +
> +	if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> +		return -EINVAL;
> +	else if (TDCALL_RETURN_CODE(ret) == TDCALL_OPERAND_BUSY)
> +		return -EBUSY;

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

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
> +
>   static void tdg_get_info(void)
>   {
>   	u64 ret;
> 

Re: [PATCH v2 1/6] x86/tdx: Add TDREPORT TDX Module call support

[Kuppuswamy, Sathyanarayanan]

On 7/8/21 1:16 AM, Xiaoyao Li wrote:
> 
> 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

Re: [PATCH v2 1/6] x86/tdx: Add TDREPORT TDX Module call support

[Hans de Goede]

Hi,

On 7/8/21 4:07 PM, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 7/8/21 1:16 AM, Xiaoyao Li wrote:
>>
>> 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

Re: [PATCH v2 1/6] x86/tdx: Add TDREPORT TDX Module call support

[Kuppuswamy, Sathyanarayanan]

On 7/8/21 7:20 AM, Hans de Goede wrote:
> 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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Andy Lutomirski]

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>
> ---
>  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
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 60592fb88e7a..7d01c473aef6 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1301,6 +1301,15 @@ config INTEL_SCU_IPC_UTIL
>  	  low level access for debug work and updating the firmware. Say
>  	  N unless you will be doing this on an Intel MID platform.
>  
> +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

> +
>  config INTEL_TELEMETRY
>  	tristate "Intel SoC Telemetry Driver"
>  	depends on X86_64
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index dcc8cdb95b4d..83439990ae47 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -138,6 +138,7 @@ obj-$(CONFIG_INTEL_SCU_PCI)		+= intel_scu_pcidrv.o
>  obj-$(CONFIG_INTEL_SCU_PLATFORM)	+= intel_scu_pltdrv.o
>  obj-$(CONFIG_INTEL_SCU_WDT)		+= intel_scu_wdt.o
>  obj-$(CONFIG_INTEL_SCU_IPC_UTIL)	+= intel_scu_ipcutil.o
> +obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
>  obj-$(CONFIG_INTEL_TELEMETRY)		+= intel_telemetry_core.o \
>  					   intel_telemetry_pltdrv.o \
>  					   intel_telemetry_debugfs.o
> diff --git a/drivers/platform/x86/intel_tdx_attest.c b/drivers/platform/x86/intel_tdx_attest.c
> new file mode 100644
> index 000000000000..a0225d053851
> --- /dev/null
> +++ b/drivers/platform/x86/intel_tdx_attest.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel_tdx_attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process and
> + * read the TD Quote result.
> + *
> + * Copyright (C) 2020 Intel Corporation
> + *
> + * Author:
> + *     Kuppuswamy Sathyanarayanan <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 */
> +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;
> +		}

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?

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

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

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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dave Hansen]

On 7/8/21 3:21 PM, Andy Lutomirski wrote:
>> +		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.

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kuppuswamy, Sathyanarayanan]

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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dan Williams]

On Wed, Jul 7, 2021 at 1:43 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> 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 <ak@linux.intel.com>
> 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
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 60592fb88e7a..7d01c473aef6 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1301,6 +1301,15 @@ config INTEL_SCU_IPC_UTIL
>           low level access for debug work and updating the firmware. Say
>           N unless you will be doing this on an Intel MID platform.
>
> +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.
> +
>  config INTEL_TELEMETRY
>         tristate "Intel SoC Telemetry Driver"
>         depends on X86_64
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index dcc8cdb95b4d..83439990ae47 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -138,6 +138,7 @@ obj-$(CONFIG_INTEL_SCU_PCI)         += intel_scu_pcidrv.o
>  obj-$(CONFIG_INTEL_SCU_PLATFORM)       += intel_scu_pltdrv.o
>  obj-$(CONFIG_INTEL_SCU_WDT)            += intel_scu_wdt.o
>  obj-$(CONFIG_INTEL_SCU_IPC_UTIL)       += intel_scu_ipcutil.o
> +obj-$(CONFIG_INTEL_TDX_ATTESTATION)    += intel_tdx_attest.o
>  obj-$(CONFIG_INTEL_TELEMETRY)          += intel_telemetry_core.o \
>                                            intel_telemetry_pltdrv.o \
>                                            intel_telemetry_debugfs.o
> diff --git a/drivers/platform/x86/intel_tdx_attest.c b/drivers/platform/x86/intel_tdx_attest.c
> new file mode 100644
> index 000000000000..a0225d053851
> --- /dev/null
> +++ b/drivers/platform/x86/intel_tdx_attest.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel_tdx_attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process and
> + * read the TD Quote result.
> + *
> + * Copyright (C) 2020 Intel Corporation
> + *
> + * Author:
> + *     Kuppuswamy Sathyanarayanan <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"

Individual module versions are typically useless as the kernel version
is sufficient.

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

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

> +
> +       file->private_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> +                                                     get_order(QUOTE_SIZE));

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.

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kuppuswamy, Sathyanarayanan]

On 7/8/21 4:36 PM, Dan Williams wrote:
>> +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.

> 
>> +
>> +       file->private_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
>> +                                                     get_order(QUOTE_SIZE));
> 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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dan Williams]

On Thu, Jul 8, 2021 at 4:57 PM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
>
> On 7/8/21 4:36 PM, Dan Williams wrote:
> >> +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.

Those appear to be for core functionality, not one off drivers. Where
is the code that does the SetupEventNotifyInterrupt, is it a driver?

>
> >
> >> +
> >> +       file->private_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> >> +                                                     get_order(QUOTE_SIZE));
> > 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.

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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Andi Kleen]

On 7/8/2021 5:20 PM, Dan Williams wrote:
>
> 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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Andi Kleen]

> 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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dan Williams]

On Thu, Jul 8, 2021 at 5:36 PM Andi Kleen <ak@linux.intel.com> wrote:
>
>
> On 7/8/2021 5:20 PM, Dan Williams wrote:
> >
> > 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.

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.

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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Andi Kleen]

> 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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dan Williams]

On Thu, Jul 8, 2021 at 6:44 PM Andi Kleen <ak@linux.intel.com> wrote:
>
>
> > 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.

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kuppuswamy, Sathyanarayanan]

On 7/8/21 7:04 PM, Dan Williams wrote:
> 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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kuppuswamy, Sathyanarayanan]

Hi Dave,

On 7/8/21 5:38 PM, Andi Kleen wrote:
>> 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.

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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Dave Hansen]

On 7/12/21 5:33 PM, Kuppuswamy, Sathyanarayanan wrote:

> On 7/8/21 5:38 PM, Andi Kleen wrote:
>>> 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.
> 
> 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()?

Re: [PATCH v2 6/6] tools/tdx: Add a sample attestation user app

[Mian Yousaf Kaukab]

On Wed, Jul 07, 2021 at 01:42:49PM -0700, Kuppuswamy Sathyanarayanan wrote:
> This application uses the misc device /dev/tdx-attest to get TDREPORT
> from the TDX Module or request quote from the VMM.
> 
> It tests following attestation features:
> 
>   - Get report using TDX_CMD_GET_TDREPORT IOCTL.
>   - Using report data request quote from VMM using TDX_CMD_GEN_QUOTE IOCTL.
>   - Get the quote size using TDX_CMD_GET_QUOTE_SIZE IOCTL.
> 
> 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>
> ---
>  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 +++++++++++++++++++++++++++++
>  5 files changed, 284 insertions(+), 6 deletions(-)
>  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
> 
> diff --git a/tools/Makefile b/tools/Makefile
> index 7e9d34ddd74c..5d68084511cb 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -30,6 +30,7 @@ help:
>  	@echo '  selftests              - various kernel selftests'
>  	@echo '  bootconfig             - boot config tool'
>  	@echo '  spi                    - spi tools'
> +	@echo '  tdx                    - TDX related test tools'
>  	@echo '  tmon                   - thermal monitoring and tuning tool'
>  	@echo '  tracing                - misc tracing tools'
>  	@echo '  turbostat              - Intel CPU idle stats and freq reporting tool'
> @@ -65,7 +66,7 @@ acpi: FORCE
>  cpupower: FORCE
>  	$(call descend,power/$@)
>  
> -cgroup firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging tracing: FORCE
> +cgroup firewire hv guest bootconfig spi usb virtio vm bpf iio gpio objtool leds wmi pci firmware debugging tracing tdx: FORCE
>  	$(call descend,$@)
>  
>  bpf/%: FORCE
> @@ -104,7 +105,7 @@ all: acpi cgroup cpupower gpio hv firewire liblockdep \
>  		perf selftests bootconfig spi turbostat usb \
>  		virtio vm bpf x86_energy_perf_policy \
>  		tmon freefall iio objtool kvm_stat wmi \
> -		pci debugging tracing
> +		pci debugging tracing tdx
>  
>  acpi_install:
>  	$(call descend,power/$(@:_install=),install)
> @@ -112,7 +113,7 @@ acpi_install:
>  cpupower_install:
>  	$(call descend,power/$(@:_install=),install)
>  
> -cgroup_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install:
> +cgroup_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install tdx_install:
>  	$(call descend,$(@:_install=),install)
>  
>  liblockdep_install:
> @@ -139,7 +140,7 @@ install: acpi_install cgroup_install cpupower_install gpio_install \
>  		virtio_install vm_install bpf_install x86_energy_perf_policy_install \
>  		tmon_install freefall_install objtool_install kvm_stat_install \
>  		wmi_install pci_install debugging_install intel-speed-select_install \
> -		tracing_install
> +		tracing_install tdx_install
>  
>  acpi_clean:
>  	$(call descend,power/acpi,clean)
> @@ -147,7 +148,7 @@ acpi_clean:
>  cpupower_clean:
>  	$(call descend,power/cpupower,clean)
>  
> -cgroup_clean hv_clean firewire_clean bootconfig_clean spi_clean usb_clean virtio_clean vm_clean wmi_clean bpf_clean iio_clean gpio_clean objtool_clean leds_clean pci_clean firmware_clean debugging_clean tracing_clean:
> +cgroup_clean hv_clean firewire_clean bootconfig_clean spi_clean usb_clean virtio_clean vm_clean wmi_clean bpf_clean iio_clean gpio_clean objtool_clean leds_clean pci_clean firmware_clean debugging_clean tracing_clean tdx_clean:
>  	$(call descend,$(@:_clean=),clean)
>  
>  liblockdep_clean:
> @@ -186,6 +187,6 @@ clean: acpi_clean cgroup_clean cpupower_clean hv_clean firewire_clean \
>  		vm_clean bpf_clean iio_clean x86_energy_perf_policy_clean tmon_clean \
>  		freefall_clean build_clean libbpf_clean libsubcmd_clean liblockdep_clean \
>  		gpio_clean objtool_clean leds_clean wmi_clean pci_clean firmware_clean debugging_clean \
> -		intel-speed-select_clean tracing_clean
> +		intel-speed-select_clean tracing_clean tdx_clean
>  
>  .PHONY: FORCE
> diff --git a/tools/tdx/Makefile b/tools/tdx/Makefile
> new file mode 100644
> index 000000000000..e2564557d463
> --- /dev/null
> +++ b/tools/tdx/Makefile
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0
> +include ../scripts/Makefile.include
> +
> +all: attest
> +
> +clean: attest_clean
> +
> +install: attest_install
> +
> +attest:
> +	$(call descend,attest)
> +
> +attest_install:
> +	$(call descend,attest,install)
> +
> +attest_clean:
> +	$(call descend,attest,clean)
> +
> +.PHONY: all install clean attest latency_install latency_clean
> diff --git a/tools/tdx/attest/.gitignore b/tools/tdx/attest/.gitignore
> new file mode 100644
> index 000000000000..5f819a8a6c49
> --- /dev/null
> +++ b/tools/tdx/attest/.gitignore
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +tdx-attest-test
> diff --git a/tools/tdx/attest/Makefile b/tools/tdx/attest/Makefile
> new file mode 100644
> index 000000000000..bf47ba718386
> --- /dev/null
> +++ b/tools/tdx/attest/Makefile
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Makefile for vm tools
> +#
> +VAR_CFLAGS := $(shell pkg-config --cflags libtracefs 2>/dev/null)
> +VAR_LDLIBS := $(shell pkg-config --libs libtracefs 2>/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 -m 755 -p $(TARGETS) $(DESTDIR)$(sbindir)
> diff --git a/tools/tdx/attest/tdx-attest-test.c b/tools/tdx/attest/tdx-attest-test.c
> new file mode 100644
> index 000000000000..7634ec6a084c
> --- /dev/null
> +++ b/tools/tdx/attest/tdx-attest-test.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * tdx-attest-test.c - utility to test TDX attestation feature.
> + *
> + * Copyright (C) 2020 - 2021 Intel Corporation. All rights reserved.
> + *
> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> + *
> + */
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <stdio.h>
> +#include <ctype.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <limits.h>
> +#include <stdbool.h>
> +#include <getopt.h>
> +#include <stdint.h> /* uintmax_t */
> +#include <sys/mman.h>
> +#include <unistd.h> /* sysconf */
> +#include <time.h>
> +
> +#include "../../../include/uapi/misc/tdx.h"
> +
> +#define devname		"/dev/tdx-attest"
> +
> +#define HEX_DUMP_SIZE	16
> +#define MAX_ROW_SIZE	70
> +
> +#define ATTESTATION_TEST_BIN_VERSION "0.1"
> +
> +struct tdx_attest_args {
> +	bool is_dump_data;
> +	bool is_get_tdreport;
> +	bool is_get_quote_size;
> +	bool is_gen_quote;
> +	bool debug_mode;
> +	char *out_file;
> +};
> +
> +static void print_hex_dump(const char *title, const char *prefix_str,
> +			   const void *buf, int len)
> +{
> +	const __u8 *ptr = buf;
> +	int i, rowsize = HEX_DUMP_SIZE;
> +
> +	if (!len || !buf)
> +		return;
> +
> +	printf("\t\t%s", title);
> +
> +	for (i = 0; i < len; i++) {
> +		if (!(i % rowsize))
> +			printf("\n%s%.8x:", prefix_str, i);
> +		printf(" %.2x", ptr[i])
> +	}
> +
> +	printf("\n");
> +}
> +
> +static void gen_report_data(__u8 *report_data, bool dump_data)
> +{
> +	int i;
> +
> +	srand(time(NULL));
> +
> +	for (i = 0; i < TDX_REPORT_DATA_LEN; i++)
> +		report_data[i] = rand();
> +
> +	if (dump_data)
> +		print_hex_dump("\n\t\tTDX report data\n", " ",
> +			       report_data, TDX_REPORT_DATA_LEN);
> +}
> +
> +static int get_tdreport(int devfd, bool dump_data, __u8 *report_data)
> +{
> +	__u8 tdrdata[TDX_TDREPORT_LEN] = {0};
> +	int ret;
> +
> +	if (!report_data)
> +		report_data = tdrdata;
> +
> +	gen_report_data(report_data, dump_data);
> +
> +	ret = ioctl(devfd, TDX_CMD_GET_TDREPORT, report_data);
> +	if (ret) {
> +		printf("TDX_CMD_GET_TDREPORT ioctl() %d failed\n", ret);
> +		return -EIO;
> +	}
> +
> +	if (dump_data)
> +		print_hex_dump("\n\t\tTDX tdreport data\n", " ", report_data,
> +			       TDX_TDREPORT_LEN);
> +
> +	return 0;
> +}
> +
> +static __u64 get_quote_size(int devfd)
> +{
> +	int ret;
> +	__u64 quote_size;
> +
> +	ret = ioctl(devfd, TDX_CMD_GET_QUOTE_SIZE, &quote_size);
> +	if (ret) {
> +		printf("TDX_CMD_GET_QUOTE_SIZE ioctl() %d failed\n", ret);
> +		return -EIO;
> +	}
> +
> +	printf("Quote size: %lld\n", quote_size);
> +
> +	return quote_size;
> +}
> +
> +static int gen_quote(int devfd, bool dump_data)
> +{
> +	__u8 *quote_data;
> +	__u64 quote_size;
> +	int ret;
> +
> +	quote_size = get_quote_size(devfd);
> +
> +	quote_data = malloc(sizeof(char) * quote_size);
> +	if (!quote_data) {
> +		printf("%s queue data alloc failed\n", devname);
> +		return -ENOMEM;
> +	}
> +
> +	ret = get_tdreport(devfd, dump_data, quote_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()?
> +	if (ret) {
> +		printf("TDX_CMD_GET_TDREPORT ioctl() %d failed\n", ret);
> +		goto done;
> +	}
> +
> +	ret = ioctl(devfd, TDX_CMD_GEN_QUOTE, quote_data);
> +	if (ret) {
> +		printf("TDX_CMD_GEN_QUOTE ioctl() %d failed\n", ret);
> +		goto done;
> +	}
> +
> +	print_hex_dump("\n\t\tTDX Quote MMIO data\n", " ", quote_data,
> +		       quote_size);
> +
> +done:
> +	free(quote_data);
> +
> +	return ret;
> +}
> +
> +static void usage(void)
> +{
> +	puts("\nUsage:\n");
> +	puts("tdx_attest [options] \n");
> +
> +	puts("Attestation device test utility.");
> +
> +	puts("\nOptions:\n");
> +	puts(" -d, --dump                Dump tdreport/tdquote data");
> +	puts(" -r, --get-tdreport        Get TDREPORT data");
> +	puts(" -g, --gen-quote           Generate TDQUOTE");
> +	puts(" -s, --get-quote-size      Get TDQUOTE size");
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int ret, devfd;
> +	struct tdx_attest_args args = {0};
> +
> +	static const struct option longopts[] = {
> +		{ "dump",           no_argument,       NULL, 'd' },
> +		{ "get-tdreport",   required_argument, NULL, 'r' },
> +		{ "gen-quote",      required_argument, NULL, 'g' },
> +		{ "gen-quote-size", required_argument, NULL, 's' },
> +		{ "version",        no_argument,       NULL, 'V' },
> +		{ NULL,             0, NULL, 0 }
> +	};
> +
> +	while ((ret = getopt_long(argc, argv, "hdrgsV", longopts,
> +				  NULL)) != -1) {
> +		switch (ret) {
> +		case 'd':
> +			args.is_dump_data = true;
> +			break;
> +		case 'r':
> +			args.is_get_tdreport = true;
> +			break;
> +		case 'g':
> +			args.is_gen_quote = true;
> +			break;
> +		case 's':
> +			args.is_get_quote_size = true;
> +			break;
> +		case 'h':
> +			usage();
> +			return 0;
> +		case 'V':
> +			printf("Version: %s\n", ATTESTATION_TEST_BIN_VERSION);
> +			return 0;
> +		default:
> +			printf("Invalid options\n");
> +			usage();
> +			return -EINVAL;
> +		}
> +	}
> +
> +	devfd = open(devname, O_RDWR | O_SYNC);
> +	if (devfd < 0) {
> +		printf("%s open() failed\n", devname);
> +		return -ENODEV;
> +	}
> +
> +	if (args.is_get_quote_size)
> +		get_quote_size(devfd);
> +
> +	if (args.is_get_tdreport)
> +		get_tdreport(devfd, args.is_dump_data, NULL);
> +
> +	if (args.is_gen_quote)
> +		gen_quote(devfd, args.is_dump_data);
> +
> +	close(devfd);
> +
> +	return 0;
> +}
> -- 
> 2.25.1

BR,
Yousaf

Re: [PATCH v2 6/6] tools/tdx: Add a sample attestation user app

[Kuppuswamy, Sathyanarayanan]

On 7/15/21 1:36 AM, Mian Yousaf Kaukab wrote:
> 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

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Hans de Goede]

Hi,

On 4/4/22 21:56, Sathyanarayanan Kuppuswamy wrote:
> Hi Hans,
> 
> On 4/4/22 3:07 AM, Hans de Goede wrote:
>>> +static int __init tdx_attest_init(void)
>>> +{
>>> +    dma_addr_t handle;
>>> +    long ret = 0;
>>> +
>>> +    mutex_lock(&attestation_lock);
>>> +
>>> +    ret = misc_register(&tdx_attest_device);
>>> +    if (ret) {
>>> +        pr_err("misc device registration failed\n");
>>> +        mutex_unlock(&attestation_lock);
>>> +        return ret;
>>> +    }
>> Why not do this as the last thing of the probe?
> 
> We need misc device reference in dma_alloc_coherent() and
> dma_set_coherent_mask() calls. This is the reason for keeping
> misc_register() at the beginining of the init function.

Erm, you are supposed to pass an actual device-device as
parameter to dma_alloc_coherent(), so that it can see
what bus/dma-domain the device is connected to and if
an ioMMU might be involved...

>> That will avoid the need to unregister this again in all
>> the error-exit paths and also fixes a possible deadlock.
>>
> 
> Agree. But, unless we create another device locally, I don't
> think we can avoid this. Do you prefer this approach?

Yes, passing the "struct device" which is embedded inside
a miscdevice as device to dma_alloc_coherent() is just
wrong. Please make your module_init function register
a platform_device using platform_device_register_simple()
(on systems with TDX support) and then turn your code/driver
into a standard platform_driver using the platform_device
which the driver binds to as parameter to dma_alloc_coherent().

See: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?id=c4d2ff35d350428eca2ae1a1e2119e4c6297811d

for a simple (completely unrelated) driver which uses this
method to have a device to bind to for talking to a secondary
function of the embedded-controller on some platforms.

Regards,

Hans

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Hans,

On 4/4/22 3:07 AM, Hans de Goede wrote:
>> +static int __init tdx_attest_init(void)
>> +{
>> +	dma_addr_t handle;
>> +	long ret = 0;
>> +
>> +	mutex_lock(&attestation_lock);
>> +
>> +	ret = misc_register(&tdx_attest_device);
>> +	if (ret) {
>> +		pr_err("misc device registration failed\n");
>> +		mutex_unlock(&attestation_lock);
>> +		return ret;
>> +	}
> Why not do this as the last thing of the probe?

We need misc device reference in dma_alloc_coherent() and
dma_set_coherent_mask() calls. This is the reason for keeping
misc_register() at the beginining of the init function.

> 
> That will avoid the need to unregister this again in all
> the error-exit paths and also fixes a possible deadlock.
> 

Agree. But, unless we create another device locally, I don't
think we can avoid this. Do you prefer this approach?

> Right now you possibly have:
> 
> 1. probe() locks attestation_lock
> 2. probe() registers misc-device
> 3. userspace calls tdx_attest_ioctl
> 4. tdx_attest_ioctl blocks waiting for attestastion_lock
> 5. Something goes wrong in probe, probe calls
>     misc_deregister()
> 6. misc_deregister waits for the ioctl to finish
> 7. deadlock
> 
> I'm not sure about 6, but if 6 does not happen then
> instead we now have tdx_attest_ioctl running
> after the misc_deregister, with tdquote_data and
> tdreport_data as NULL, or pointing to free-ed memory
> leading to various crash scenarios.

Makes sense. But as I have mentioned above, we have reason
for keeping the misc_register() at the begining of the
init function.

One way to avoid this deadlock is to use global initalization
check.

--- a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
+++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
@@ -48,6 +48,8 @@ static void *tdreport_data;
  /* DMA handle used to allocate and free tdquote DMA buffer */
  dma_addr_t tdquote_dma_handle;

+static bool device_initialized;
+
  static void attestation_callback_handler(void)
  {
         complete(&attestation_done);
@@ -60,6 +62,9 @@ static long tdx_attest_ioctl(struct file *file, 
unsigned int cmd,
         struct tdx_gen_quote tdquote_req;
         long ret = 0;

+       if (!device_initialized)
+               return -ENODEV;
+
         mutex_lock(&attestation_lock);

         switch (cmd) {
@@ -191,6 +196,8 @@ static int __init tdx_attest_init(void)

         mutex_unlock(&attestation_lock);

+       device_initialized = true;
+
         pr_debug("module initialization success\n");

         return 0;

Please let me know your comment on above solution.

> 
> TL;DR: you must always delay registering any
> interfaces for userspace until your code is
> ready to deal with userspace calls.
> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> As I mentioned with v1:
> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Hans de Goede]

Hi,

On 4/4/22 12:09, Hans de Goede wrote:
> <hit send to soon, now with a complete p.s. section>
> 
> Hi,
> 
> On 3/31/22 00:18, Kuppuswamy Sathyanarayanan wrote:
>> 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. 
>>
>> [Chenyi Qiang: Proposed struct tdx_gen_quote for passing user buffer]
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>  drivers/platform/x86/intel/Kconfig            |   1 +
>>  drivers/platform/x86/intel/Makefile           |   1 +
>>  drivers/platform/x86/intel/tdx/Kconfig        |  13 +
>>  drivers/platform/x86/intel/tdx/Makefile       |   3 +
>>  .../platform/x86/intel/tdx/intel_tdx_attest.c | 230 ++++++++++++++++++
>>  include/uapi/misc/tdx.h                       |  42 ++++
>>  6 files changed, 290 insertions(+)
>>  create mode 100644 drivers/platform/x86/intel/tdx/Kconfig
>>  create mode 100644 drivers/platform/x86/intel/tdx/Makefile
>>  create mode 100644 drivers/platform/x86/intel/tdx/intel_tdx_attest.c
>>  create mode 100644 include/uapi/misc/tdx.h
>>
>> diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
>> index 8e65086bb6c8..a2ed17d67052 100644
>> --- a/drivers/platform/x86/intel/Kconfig
>> +++ b/drivers/platform/x86/intel/Kconfig
>> @@ -12,6 +12,7 @@ source "drivers/platform/x86/intel/pmt/Kconfig"
>>  source "drivers/platform/x86/intel/speed_select_if/Kconfig"
>>  source "drivers/platform/x86/intel/telemetry/Kconfig"
>>  source "drivers/platform/x86/intel/wmi/Kconfig"
>> +source "drivers/platform/x86/intel/tdx/Kconfig"
>>  
>>  config INTEL_HID_EVENT
>>  	tristate "Intel HID Event"
>> diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
>> index 35f2066578b2..27a6c6c5a83f 100644
>> --- a/drivers/platform/x86/intel/Makefile
>> +++ b/drivers/platform/x86/intel/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_INTEL_SKL_INT3472)		+= int3472/
>>  obj-$(CONFIG_INTEL_PMC_CORE)		+= pmc/
>>  obj-$(CONFIG_INTEL_PMT_CLASS)		+= pmt/
>>  obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += speed_select_if/
>> +obj-$(CONFIG_INTEL_TDX_GUEST)		+= tdx/
>>  obj-$(CONFIG_INTEL_TELEMETRY)		+= telemetry/
>>  obj-$(CONFIG_INTEL_WMI)			+= wmi/
>>  
>> diff --git a/drivers/platform/x86/intel/tdx/Kconfig b/drivers/platform/x86/intel/tdx/Kconfig
>> new file mode 100644
>> index 000000000000..853e3a34c889
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel/tdx/Kconfig
>> @@ -0,0 +1,13 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# X86 TDX Platform Specific Drivers
>> +#
>> +
>> +config INTEL_TDX_ATTESTATION
>> +	tristate "Intel TDX attestation driver"
>> +	depends on INTEL_TDX_GUEST
>> +	help
>> +	  The TDX attestation driver provides IOCTL interfaces to the user to
>> +	  request TDREPORT from the TDX module or request quote from the VMM
>> +	  or to get quote buffer size. It is mainly used to get secure disk
>> +	  decryption keys from the key server.
>> diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
>> new file mode 100644
>> index 000000000000..124d6b7b20a0
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel/tdx/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
>> diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
>> new file mode 100644
>> index 000000000000..0bf78d30e057
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
>> @@ -0,0 +1,230 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * intel_tdx_attest.c - TDX guest attestation interface driver.
>> + *
>> + * Implements user interface to trigger attestation process and
>> + * read the TD Quote result.
>> + *
>> + * Copyright (C) 2021-2022 Intel Corporation
>> + *
>> + * Author:
>> + *     Kuppuswamy Sathyanarayanan <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/dma-mapping.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/io.h>
>> +#include <asm/apic.h>
>> +#include <asm/tdx.h>
>> +#include <asm/irq_vectors.h>
>> +#include <uapi/misc/tdx.h>
>> +
>> +/* Used in Quote memory allocation */
>> +#define QUOTE_SIZE			(2 * PAGE_SIZE)
>> +/* Used in Get Quote request memory allocation */
>> +#define GET_QUOTE_MAX_SIZE		(4 * PAGE_SIZE)
>> +/* Get Quote timeout in msec */
>> +#define GET_QUOTE_TIMEOUT		(5000)
>> +
>> +/* Mutex to synchronize attestation requests */
>> +static DEFINE_MUTEX(attestation_lock);
>> +/* Completion object to track attestation status */
>> +static DECLARE_COMPLETION(attestation_done);
>> +/* Buffer used to copy report data in attestation handler */
>> +static u8 report_data[TDX_REPORT_DATA_LEN] __aligned(64);
>> +/* Data pointer used to get TD Quote data in attestation handler */
>> +static void *tdquote_data;
>> +/* Data pointer used to get TDREPORT data in attestation handler */
>> +static void *tdreport_data;
>> +/* DMA handle used to allocate and free tdquote DMA buffer */
>> +dma_addr_t tdquote_dma_handle;
>> +
>> +static void attestation_callback_handler(void)
>> +{
>> +	complete(&attestation_done);
>> +}
>> +
>> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>> +			     unsigned long arg)
>> +{
>> +	void __user *argp = (void __user *)arg;
>> +	struct tdx_gen_quote tdquote_req;
>> +	long ret = 0;
>> +
>> +	mutex_lock(&attestation_lock);
>> +
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_TDREPORT:
>> +		if (copy_from_user(report_data, argp, TDX_REPORT_DATA_LEN)) {
>> +			ret = -EFAULT;
>> +			break;
>> +		}
>> +
>> +		/* Generate TDREPORT_STRUCT */
>> +		if (tdx_mcall_tdreport(tdreport_data, report_data)) {
>> +			ret = -EIO;
>> +			break;
>> +		}
>> +
>> +		if (copy_to_user(argp, tdreport_data, TDX_TDREPORT_LEN))
>> +			ret = -EFAULT;
>> +		break;
>> +	case TDX_CMD_GEN_QUOTE:
>> +		reinit_completion(&attestation_done);
>> +
>> +		/* Copy TDREPORT data from user buffer */
>> +		if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) {
>> +			ret = -EFAULT;
>> +			break;
>> +		}
>> +
>> +		if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) {
>> +			ret = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		if (copy_from_user(tdquote_data, tdquote_req.buf, tdquote_req.len)) {
>> +			ret = -EFAULT;
>> +			break;
>> +		}
>> +
>> +		/* Submit GetQuote Request */
>> +		if (tdx_hcall_get_quote(tdquote_data, GET_QUOTE_MAX_SIZE)) {
>> +			ret = -EIO;
>> +			break;
>> +		}
>> +
>> +		/* Wait for attestation completion */
>> +		ret = wait_for_completion_interruptible_timeout(
>> +				&attestation_done,
>> +				msecs_to_jiffies(GET_QUOTE_TIMEOUT));
>> +		if (ret <= 0) {
>> +			ret = -EIO;
>> +			break;
>> +		}
>> +
>> +		/* ret will be positive if completed. */
>> +		ret = 0;
>> +
>> +		if (copy_to_user(tdquote_req.buf, tdquote_data, tdquote_req.len))
>> +			ret = -EFAULT;
>> +
>> +		break;
>> +	case TDX_CMD_GET_QUOTE_SIZE:
>> +		ret = put_user(QUOTE_SIZE, (u64 __user *)argp);
>> +		break;
>> +	default:
>> +		pr_err("cmd %d not supported\n", cmd);
>> +		break;
>> +	}
>> +
>> +	mutex_unlock(&attestation_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations tdx_attest_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.unlocked_ioctl	= tdx_attest_ioctl,
>> +	.llseek		= no_llseek,
>> +};
>> +
>> +static struct miscdevice tdx_attest_device = {
>> +	.minor          = MISC_DYNAMIC_MINOR,
>> +	.name           = "tdx-attest",
>> +	.fops           = &tdx_attest_fops,
>> +};
>> +
>> +static int __init tdx_attest_init(void)
>> +{
>> +	dma_addr_t handle;
>> +	long ret = 0;
>> +
>> +	mutex_lock(&attestation_lock);
>> +
>> +	ret = misc_register(&tdx_attest_device);
>> +	if (ret) {
>> +		pr_err("misc device registration failed\n");
>> +		mutex_unlock(&attestation_lock);
>> +		return ret;
>> +	}
> 
> Why not do this as the last thing of the probe?
> 
> That will avoid the need to unregister this again in all
> the error-exit paths and also fixes a possible deadlock.
> 
> Right now you possibly have:
> 
> 1. probe() locks attestation_lock
> 2. probe() registers misc-device
> 3. userspace calls tdx_attest_ioctl
> 4. tdx_attest_ioctl blocks waiting for attestastion_lock
> 5. Something goes wrong in probe, probe calls
>    misc_deregister()
> 6. misc_deregister waits for the ioctl to finish
> 7. deadlock
> 
> I'm not sure about 6, but if 6 does not happen then
> instead we now have tdx_attest_ioctl running
> after the misc_deregister, with tdquote_data and
> tdreport_data as NULL, or pointing to free-ed memory
> leading to various crash scenarios.
> 
> TL;DR: you must always delay registering any
> interfaces for userspace until your code is
> ready to deal with userspace calls.

One more remark about this, if you make the 
misc_register() just before the "return 0;" from
probe() then there also is no need to take the lock
in probe() at all which also simplifies error handling.

Regards,

Hans


> p.s.
> 
> As I mentioned with v1:
> 
> I really know very little about TDX.
> I assume the rest of the series will be reviewed by someone
> with more detailed knowledge of TDX as such I believe it would be good
> if the platform/x86 patch is also reviewed as part of that.
> 
> Since the platform/x86 patch depends on the other patches I believe
> it is also best if the entire series is merged in one go by the x86/tip
> maintainers here is my ack for this:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> 
> 
> 
> 
>> +
>> +	/*
>> +	 * tdreport_data needs to be 64-byte aligned.
>> +	 * Full page alignment is more than enough.
>> +	 */
>> +	tdreport_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 0);
>> +	if (!tdreport_data) {
>> +		ret = -ENOMEM;
>> +		goto failed;
>> +	}
>> +
>> +	ret = dma_set_coherent_mask(tdx_attest_device.this_device,
>> +				    DMA_BIT_MASK(64));
>> +	if (ret) {
>> +		pr_err("dma set coherent mask failed\n");
>> +		goto failed;
>> +	}
>> +
>> +	/* Allocate DMA buffer to get TDQUOTE data from the VMM */
>> +	tdquote_data = dma_alloc_coherent(tdx_attest_device.this_device,
>> +					  GET_QUOTE_MAX_SIZE, &handle,
>> +					  GFP_KERNEL | __GFP_ZERO);
>> +	if (!tdquote_data) {
>> +		ret = -ENOMEM;
>> +		goto failed;
>> +	}
>> +
>> +	tdquote_dma_handle =  handle;
>> +
>> +	/* Register attestation event notify handler */
>> +	tdx_setup_ev_notify_handler(attestation_callback_handler);
>> +
>> +	mutex_unlock(&attestation_lock);
>> +
>> +	pr_debug("module initialization success\n");
>> +
>> +	return 0;
>> +
>> +failed:
>> +	if (tdreport_data)
>> +		free_pages((unsigned long)tdreport_data, 0);
>> +
>> +	misc_deregister(&tdx_attest_device);
>> +
>> +	mutex_unlock(&attestation_lock);
>> +
>> +	pr_debug("module initialization failed\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static void __exit tdx_attest_exit(void)
>> +{
>> +	mutex_lock(&attestation_lock);
>> +
>> +	dma_free_coherent(tdx_attest_device.this_device, GET_QUOTE_MAX_SIZE,
>> +			  tdquote_data, tdquote_dma_handle);
>> +	free_pages((unsigned long)tdreport_data, 0);
>> +	misc_deregister(&tdx_attest_device);
>> +	/* Unregister attestation event notify handler */
>> +	tdx_remove_ev_notify_handler();
>> +	mutex_unlock(&attestation_lock);
>> +	pr_debug("module is successfully removed\n");
>> +}
>> +
>> +module_init(tdx_attest_init);
>> +module_exit(tdx_attest_exit);
>> +
>> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
>> +MODULE_DESCRIPTION("TDX attestation driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/uapi/misc/tdx.h b/include/uapi/misc/tdx.h
>> new file mode 100644
>> index 000000000000..839b9a220022
>> --- /dev/null
>> +++ b/include/uapi/misc/tdx.h
>> @@ -0,0 +1,42 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef _UAPI_MISC_TDX_H
>> +#define _UAPI_MISC_TDX_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/ioctl.h>
>> +
>> +/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
>> +#define TDX_REPORT_DATA_LEN		64
>> +
>> +/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
>> +#define TDX_TDREPORT_LEN		1024
>> +
>> +/*
>> + * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
>> + * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
>> + * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
>> + * TDREPORT data is copied to the user buffer.
>> + */
>> +#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
>> +
>> +/*
>> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
>> + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
>> + * buffer of quote size. Once IOCTL is successful quote data is copied back to
>> + * the user buffer.
>> + */
>> +#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
>> +
>> +/*
>> + * TDX_CMD_GET_QUOTE_SIZE IOCTL is used to get the TD Quote size info in bytes.
>> + * This will be used for determining the input buffer allocation size when
>> + * using TDX_CMD_GEN_QUOTE IOCTL.
>> + */
>> +#define TDX_CMD_GET_QUOTE_SIZE		_IOR('T', 0x03, __u64)
>> +
>> +struct tdx_gen_quote {
>> +	void *buf __user;
>> +	size_t len;
>> +};
>> +
>> +#endif /* _UAPI_MISC_TDX_H */

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Hans de Goede]

<hit send to soon, now with a complete p.s. section>

Hi,

On 3/31/22 00:18, Kuppuswamy Sathyanarayanan wrote:
> 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. 
> 
> [Chenyi Qiang: Proposed struct tdx_gen_quote for passing user buffer]
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel/Kconfig            |   1 +
>  drivers/platform/x86/intel/Makefile           |   1 +
>  drivers/platform/x86/intel/tdx/Kconfig        |  13 +
>  drivers/platform/x86/intel/tdx/Makefile       |   3 +
>  .../platform/x86/intel/tdx/intel_tdx_attest.c | 230 ++++++++++++++++++
>  include/uapi/misc/tdx.h                       |  42 ++++
>  6 files changed, 290 insertions(+)
>  create mode 100644 drivers/platform/x86/intel/tdx/Kconfig
>  create mode 100644 drivers/platform/x86/intel/tdx/Makefile
>  create mode 100644 drivers/platform/x86/intel/tdx/intel_tdx_attest.c
>  create mode 100644 include/uapi/misc/tdx.h
> 
> diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
> index 8e65086bb6c8..a2ed17d67052 100644
> --- a/drivers/platform/x86/intel/Kconfig
> +++ b/drivers/platform/x86/intel/Kconfig
> @@ -12,6 +12,7 @@ source "drivers/platform/x86/intel/pmt/Kconfig"
>  source "drivers/platform/x86/intel/speed_select_if/Kconfig"
>  source "drivers/platform/x86/intel/telemetry/Kconfig"
>  source "drivers/platform/x86/intel/wmi/Kconfig"
> +source "drivers/platform/x86/intel/tdx/Kconfig"
>  
>  config INTEL_HID_EVENT
>  	tristate "Intel HID Event"
> diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
> index 35f2066578b2..27a6c6c5a83f 100644
> --- a/drivers/platform/x86/intel/Makefile
> +++ b/drivers/platform/x86/intel/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_INTEL_SKL_INT3472)		+= int3472/
>  obj-$(CONFIG_INTEL_PMC_CORE)		+= pmc/
>  obj-$(CONFIG_INTEL_PMT_CLASS)		+= pmt/
>  obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += speed_select_if/
> +obj-$(CONFIG_INTEL_TDX_GUEST)		+= tdx/
>  obj-$(CONFIG_INTEL_TELEMETRY)		+= telemetry/
>  obj-$(CONFIG_INTEL_WMI)			+= wmi/
>  
> diff --git a/drivers/platform/x86/intel/tdx/Kconfig b/drivers/platform/x86/intel/tdx/Kconfig
> new file mode 100644
> index 000000000000..853e3a34c889
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# X86 TDX Platform Specific Drivers
> +#
> +
> +config INTEL_TDX_ATTESTATION
> +	tristate "Intel TDX attestation driver"
> +	depends on INTEL_TDX_GUEST
> +	help
> +	  The TDX attestation driver provides IOCTL interfaces to the user to
> +	  request TDREPORT from the TDX module or request quote from the VMM
> +	  or to get quote buffer size. It is mainly used to get secure disk
> +	  decryption keys from the key server.
> diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
> new file mode 100644
> index 000000000000..124d6b7b20a0
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
> diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> new file mode 100644
> index 000000000000..0bf78d30e057
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel_tdx_attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process and
> + * read the TD Quote result.
> + *
> + * Copyright (C) 2021-2022 Intel Corporation
> + *
> + * Author:
> + *     Kuppuswamy Sathyanarayanan <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/dma-mapping.h>
> +#include <linux/jiffies.h>
> +#include <linux/io.h>
> +#include <asm/apic.h>
> +#include <asm/tdx.h>
> +#include <asm/irq_vectors.h>
> +#include <uapi/misc/tdx.h>
> +
> +/* Used in Quote memory allocation */
> +#define QUOTE_SIZE			(2 * PAGE_SIZE)
> +/* Used in Get Quote request memory allocation */
> +#define GET_QUOTE_MAX_SIZE		(4 * PAGE_SIZE)
> +/* Get Quote timeout in msec */
> +#define GET_QUOTE_TIMEOUT		(5000)
> +
> +/* Mutex to synchronize attestation requests */
> +static DEFINE_MUTEX(attestation_lock);
> +/* Completion object to track attestation status */
> +static DECLARE_COMPLETION(attestation_done);
> +/* Buffer used to copy report data in attestation handler */
> +static u8 report_data[TDX_REPORT_DATA_LEN] __aligned(64);
> +/* Data pointer used to get TD Quote data in attestation handler */
> +static void *tdquote_data;
> +/* Data pointer used to get TDREPORT data in attestation handler */
> +static void *tdreport_data;
> +/* DMA handle used to allocate and free tdquote DMA buffer */
> +dma_addr_t tdquote_dma_handle;
> +
> +static void attestation_callback_handler(void)
> +{
> +	complete(&attestation_done);
> +}
> +
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	struct tdx_gen_quote tdquote_req;
> +	long ret = 0;
> +
> +	mutex_lock(&attestation_lock);
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_TDREPORT:
> +		if (copy_from_user(report_data, argp, TDX_REPORT_DATA_LEN)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		/* Generate TDREPORT_STRUCT */
> +		if (tdx_mcall_tdreport(tdreport_data, report_data)) {
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		if (copy_to_user(argp, tdreport_data, TDX_TDREPORT_LEN))
> +			ret = -EFAULT;
> +		break;
> +	case TDX_CMD_GEN_QUOTE:
> +		reinit_completion(&attestation_done);
> +
> +		/* Copy TDREPORT data from user buffer */
> +		if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (copy_from_user(tdquote_data, tdquote_req.buf, tdquote_req.len)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		/* Submit GetQuote Request */
> +		if (tdx_hcall_get_quote(tdquote_data, GET_QUOTE_MAX_SIZE)) {
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		/* Wait for attestation completion */
> +		ret = wait_for_completion_interruptible_timeout(
> +				&attestation_done,
> +				msecs_to_jiffies(GET_QUOTE_TIMEOUT));
> +		if (ret <= 0) {
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		/* ret will be positive if completed. */
> +		ret = 0;
> +
> +		if (copy_to_user(tdquote_req.buf, tdquote_data, tdquote_req.len))
> +			ret = -EFAULT;
> +
> +		break;
> +	case TDX_CMD_GET_QUOTE_SIZE:
> +		ret = put_user(QUOTE_SIZE, (u64 __user *)argp);
> +		break;
> +	default:
> +		pr_err("cmd %d not supported\n", cmd);
> +		break;
> +	}
> +
> +	mutex_unlock(&attestation_lock);
> +
> +	return ret;
> +}
> +
> +static const struct file_operations tdx_attest_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= tdx_attest_ioctl,
> +	.llseek		= no_llseek,
> +};
> +
> +static struct miscdevice tdx_attest_device = {
> +	.minor          = MISC_DYNAMIC_MINOR,
> +	.name           = "tdx-attest",
> +	.fops           = &tdx_attest_fops,
> +};
> +
> +static int __init tdx_attest_init(void)
> +{
> +	dma_addr_t handle;
> +	long ret = 0;
> +
> +	mutex_lock(&attestation_lock);
> +
> +	ret = misc_register(&tdx_attest_device);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		mutex_unlock(&attestation_lock);
> +		return ret;
> +	}

Why not do this as the last thing of the probe?

That will avoid the need to unregister this again in all
the error-exit paths and also fixes a possible deadlock.

Right now you possibly have:

1. probe() locks attestation_lock
2. probe() registers misc-device
3. userspace calls tdx_attest_ioctl
4. tdx_attest_ioctl blocks waiting for attestastion_lock
5. Something goes wrong in probe, probe calls
   misc_deregister()
6. misc_deregister waits for the ioctl to finish
7. deadlock

I'm not sure about 6, but if 6 does not happen then
instead we now have tdx_attest_ioctl running
after the misc_deregister, with tdquote_data and
tdreport_data as NULL, or pointing to free-ed memory
leading to various crash scenarios.

TL;DR: you must always delay registering any
interfaces for userspace until your code is
ready to deal with userspace calls.

Regards,

Hans

p.s.

As I mentioned with v1:

I really know very little about TDX.
I assume the rest of the series will be reviewed by someone
with more detailed knowledge of TDX as such I believe it would be good
if the platform/x86 patch is also reviewed as part of that.

Since the platform/x86 patch depends on the other patches I believe
it is also best if the entire series is merged in one go by the x86/tip
maintainers here is my ack for this:

Acked-by: Hans de Goede <hdegoede@redhat.com>





> +
> +	/*
> +	 * tdreport_data needs to be 64-byte aligned.
> +	 * Full page alignment is more than enough.
> +	 */
> +	tdreport_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 0);
> +	if (!tdreport_data) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}
> +
> +	ret = dma_set_coherent_mask(tdx_attest_device.this_device,
> +				    DMA_BIT_MASK(64));
> +	if (ret) {
> +		pr_err("dma set coherent mask failed\n");
> +		goto failed;
> +	}
> +
> +	/* Allocate DMA buffer to get TDQUOTE data from the VMM */
> +	tdquote_data = dma_alloc_coherent(tdx_attest_device.this_device,
> +					  GET_QUOTE_MAX_SIZE, &handle,
> +					  GFP_KERNEL | __GFP_ZERO);
> +	if (!tdquote_data) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}
> +
> +	tdquote_dma_handle =  handle;
> +
> +	/* Register attestation event notify handler */
> +	tdx_setup_ev_notify_handler(attestation_callback_handler);
> +
> +	mutex_unlock(&attestation_lock);
> +
> +	pr_debug("module initialization success\n");
> +
> +	return 0;
> +
> +failed:
> +	if (tdreport_data)
> +		free_pages((unsigned long)tdreport_data, 0);
> +
> +	misc_deregister(&tdx_attest_device);
> +
> +	mutex_unlock(&attestation_lock);
> +
> +	pr_debug("module initialization failed\n");
> +
> +	return ret;
> +}
> +
> +static void __exit tdx_attest_exit(void)
> +{
> +	mutex_lock(&attestation_lock);
> +
> +	dma_free_coherent(tdx_attest_device.this_device, GET_QUOTE_MAX_SIZE,
> +			  tdquote_data, tdquote_dma_handle);
> +	free_pages((unsigned long)tdreport_data, 0);
> +	misc_deregister(&tdx_attest_device);
> +	/* Unregister attestation event notify handler */
> +	tdx_remove_ev_notify_handler();
> +	mutex_unlock(&attestation_lock);
> +	pr_debug("module is successfully removed\n");
> +}
> +
> +module_init(tdx_attest_init);
> +module_exit(tdx_attest_exit);
> +
> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
> +MODULE_DESCRIPTION("TDX attestation driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/misc/tdx.h b/include/uapi/misc/tdx.h
> new file mode 100644
> index 000000000000..839b9a220022
> --- /dev/null
> +++ b/include/uapi/misc/tdx.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_MISC_TDX_H
> +#define _UAPI_MISC_TDX_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
> +#define TDX_REPORT_DATA_LEN		64
> +
> +/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
> +#define TDX_TDREPORT_LEN		1024
> +
> +/*
> + * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
> + * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
> + * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
> + * TDREPORT data is copied to the user buffer.
> + */
> +#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
> +
> +/*
> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
> + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
> + * buffer of quote size. Once IOCTL is successful quote data is copied back to
> + * the user buffer.
> + */
> +#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
> +
> +/*
> + * TDX_CMD_GET_QUOTE_SIZE IOCTL is used to get the TD Quote size info in bytes.
> + * This will be used for determining the input buffer allocation size when
> + * using TDX_CMD_GEN_QUOTE IOCTL.
> + */
> +#define TDX_CMD_GET_QUOTE_SIZE		_IOR('T', 0x03, __u64)
> +
> +struct tdx_gen_quote {
> +	void *buf __user;
> +	size_t len;
> +};
> +
> +#endif /* _UAPI_MISC_TDX_H */

Re: [PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Hans de Goede]

Hi,

On 3/31/22 00:18, Kuppuswamy Sathyanarayanan wrote:
> 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. 
> 
> [Chenyi Qiang: Proposed struct tdx_gen_quote for passing user buffer]
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/platform/x86/intel/Kconfig            |   1 +
>  drivers/platform/x86/intel/Makefile           |   1 +
>  drivers/platform/x86/intel/tdx/Kconfig        |  13 +
>  drivers/platform/x86/intel/tdx/Makefile       |   3 +
>  .../platform/x86/intel/tdx/intel_tdx_attest.c | 230 ++++++++++++++++++
>  include/uapi/misc/tdx.h                       |  42 ++++
>  6 files changed, 290 insertions(+)
>  create mode 100644 drivers/platform/x86/intel/tdx/Kconfig
>  create mode 100644 drivers/platform/x86/intel/tdx/Makefile
>  create mode 100644 drivers/platform/x86/intel/tdx/intel_tdx_attest.c
>  create mode 100644 include/uapi/misc/tdx.h
> 
> diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
> index 8e65086bb6c8..a2ed17d67052 100644
> --- a/drivers/platform/x86/intel/Kconfig
> +++ b/drivers/platform/x86/intel/Kconfig
> @@ -12,6 +12,7 @@ source "drivers/platform/x86/intel/pmt/Kconfig"
>  source "drivers/platform/x86/intel/speed_select_if/Kconfig"
>  source "drivers/platform/x86/intel/telemetry/Kconfig"
>  source "drivers/platform/x86/intel/wmi/Kconfig"
> +source "drivers/platform/x86/intel/tdx/Kconfig"
>  
>  config INTEL_HID_EVENT
>  	tristate "Intel HID Event"
> diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
> index 35f2066578b2..27a6c6c5a83f 100644
> --- a/drivers/platform/x86/intel/Makefile
> +++ b/drivers/platform/x86/intel/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_INTEL_SKL_INT3472)		+= int3472/
>  obj-$(CONFIG_INTEL_PMC_CORE)		+= pmc/
>  obj-$(CONFIG_INTEL_PMT_CLASS)		+= pmt/
>  obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += speed_select_if/
> +obj-$(CONFIG_INTEL_TDX_GUEST)		+= tdx/
>  obj-$(CONFIG_INTEL_TELEMETRY)		+= telemetry/
>  obj-$(CONFIG_INTEL_WMI)			+= wmi/
>  
> diff --git a/drivers/platform/x86/intel/tdx/Kconfig b/drivers/platform/x86/intel/tdx/Kconfig
> new file mode 100644
> index 000000000000..853e3a34c889
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# X86 TDX Platform Specific Drivers
> +#
> +
> +config INTEL_TDX_ATTESTATION
> +	tristate "Intel TDX attestation driver"
> +	depends on INTEL_TDX_GUEST
> +	help
> +	  The TDX attestation driver provides IOCTL interfaces to the user to
> +	  request TDREPORT from the TDX module or request quote from the VMM
> +	  or to get quote buffer size. It is mainly used to get secure disk
> +	  decryption keys from the key server.
> diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
> new file mode 100644
> index 000000000000..124d6b7b20a0
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
> diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> new file mode 100644
> index 000000000000..0bf78d30e057
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel_tdx_attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process and
> + * read the TD Quote result.
> + *
> + * Copyright (C) 2021-2022 Intel Corporation
> + *
> + * Author:
> + *     Kuppuswamy Sathyanarayanan <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/dma-mapping.h>
> +#include <linux/jiffies.h>
> +#include <linux/io.h>
> +#include <asm/apic.h>
> +#include <asm/tdx.h>
> +#include <asm/irq_vectors.h>
> +#include <uapi/misc/tdx.h>
> +
> +/* Used in Quote memory allocation */
> +#define QUOTE_SIZE			(2 * PAGE_SIZE)
> +/* Used in Get Quote request memory allocation */
> +#define GET_QUOTE_MAX_SIZE		(4 * PAGE_SIZE)
> +/* Get Quote timeout in msec */
> +#define GET_QUOTE_TIMEOUT		(5000)
> +
> +/* Mutex to synchronize attestation requests */
> +static DEFINE_MUTEX(attestation_lock);
> +/* Completion object to track attestation status */
> +static DECLARE_COMPLETION(attestation_done);
> +/* Buffer used to copy report data in attestation handler */
> +static u8 report_data[TDX_REPORT_DATA_LEN] __aligned(64);
> +/* Data pointer used to get TD Quote data in attestation handler */
> +static void *tdquote_data;
> +/* Data pointer used to get TDREPORT data in attestation handler */
> +static void *tdreport_data;
> +/* DMA handle used to allocate and free tdquote DMA buffer */
> +dma_addr_t tdquote_dma_handle;
> +
> +static void attestation_callback_handler(void)
> +{
> +	complete(&attestation_done);
> +}
> +
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	struct tdx_gen_quote tdquote_req;
> +	long ret = 0;
> +
> +	mutex_lock(&attestation_lock);
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_TDREPORT:
> +		if (copy_from_user(report_data, argp, TDX_REPORT_DATA_LEN)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		/* Generate TDREPORT_STRUCT */
> +		if (tdx_mcall_tdreport(tdreport_data, report_data)) {
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		if (copy_to_user(argp, tdreport_data, TDX_TDREPORT_LEN))
> +			ret = -EFAULT;
> +		break;
> +	case TDX_CMD_GEN_QUOTE:
> +		reinit_completion(&attestation_done);
> +
> +		/* Copy TDREPORT data from user buffer */
> +		if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (copy_from_user(tdquote_data, tdquote_req.buf, tdquote_req.len)) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		/* Submit GetQuote Request */
> +		if (tdx_hcall_get_quote(tdquote_data, GET_QUOTE_MAX_SIZE)) {
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		/* Wait for attestation completion */
> +		ret = wait_for_completion_interruptible_timeout(
> +				&attestation_done,
> +				msecs_to_jiffies(GET_QUOTE_TIMEOUT));
> +		if (ret <= 0) {
> +			ret = -EIO;
> +			break;
> +		}
> +
> +		/* ret will be positive if completed. */
> +		ret = 0;
> +
> +		if (copy_to_user(tdquote_req.buf, tdquote_data, tdquote_req.len))
> +			ret = -EFAULT;
> +
> +		break;
> +	case TDX_CMD_GET_QUOTE_SIZE:
> +		ret = put_user(QUOTE_SIZE, (u64 __user *)argp);
> +		break;
> +	default:
> +		pr_err("cmd %d not supported\n", cmd);
> +		break;
> +	}
> +
> +	mutex_unlock(&attestation_lock);
> +
> +	return ret;
> +}
> +
> +static const struct file_operations tdx_attest_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= tdx_attest_ioctl,
> +	.llseek		= no_llseek,
> +};
> +
> +static struct miscdevice tdx_attest_device = {
> +	.minor          = MISC_DYNAMIC_MINOR,
> +	.name           = "tdx-attest",
> +	.fops           = &tdx_attest_fops,
> +};
> +
> +static int __init tdx_attest_init(void)
> +{
> +	dma_addr_t handle;
> +	long ret = 0;
> +
> +	mutex_lock(&attestation_lock);
> +
> +	ret = misc_register(&tdx_attest_device);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		mutex_unlock(&attestation_lock);
> +		return ret;
> +	}

Why not do this as the last thing of the probe?

That will avoid the need to unregister this again in all
the error-exit paths and also fixes a possible deadlock.

Right now you possibly have:

1. probe() locks attestation_lock
2. probe() registers misc-device
3. userspace calls tdx_attest_ioctl
4. tdx_attest_ioctl blocks waiting for attestastion_lock
5. Something goes wrong in probe, probe calls
   misc_deregister()
6. misc_deregister waits for the ioctl to finish
7. deadlock

I'm not sure about 6, but if 6 does not happen then
instead we now have tdx_attest_ioctl running
after the misc_deregister, with tdquote_data and
tdreport_data as NULL, or pointing to free-ed memory
leading to various crash scenarios.

TL;DR: you must always delay registering any
interfaces for userspace until your code is
ready to deal with userspace calls.

Regards,

Hans

p.s.

As I mentioned with v1:


> +
> +	/*
> +	 * tdreport_data needs to be 64-byte aligned.
> +	 * Full page alignment is more than enough.
> +	 */
> +	tdreport_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 0);
> +	if (!tdreport_data) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}
> +
> +	ret = dma_set_coherent_mask(tdx_attest_device.this_device,
> +				    DMA_BIT_MASK(64));
> +	if (ret) {
> +		pr_err("dma set coherent mask failed\n");
> +		goto failed;
> +	}
> +
> +	/* Allocate DMA buffer to get TDQUOTE data from the VMM */
> +	tdquote_data = dma_alloc_coherent(tdx_attest_device.this_device,
> +					  GET_QUOTE_MAX_SIZE, &handle,
> +					  GFP_KERNEL | __GFP_ZERO);
> +	if (!tdquote_data) {
> +		ret = -ENOMEM;
> +		goto failed;
> +	}
> +
> +	tdquote_dma_handle =  handle;
> +
> +	/* Register attestation event notify handler */
> +	tdx_setup_ev_notify_handler(attestation_callback_handler);
> +
> +	mutex_unlock(&attestation_lock);
> +
> +	pr_debug("module initialization success\n");
> +
> +	return 0;
> +
> +failed:
> +	if (tdreport_data)
> +		free_pages((unsigned long)tdreport_data, 0);
> +
> +	misc_deregister(&tdx_attest_device);
> +
> +	mutex_unlock(&attestation_lock);
> +
> +	pr_debug("module initialization failed\n");
> +
> +	return ret;
> +}
> +
> +static void __exit tdx_attest_exit(void)
> +{
> +	mutex_lock(&attestation_lock);
> +
> +	dma_free_coherent(tdx_attest_device.this_device, GET_QUOTE_MAX_SIZE,
> +			  tdquote_data, tdquote_dma_handle);
> +	free_pages((unsigned long)tdreport_data, 0);
> +	misc_deregister(&tdx_attest_device);
> +	/* Unregister attestation event notify handler */
> +	tdx_remove_ev_notify_handler();
> +	mutex_unlock(&attestation_lock);
> +	pr_debug("module is successfully removed\n");
> +}
> +
> +module_init(tdx_attest_init);
> +module_exit(tdx_attest_exit);
> +
> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
> +MODULE_DESCRIPTION("TDX attestation driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/misc/tdx.h b/include/uapi/misc/tdx.h
> new file mode 100644
> index 000000000000..839b9a220022
> --- /dev/null
> +++ b/include/uapi/misc/tdx.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_MISC_TDX_H
> +#define _UAPI_MISC_TDX_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
> +#define TDX_REPORT_DATA_LEN		64
> +
> +/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
> +#define TDX_TDREPORT_LEN		1024
> +
> +/*
> + * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
> + * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
> + * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
> + * TDREPORT data is copied to the user buffer.
> + */
> +#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
> +
> +/*
> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
> + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
> + * buffer of quote size. Once IOCTL is successful quote data is copied back to
> + * the user buffer.
> + */
> +#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
> +
> +/*
> + * TDX_CMD_GET_QUOTE_SIZE IOCTL is used to get the TD Quote size info in bytes.
> + * This will be used for determining the input buffer allocation size when
> + * using TDX_CMD_GEN_QUOTE IOCTL.
> + */
> +#define TDX_CMD_GET_QUOTE_SIZE		_IOR('T', 0x03, __u64)
> +
> +struct tdx_gen_quote {
> +	void *buf __user;
> +	size_t len;
> +};
> +
> +#endif /* _UAPI_MISC_TDX_H */

[PATCH v2 5/6] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

[Kuppuswamy Sathyanarayanan]

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. 

[Chenyi Qiang: Proposed struct tdx_gen_quote for passing user buffer]
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/platform/x86/intel/Kconfig            |   1 +
 drivers/platform/x86/intel/Makefile           |   1 +
 drivers/platform/x86/intel/tdx/Kconfig        |  13 +
 drivers/platform/x86/intel/tdx/Makefile       |   3 +
 .../platform/x86/intel/tdx/intel_tdx_attest.c | 230 ++++++++++++++++++
 include/uapi/misc/tdx.h                       |  42 ++++
 6 files changed, 290 insertions(+)
 create mode 100644 drivers/platform/x86/intel/tdx/Kconfig
 create mode 100644 drivers/platform/x86/intel/tdx/Makefile
 create mode 100644 drivers/platform/x86/intel/tdx/intel_tdx_attest.c
 create mode 100644 include/uapi/misc/tdx.h

diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 8e65086bb6c8..a2ed17d67052 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -12,6 +12,7 @@ source "drivers/platform/x86/intel/pmt/Kconfig"
 source "drivers/platform/x86/intel/speed_select_if/Kconfig"
 source "drivers/platform/x86/intel/telemetry/Kconfig"
 source "drivers/platform/x86/intel/wmi/Kconfig"
+source "drivers/platform/x86/intel/tdx/Kconfig"
 
 config INTEL_HID_EVENT
 	tristate "Intel HID Event"
diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
index 35f2066578b2..27a6c6c5a83f 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_INTEL_SKL_INT3472)		+= int3472/
 obj-$(CONFIG_INTEL_PMC_CORE)		+= pmc/
 obj-$(CONFIG_INTEL_PMT_CLASS)		+= pmt/
 obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += speed_select_if/
+obj-$(CONFIG_INTEL_TDX_GUEST)		+= tdx/
 obj-$(CONFIG_INTEL_TELEMETRY)		+= telemetry/
 obj-$(CONFIG_INTEL_WMI)			+= wmi/
 
diff --git a/drivers/platform/x86/intel/tdx/Kconfig b/drivers/platform/x86/intel/tdx/Kconfig
new file mode 100644
index 000000000000..853e3a34c889
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# X86 TDX Platform Specific Drivers
+#
+
+config INTEL_TDX_ATTESTATION
+	tristate "Intel TDX attestation driver"
+	depends on INTEL_TDX_GUEST
+	help
+	  The TDX attestation driver provides IOCTL interfaces to the user to
+	  request TDREPORT from the TDX module or request quote from the VMM
+	  or to get quote buffer size. It is mainly used to get secure disk
+	  decryption keys from the key server.
diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
new file mode 100644
index 000000000000..124d6b7b20a0
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
new file mode 100644
index 000000000000..0bf78d30e057
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel_tdx_attest.c - TDX guest attestation interface driver.
+ *
+ * Implements user interface to trigger attestation process and
+ * read the TD Quote result.
+ *
+ * Copyright (C) 2021-2022 Intel Corporation
+ *
+ * Author:
+ *     Kuppuswamy Sathyanarayanan <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/dma-mapping.h>
+#include <linux/jiffies.h>
+#include <linux/io.h>
+#include <asm/apic.h>
+#include <asm/tdx.h>
+#include <asm/irq_vectors.h>
+#include <uapi/misc/tdx.h>
+
+/* Used in Quote memory allocation */
+#define QUOTE_SIZE			(2 * PAGE_SIZE)
+/* Used in Get Quote request memory allocation */
+#define GET_QUOTE_MAX_SIZE		(4 * PAGE_SIZE)
+/* Get Quote timeout in msec */
+#define GET_QUOTE_TIMEOUT		(5000)
+
+/* Mutex to synchronize attestation requests */
+static DEFINE_MUTEX(attestation_lock);
+/* Completion object to track attestation status */
+static DECLARE_COMPLETION(attestation_done);
+/* Buffer used to copy report data in attestation handler */
+static u8 report_data[TDX_REPORT_DATA_LEN] __aligned(64);
+/* Data pointer used to get TD Quote data in attestation handler */
+static void *tdquote_data;
+/* Data pointer used to get TDREPORT data in attestation handler */
+static void *tdreport_data;
+/* DMA handle used to allocate and free tdquote DMA buffer */
+dma_addr_t tdquote_dma_handle;
+
+static void attestation_callback_handler(void)
+{
+	complete(&attestation_done);
+}
+
+static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
+			     unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	struct tdx_gen_quote tdquote_req;
+	long ret = 0;
+
+	mutex_lock(&attestation_lock);
+
+	switch (cmd) {
+	case TDX_CMD_GET_TDREPORT:
+		if (copy_from_user(report_data, argp, TDX_REPORT_DATA_LEN)) {
+			ret = -EFAULT;
+			break;
+		}
+
+		/* Generate TDREPORT_STRUCT */
+		if (tdx_mcall_tdreport(tdreport_data, report_data)) {
+			ret = -EIO;
+			break;
+		}
+
+		if (copy_to_user(argp, tdreport_data, TDX_TDREPORT_LEN))
+			ret = -EFAULT;
+		break;
+	case TDX_CMD_GEN_QUOTE:
+		reinit_completion(&attestation_done);
+
+		/* Copy TDREPORT data from user buffer */
+		if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (copy_from_user(tdquote_data, tdquote_req.buf, tdquote_req.len)) {
+			ret = -EFAULT;
+			break;
+		}
+
+		/* Submit GetQuote Request */
+		if (tdx_hcall_get_quote(tdquote_data, GET_QUOTE_MAX_SIZE)) {
+			ret = -EIO;
+			break;
+		}
+
+		/* Wait for attestation completion */
+		ret = wait_for_completion_interruptible_timeout(
+				&attestation_done,
+				msecs_to_jiffies(GET_QUOTE_TIMEOUT));
+		if (ret <= 0) {
+			ret = -EIO;
+			break;
+		}
+
+		/* ret will be positive if completed. */
+		ret = 0;
+
+		if (copy_to_user(tdquote_req.buf, tdquote_data, tdquote_req.len))
+			ret = -EFAULT;
+
+		break;
+	case TDX_CMD_GET_QUOTE_SIZE:
+		ret = put_user(QUOTE_SIZE, (u64 __user *)argp);
+		break;
+	default:
+		pr_err("cmd %d not supported\n", cmd);
+		break;
+	}
+
+	mutex_unlock(&attestation_lock);
+
+	return ret;
+}
+
+static const struct file_operations tdx_attest_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= tdx_attest_ioctl,
+	.llseek		= no_llseek,
+};
+
+static struct miscdevice tdx_attest_device = {
+	.minor          = MISC_DYNAMIC_MINOR,
+	.name           = "tdx-attest",
+	.fops           = &tdx_attest_fops,
+};
+
+static int __init tdx_attest_init(void)
+{
+	dma_addr_t handle;
+	long ret = 0;
+
+	mutex_lock(&attestation_lock);
+
+	ret = misc_register(&tdx_attest_device);
+	if (ret) {
+		pr_err("misc device registration failed\n");
+		mutex_unlock(&attestation_lock);
+		return ret;
+	}
+
+	/*
+	 * tdreport_data needs to be 64-byte aligned.
+	 * Full page alignment is more than enough.
+	 */
+	tdreport_data = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 0);
+	if (!tdreport_data) {
+		ret = -ENOMEM;
+		goto failed;
+	}
+
+	ret = dma_set_coherent_mask(tdx_attest_device.this_device,
+				    DMA_BIT_MASK(64));
+	if (ret) {
+		pr_err("dma set coherent mask failed\n");
+		goto failed;
+	}
+
+	/* Allocate DMA buffer to get TDQUOTE data from the VMM */
+	tdquote_data = dma_alloc_coherent(tdx_attest_device.this_device,
+					  GET_QUOTE_MAX_SIZE, &handle,
+					  GFP_KERNEL | __GFP_ZERO);
+	if (!tdquote_data) {
+		ret = -ENOMEM;
+		goto failed;
+	}
+
+	tdquote_dma_handle =  handle;
+
+	/* Register attestation event notify handler */
+	tdx_setup_ev_notify_handler(attestation_callback_handler);
+
+	mutex_unlock(&attestation_lock);
+
+	pr_debug("module initialization success\n");
+
+	return 0;
+
+failed:
+	if (tdreport_data)
+		free_pages((unsigned long)tdreport_data, 0);
+
+	misc_deregister(&tdx_attest_device);
+
+	mutex_unlock(&attestation_lock);
+
+	pr_debug("module initialization failed\n");
+
+	return ret;
+}
+
+static void __exit tdx_attest_exit(void)
+{
+	mutex_lock(&attestation_lock);
+
+	dma_free_coherent(tdx_attest_device.this_device, GET_QUOTE_MAX_SIZE,
+			  tdquote_data, tdquote_dma_handle);
+	free_pages((unsigned long)tdreport_data, 0);
+	misc_deregister(&tdx_attest_device);
+	/* Unregister attestation event notify handler */
+	tdx_remove_ev_notify_handler();
+	mutex_unlock(&attestation_lock);
+	pr_debug("module is successfully removed\n");
+}
+
+module_init(tdx_attest_init);
+module_exit(tdx_attest_exit);
+
+MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
+MODULE_DESCRIPTION("TDX attestation driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/misc/tdx.h b/include/uapi/misc/tdx.h
new file mode 100644
index 000000000000..839b9a220022
--- /dev/null
+++ b/include/uapi/misc/tdx.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_MISC_TDX_H
+#define _UAPI_MISC_TDX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
+#define TDX_REPORT_DATA_LEN		64
+
+/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
+#define TDX_TDREPORT_LEN		1024
+
+/*
+ * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
+ * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
+ * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
+ * TDREPORT data is copied to the user buffer.
+ */
+#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
+
+/*
+ * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
+ * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
+ * buffer of quote size. Once IOCTL is successful quote data is copied back to
+ * the user buffer.
+ */
+#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
+
+/*
+ * TDX_CMD_GET_QUOTE_SIZE IOCTL is used to get the TD Quote size info in bytes.
+ * This will be used for determining the input buffer allocation size when
+ * using TDX_CMD_GEN_QUOTE IOCTL.
+ */
+#define TDX_CMD_GET_QUOTE_SIZE		_IOR('T', 0x03, __u64)
+
+struct tdx_gen_quote {
+	void *buf __user;
+	size_t len;
+};
+
+#endif /* _UAPI_MISC_TDX_H */
-- 
2.25.1