[PATCH v6 0/5] Add TDX Guest Attestation support

[PATCH v6 0/5] Add TDX Guest 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 add the attestation support to TDX guest which
includes attestation user interface driver and related hypercall support.

Any distribution enabling TDX is also expected to need attestation. So
enable it by default with TDX guest support. The compiled size is
quite small (~500 bytes).

Changes since v5:
 * Added support for parallel GetQuote requests.
 * Add noalias variants of set_memory_*crypted() functions to
   changes page attribute without touching direct map.
 * Made set_memory_*crypted() functions vmalloc address compatible.
 * Use vmap()/set_memory_*crypted() functions to share/unshare
   memory without touching the direct map.
 * Add support to let driver handle the memory cleanup for the
   early termination of user requests.
 * Removed unused headers in attest.c
 * Fixed commit log and comments as per review comments.

Changes since v4:
 * Removed platform driver model in attestation driver and used
   miscdevice and initcall approach.
 * Since dma_alloc*() APIs require a valid device reference,
   replaced it with __get_free_pages() and set_memory_decrypted()
   for quote memory allocation.
 * Removed tdx_mcall_tdreport() and moved TDG.MR.REPORT TDCALL code
   to tdx_get_report().
 * Used kmalloc() for TDREPORT memory allocation instead of
   get_zeroed_page().
 * Returned -EINVAL in default case of tdx_attest_ioctl().
 * Added struct tdx_report_req to explicitly mention the
   TDX_CMD_GET_REPORT IOCTL argument.
 * Removed tdx_get_quote_hypercall() and moved hypercall code to
   attestation driver itself.
 * Removed GetQuote timeout support (since it is not defined in
   spec)
 * Added support to check for spurious callback interrupt in GetQuote
   request.
 * Fixed commit log and comments as per review suggestions.
   

Changes since v3:
 * Moved the attestation driver from platform/x86 to arch/x86/coco/tdx/ and
   renamed intel_tdx_attest.c to attest.c.
 * Dropped CONFIG_INTEL_TDX_ATTESTATION and added support to compile
   attestation changes with CONFIG_INTEL_TDX_GUEST option.
 * Merged patch titled "x86/tdx: Add tdx_mcall_tdreport() API support" and
   "platform/x86: intel_tdx_attest: Add TDX Guest attestation interface" into
   a single patch.
 * Moved GetQuote IOCTL support changes from patch titled "platform/x86:
   intel_tdx_attest: Add TDX Guest attestation interface driver" to a
   separate patch.
 * Removed 8K size restriction when requesting quote, and added support
   to let userspace decide the quote size.
 * Added support to allow attestation agent configure quote generation
   timeout value.
 * Fixed commit log and comments as per review comments.

Changes since v2:
 * As per Han's suggestion, modified the attestation driver to use
   platform device driver model.
 * Modified tdx_hcall_get_quote() and tdx_mcall_tdreport() APIs to
   return TDCALL error code instead of generic error info (like -EIO).
 * Removed attestation test app patch from this series to simplify
   the patchset and review process. Test app patches will be submitted
   once attestation support patches are merged.
 * Since patches titled "x86/tdx: Add SetupEventNotifyInterrupt TDX
   hypercall support" and "x86/tdx: Add TDX Guest event notify
   interrupt vector support" are related, combining them into a
   single patch.

Changes since v1:
 * Moved test driver from "tools/tdx/attest/tdx-attest-test.c" to
   "tools/arch/x86/tdx/attest/tdx-attest-test.c" as per Hans review
   suggestion.
 * Minor commit log and comment fixes in patches titled
   "x86/tdx: Add tdx_mcall_tdreport() API support" and "x86/tdx:
   Add tdx_hcall_get_quote() API support"
 * Extended tdx_hcall_get_quote() API to accept GPA length as argument
   to accomodate latest TDQUOTE TDVMCALL related specification update.
 * Added support for tdx_setup_ev_notify_handler() and
   tdx_remove_ev_notify_handler() in patch titled "x86/tdx: Add TDX
   Guest event notify interrupt vector support"

Kuppuswamy Sathyanarayanan (5):
  x86/tdx: Add TDX Guest attestation interface driver
  x86/tdx: Add TDX Guest event notify interrupt support
  x86/mm: Make tdx_enc_status_changed() vmalloc address compatible
  x86/mm: Add noalias variants of set_memory_*crypted() functions
  x86/tdx: Add Quote generation support

 arch/x86/coco/tdx/Makefile         |   2 +-
 arch/x86/coco/tdx/attest.c         | 411 +++++++++++++++++++++++++++++
 arch/x86/coco/tdx/tdx.c            |  84 +++++-
 arch/x86/include/asm/hardirq.h     |   3 +
 arch/x86/include/asm/idtentry.h    |   4 +
 arch/x86/include/asm/irq_vectors.h |   7 +-
 arch/x86/include/asm/set_memory.h  |   2 +
 arch/x86/include/asm/tdx.h         |   4 +
 arch/x86/include/uapi/asm/tdx.h    |  87 ++++++
 arch/x86/kernel/irq.c              |   7 +
 arch/x86/mm/pat/set_memory.c       |  26 +-
 11 files changed, 627 insertions(+), 10 deletions(-)
 create mode 100644 arch/x86/coco/tdx/attest.c
 create mode 100644 arch/x86/include/uapi/asm/tdx.h

-- 
2.25.1

[PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Kuppuswamy Sathyanarayanan]

In TDX guest, attestation is used to verify the trustworthiness of a TD
to other entities before provisioning secrets to the TD.

One usage example is, when a TD guest uses encrypted drive and if the
decryption keys required to access the drive are stored in a secure 3rd
party keyserver, the key server can use attestation to verify TD's
trustworthiness and release the decryption keys to the TD.

The attestation process consists of two steps: TDREPORT generation and
Quote generation.

TDREPORT (TDREPORT_STRUCT) is a fixed-size data structure generated by
the TDX module which contains TD-specific information (such as TD
measurements), platform security version, and the MAC to protect the
integrity of the TDREPORT. The TD kernel uses TDCALL[TDG.MR.REPORT] to
get the TDREPORT from the TDX module. A user-provided 64-Byte
REPORTDATA is used as input and included in the TDREPORT. Typically it
can be some nonce provided by attestation service so the TDREPORT can
be verified uniquely. More details about TDREPORT can be found in
Intel TDX Module specification, section titled "TDG.MR.REPORT Leaf".

Also note that the MAC added to the TDREPORT is bound to the platform.
So TDREPORT can only be verified locally. Intel SGX Quote Enclave (QE)
is leveraged to verify the TDREPORT locally and convert it to a remote
verifiable Quote to support remote attestation of the TDREPORT.

After getting the TDREPORT, the second step of the attestation process
is to send it to QE or Quote Generation Service (QGS) to generate a TD
Quote. The QE sends the Quote back after it is generated. How is the
data sent and received is QE implementation and deployment specific.TD
userspace attestation software can use whatever communication channel
available (i.e. tcp/ip, vsock) to communicate with the QE using whatever
data format. TDX also defines TDVMCALLs to allow TD to ask VMM to
facilitate sending/receiving data between TD attestation software and
the QE. This support is documented in GHCI 1.0 spec "5.4 TD attestation".

Implement a basic attestation driver to allow TD userspace to get the
TDREPORT, which is sent to QE by the attestation software to generate
a Quote for remote verification.

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.

Operations like getting TDREPORT or Quote generation involves sending
a blob of data as input and getting another blob of data as output. 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 for this use case. Also, the REPORTDATA used
in TDREPORT generation can possibly come from attestation service to
uniquely verify the Quote (like per instance verification). In such
case, since REPORTDATA is a secret, using sysfs to share it is insecure
compared to sending it via IOCTL.

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>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/tdx/Makefile      |   2 +-
 arch/x86/coco/tdx/attest.c      | 118 ++++++++++++++++++++++++++++++++
 arch/x86/include/uapi/asm/tdx.h |  42 ++++++++++++
 3 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/coco/tdx/attest.c
 create mode 100644 arch/x86/include/uapi/asm/tdx.h

diff --git a/arch/x86/coco/tdx/Makefile b/arch/x86/coco/tdx/Makefile
index 46c55998557d..d2db3e6770e5 100644
--- a/arch/x86/coco/tdx/Makefile
+++ b/arch/x86/coco/tdx/Makefile
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 
-obj-y += tdx.o tdcall.o
+obj-y += tdx.o tdcall.o attest.o
diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
new file mode 100644
index 000000000000..a5f4111f9b18
--- /dev/null
+++ b/arch/x86/coco/tdx/attest.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * attest.c - TDX guest attestation interface driver.
+ *
+ * Implements user interface to trigger attestation process.
+ *
+ * Copyright (C) 2022 Intel Corporation
+ *
+ */
+
+#define pr_fmt(fmt) "x86/tdx: attest: " fmt
+
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/io.h>
+#include <asm/tdx.h>
+#include <uapi/asm/tdx.h>
+
+#define DRIVER_NAME "tdx-attest"
+
+/* TDREPORT module call leaf ID */
+#define TDX_GET_REPORT			4
+
+static struct miscdevice miscdev;
+
+static long tdx_get_report(void __user *argp)
+{
+	void *reportdata = NULL, *tdreport = NULL;
+	long ret = 0;
+
+	/* Allocate buffer space for REPORTDATA */
+	reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
+	if (!reportdata)
+		return -ENOMEM;
+
+	/* Allocate buffer space for TDREPORT */
+	tdreport = kmalloc(TDX_REPORT_LEN, GFP_KERNEL);
+	if (!tdreport) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* Copy REPORTDATA from the user buffer */
+	if (copy_from_user(reportdata, argp, TDX_REPORTDATA_LEN)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	/*
+	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
+	 *
+	 * Get the TDREPORT using REPORTDATA as input. Refer to
+	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
+	 * Specification for detailed information.
+	 */
+	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
+				virt_to_phys(reportdata), 0, 0, NULL);
+	if (ret) {
+		pr_debug("TDREPORT TDCALL failed, status:%lx\n", ret);
+		ret = -EIO;
+		goto out;
+	}
+
+	/* Copy TDREPORT back to the user buffer */
+	if (copy_to_user(argp, tdreport, TDX_REPORT_LEN))
+		ret = -EFAULT;
+
+out:
+	kfree(reportdata);
+	kfree(tdreport);
+	return ret;
+}
+
+static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
+			     unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	long ret = -EINVAL;
+
+	switch (cmd) {
+	case TDX_CMD_GET_REPORT:
+		ret = tdx_get_report(argp);
+		break;
+	default:
+		pr_debug("cmd %d not supported\n", cmd);
+		break;
+	}
+
+	return ret;
+}
+
+static const struct file_operations tdx_attest_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= tdx_attest_ioctl,
+	.llseek		= no_llseek,
+};
+
+static int __init tdx_attestation_init(void)
+{
+	long ret;
+
+	/* Make sure we are in a valid TDX platform */
+	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return -EIO;
+
+	miscdev.name = DRIVER_NAME;
+	miscdev.minor = MISC_DYNAMIC_MINOR;
+	miscdev.fops = &tdx_attest_fops;
+
+	ret = misc_register(&miscdev);
+	if (ret) {
+		pr_err("misc device registration failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+device_initcall(tdx_attestation_init)
diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
new file mode 100644
index 000000000000..e06da56058a1
--- /dev/null
+++ b/arch/x86/include/uapi/asm/tdx.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_ASM_X86_TDX_H
+#define _UAPI_ASM_X86_TDX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
+#define TDX_REPORTDATA_LEN		64
+
+/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
+#define TDX_REPORT_LEN			1024
+
+/**
+ * struct tdx_report_req: Get TDREPORT using REPORTDATA as input.
+ *
+ * @reportdata : User-defined 64-Byte REPORTDATA to be included into
+ *		 TDREPORT. Typically it can be some nonce provided by
+ *		 attestation software so the generated TDREPORT can be
+ *		 uniquely verified.
+ * @tdreport   : TDREPORT output from TDCALL[TDG.MR.REPORT] of size
+ *		 TDX_REPORT_LEN.
+ *
+ * Used in TDX_CMD_GET_REPORT IOCTL request.
+ */
+struct tdx_report_req {
+	union {
+		__u8 reportdata[TDX_REPORTDATA_LEN];
+		__u8 tdreport[TDX_REPORT_LEN];
+	};
+};
+
+/*
+ * TDX_CMD_GET_REPORT - Get TDREPORT using TDCALL[TDG.MR.REPORT)
+ *
+ * Return 0 on success, -EIO on TDCALL execution failure, and
+ * standard errno on other general error cases.
+ *
+ */
+#define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, struct tdx_report_req)
+
+#endif /* _UAPI_ASM_X86_TDX_H */
-- 
2.25.1

[PATCH v6 2/5] x86/tdx: Add TDX Guest event notify interrupt support

[Kuppuswamy Sathyanarayanan]

Host-guest event notification via configured interrupt vector is useful
in cases where a guest makes an asynchronous request and needs a
callback from the host to indicate the completion or to let the host
notify the guest about events like device removal. One usage example is,
callback requirement of GetQuote asynchronous hypercall.

In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
guest to specify which interrupt vector to use as an event-notify
vector to the VMM. Details about the SetupEventNotifyInterrupt
hypercall can be found in TDX Guest-Host Communication Interface
(GHCI) Specification, sec 3.5 "VP.VMCALL<SetupEventNotifyInterrupt>".
Add a tdx_hcall_set_notify_intr() helper function to implement the
SetupEventNotifyInterrupt hypercall.

Reserve 0xec IRQ vector address for TDX guest to receive the event
completion notification from VMM. Also add related IDT handler to
process the notification event.

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>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Acked-by: Wander Lairson Costa <wander@redhat.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/tdx/tdx.c            | 73 ++++++++++++++++++++++++++++++
 arch/x86/include/asm/hardirq.h     |  3 ++
 arch/x86/include/asm/idtentry.h    |  4 ++
 arch/x86/include/asm/irq_vectors.h |  7 ++-
 arch/x86/include/asm/tdx.h         |  4 ++
 arch/x86/kernel/irq.c              |  7 +++
 6 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 03deb4d6920d..b49211994864 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -11,6 +11,10 @@
 #include <asm/insn.h>
 #include <asm/insn-eval.h>
 #include <asm/pgtable.h>
+#include <asm/apic.h>
+#include <asm/idtentry.h>
+#include <asm/irq_regs.h>
+#include <asm/desc.h>
 
 /* TDX module Call Leaf IDs */
 #define TDX_GET_INFO			1
@@ -19,6 +23,7 @@
 
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
+#define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
 
 /* MMIO direction */
 #define EPT_READ	0
@@ -34,6 +39,28 @@
 #define VE_GET_PORT_NUM(e)	((e) >> 16)
 #define VE_IS_IO_STRING(e)	((e) & BIT(4))
 
+/*
+ * Handler used to report notifications about
+ * TDX_GUEST_EVENT_NOTIFY_VECTOR IRQ. Currently it will be
+ * used only by the attestation driver. So, race condition
+ * with read/write operation is not considered.
+ */
+static void (*tdx_event_notify_handler)(void);
+
+/* Helper function to register tdx_event_notify_handler */
+void tdx_setup_ev_notify_handler(void (*handler)(void))
+{
+	tdx_event_notify_handler = handler;
+}
+EXPORT_SYMBOL_GPL(tdx_setup_ev_notify_handler);
+
+/* Helper function to unregister tdx_event_notify_handler */
+void tdx_remove_ev_notify_handler(void)
+{
+	tdx_event_notify_handler = NULL;
+}
+EXPORT_SYMBOL_GPL(tdx_remove_ev_notify_handler);
+
 /*
  * Wrapper for standard use of __tdx_hypercall with no output aside from
  * return code.
@@ -98,6 +125,46 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
 }
 
+/* TDX guest event notification handler */
+DEFINE_IDTENTRY_SYSVEC(sysvec_tdx_event_notify)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	inc_irq_stat(irq_tdx_event_notify_count);
+
+	if (tdx_event_notify_handler)
+		tdx_event_notify_handler();
+
+	ack_APIC_irq();
+
+	set_irq_regs(old_regs);
+}
+
+/*
+ * 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.
+ */
+static long tdx_hcall_set_notify_intr(u8 vector)
+{
+	/* Minimum vector value allowed is 32 */
+	if (vector < 32)
+		return -EINVAL;
+
+	/*
+	 * Register callback vector address with VMM. More details
+	 * about the ABI can be found in TDX Guest-Host-Communication
+	 * Interface (GHCI), sec titled
+	 * "TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
+	 */
+	if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, vector, 0, 0, 0))
+		return -EIO;
+
+	return 0;
+}
+
 static u64 get_cc_mask(void)
 {
 	struct tdx_module_output out;
@@ -688,5 +755,11 @@ void __init tdx_early_init(void)
 	x86_platform.guest.enc_tlb_flush_required   = tdx_tlb_flush_required;
 	x86_platform.guest.enc_status_change_finish = tdx_enc_status_changed;
 
+	alloc_intr_gate(TDX_GUEST_EVENT_NOTIFY_VECTOR,
+			asm_sysvec_tdx_event_notify);
+
+	if (tdx_hcall_set_notify_intr(TDX_GUEST_EVENT_NOTIFY_VECTOR))
+		pr_warn("Setting event notification interrupt failed\n");
+
 	pr_info("Guest detected\n");
 }
diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
index 275e7fd20310..582deff56210 100644
--- a/arch/x86/include/asm/hardirq.h
+++ b/arch/x86/include/asm/hardirq.h
@@ -44,6 +44,9 @@ typedef struct {
 	unsigned int irq_hv_reenlightenment_count;
 	unsigned int hyperv_stimer0_count;
 #endif
+#if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)
+	unsigned int irq_tdx_event_notify_count;
+#endif
 } ____cacheline_aligned irq_cpustat_t;
 
 DECLARE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat);
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index 72184b0b2219..655086dd940e 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -700,6 +700,10 @@ DECLARE_IDTENTRY_SYSVEC(HYPERVISOR_CALLBACK_VECTOR,	sysvec_xen_hvm_callback);
 DECLARE_IDTENTRY_SYSVEC(HYPERVISOR_CALLBACK_VECTOR,	sysvec_kvm_asyncpf_interrupt);
 #endif
 
+#if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)
+DECLARE_IDTENTRY_SYSVEC(TDX_GUEST_EVENT_NOTIFY_VECTOR,	sysvec_tdx_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 43dcb9284208..82ac0c0a34b1 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 020c81a7c729..eb4db837cc44 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -67,6 +67,10 @@ void tdx_safe_halt(void);
 
 bool tdx_early_handle_ve(struct pt_regs *regs);
 
+void tdx_setup_ev_notify_handler(void (*handler)(void));
+
+void tdx_remove_ev_notify_handler(void);
+
 #else
 
 static inline void tdx_early_init(void) { };
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 766ffe3ba313..a96ecd866723 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -181,6 +181,13 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 		seq_printf(p, "%10u ",
 			   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, "TGN");
+	for_each_online_cpu(j)
+		seq_printf(p, "%10u ",
+			   irq_stats(j)->irq_tdx_event_notify_count);
+	seq_puts(p, "  TDX Guest event notification\n");
 #endif
 	return 0;
 }
-- 
2.25.1

[PATCH v6 3/5] x86/mm: Make tdx_enc_status_changed() vmalloc address compatible

[Kuppuswamy Sathyanarayanan]

set_memory_*crypted() APIs are used to change encryption or decryption
page attributes for the given address. It also supports conversion for
the vmalloc'ed memory.

In TDX Guest, tdx_enc_status_changed() function is triggered by
set_memory_*crypted() APIs when converting memory from/to shared or
private. Internally this function uses __pa() for physical address
conversion, which breaks the vmalloc address compatibility of the
set_memory_*crypted() API.

So add support to fix the vmalloc'ed address compatibility issue.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/tdx/tdx.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index b49211994864..37d58675ccf1 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -15,6 +15,7 @@
 #include <asm/idtentry.h>
 #include <asm/irq_regs.h>
 #include <asm/desc.h>
+#include <asm/io.h>
 
 /* TDX module Call Leaf IDs */
 #define TDX_GET_INFO			1
@@ -680,8 +681,14 @@ static bool try_accept_one(phys_addr_t *start, unsigned long len,
  */
 static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
 {
-	phys_addr_t start = __pa(vaddr);
-	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
+	phys_addr_t start, end;
+
+	if (is_vmalloc_addr((void *)vaddr))
+		start = vmalloc_to_pfn((void *) vaddr) << PAGE_SHIFT;
+	else
+		start = __pa(vaddr);
+
+	end = start + numpages * PAGE_SIZE;
 
 	if (!enc) {
 		/* Set the shared (decrypted) bits: */
-- 
2.25.1

[PATCH v6 4/5] x86/mm: Add noalias variants of set_memory_*crypted() functions

[Kuppuswamy Sathyanarayanan]

In TDX guest, when creating a shared buffer for the VMM access, to
avoid breaking the direct mapping, vmap() can be used to remap the
memory and use it to create the shared mapping.

Currently, both set_memory_encrypted() and set_memory_decrypted()
functions modify the page attributes of aliased mappings (which also
includes the direct mapping). So handle the use case like mentioned
above, create noalias variants of set_memory_*crypted() functions.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/include/asm/set_memory.h |  2 ++
 arch/x86/mm/pat/set_memory.c      | 26 ++++++++++++++++++++------
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h
index 78ca53512486..0e5fc2b818be 100644
--- a/arch/x86/include/asm/set_memory.h
+++ b/arch/x86/include/asm/set_memory.h
@@ -46,7 +46,9 @@ int set_memory_wb(unsigned long addr, int numpages);
 int set_memory_np(unsigned long addr, int numpages);
 int set_memory_4k(unsigned long addr, int numpages);
 int set_memory_encrypted(unsigned long addr, int numpages);
+int set_memory_encrypted_noalias(unsigned long addr, int numpages);
 int set_memory_decrypted(unsigned long addr, int numpages);
+int set_memory_decrypted_noalias(unsigned long addr, int numpages);
 int set_memory_np_noalias(unsigned long addr, int numpages);
 int set_memory_nonglobal(unsigned long addr, int numpages);
 int set_memory_global(unsigned long addr, int numpages);
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index abf5ed76e4b7..ef54178855a1 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -1987,7 +1987,8 @@ int set_memory_global(unsigned long addr, int numpages)
  * __set_memory_enc_pgtable() is used for the hypervisors that get
  * informed about "encryption" status via page tables.
  */
-static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
+static int __set_memory_enc_pgtable(unsigned long addr, int numpages,
+		bool enc, int checkalias)
 {
 	pgprot_t empty = __pgprot(0);
 	struct cpa_data cpa;
@@ -2015,7 +2016,7 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
 	/* Notify hypervisor that we are about to set/clr encryption attribute. */
 	x86_platform.guest.enc_status_change_prepare(addr, numpages, enc);
 
-	ret = __change_page_attr_set_clr(&cpa, 1);
+	ret = __change_page_attr_set_clr(&cpa, checkalias);
 
 	/*
 	 * After changing the encryption attribute, we need to flush TLBs again
@@ -2035,29 +2036,42 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
 	return ret;
 }
 
-static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
+static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc,
+		int checkalias)
 {
 	if (hv_is_isolation_supported())
 		return hv_set_mem_host_visibility(addr, numpages, !enc);
 
 	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
-		return __set_memory_enc_pgtable(addr, numpages, enc);
+		return __set_memory_enc_pgtable(addr, numpages, enc, checkalias);
 
 	return 0;
 }
 
 int set_memory_encrypted(unsigned long addr, int numpages)
 {
-	return __set_memory_enc_dec(addr, numpages, true);
+	return __set_memory_enc_dec(addr, numpages, true, 1);
 }
 EXPORT_SYMBOL_GPL(set_memory_encrypted);
 
 int set_memory_decrypted(unsigned long addr, int numpages)
 {
-	return __set_memory_enc_dec(addr, numpages, false);
+	return __set_memory_enc_dec(addr, numpages, false, 1);
 }
 EXPORT_SYMBOL_GPL(set_memory_decrypted);
 
+int set_memory_encrypted_noalias(unsigned long addr, int numpages)
+{
+	return __set_memory_enc_dec(addr, numpages, true, 0);
+}
+EXPORT_SYMBOL_GPL(set_memory_encrypted_noalias);
+
+int set_memory_decrypted_noalias(unsigned long addr, int numpages)
+{
+	return __set_memory_enc_dec(addr, numpages, false, 0);
+}
+EXPORT_SYMBOL_GPL(set_memory_decrypted_noalias);
+
 int set_pages_uc(struct page *page, int numpages)
 {
 	unsigned long addr = (unsigned long)page_address(page);
-- 
2.25.1

[PATCH v6 5/5] x86/tdx: Add Quote generation support

[Kuppuswamy Sathyanarayanan]

In TDX guest, the second stage in attestation process is to send the
TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
not support communication channels like vsock or TCP/IP, implement
support to get TD Quote using hypercall. GetQuote hypercall can be used
by the TD guest to request VMM facilitate the Quote generation via
QE/QGS. More details about GetQuote hypercall can be found in TDX
Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
titled "TDG.VP.VMCALL<GetQuote>.

Since GetQuote is an asynchronous request hypercall, it will not block
till the TD Quote is generated. So VMM uses callback interrupt vector
configured by SetupEventNotifyInterrupt hypercall to notify the guest
about Quote generation completion or failure.

GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
with TDREPORT data as input, which is further used by the VMM to copy
the TD Quote result after successful Quote generation. To create the
shared buffer without breaking the direct map, allocate physically
contiguous kernel memory and create a virtual mapping for it using
vmap(). set_memory_*crypted_noalias() functions can be used to share or
unshare the vmapped page without affecting the direct map.

Also note that, shared buffer allocation is currently handled in IOCTL
handler, although it will increase the TDX_CMD_GET_QUOTE IOCTL response
time, it is negligible compared to the time required for the quote
generation completion. So IOCTL performance optimization is not
considered at this time.

For shared buffer allocation, alternatives like using the DMA API is
also considered. Although it simpler to use, it is not preferred because
dma_alloc_*() APIs require a valid bus device as argument, which would
need converting the attestation driver into a platform device driver.
This is unnecessary, and since the attestation driver does not do real
DMA, there is no need to use real DMA APIs.

Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
submit GetQuote requests from the user space. Since Quote generation
is an asynchronous request, IOCTL will block indefinitely for the VMM
response in wait_for_completion_interruptible() call. Using this call
will also add an option for the user to end the current request
prematurely by raising any signals. This can be used by attestation
agent to implement Quote generation timeout feature. If attestation
agent is aware of time it can validly wait for QE/QGS response, then
a possible timeout support can be implemented in the user application
using signals. Quote generation timeout feature is currently not
implemented in the driver because the current TDX specification does
not have any recommendation for it.

After submitting the GetQuote request using hypercall, the shared buffer
allocated for the current request is owned by the VMM. So, during this
wait window, if the user terminates the request by raising a signal or
by terminating the application, add a logic to do the memory cleanup
after receiving the VMM response at a later time. Such memory cleanup
support requires accepting the page again using TDX_ACCEPT_PAGE TDX
Module call. So to not overload the callback IRQ handler, move the
callback handler logic to a separate work queue.

To support parallel GetQuote requests, use linked list to track the
active GetQuote requests and upon receiving the callback IRQ, loop
through the active requests and mark the processed requests complete.
Users can open multiple instances of the attestation device and send
GetQuote requests in parallel.

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>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/tdx/attest.c      | 293 ++++++++++++++++++++++++++++++++
 arch/x86/include/uapi/asm/tdx.h |  45 +++++
 2 files changed, 338 insertions(+)

diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
index a5f4111f9b18..5531a1834f8c 100644
--- a/arch/x86/coco/tdx/attest.c
+++ b/arch/x86/coco/tdx/attest.c
@@ -13,16 +13,51 @@
 #include <linux/miscdevice.h>
 #include <linux/mm.h>
 #include <linux/io.h>
+#include <linux/set_memory.h>
+#include <linux/mutex.h>
 #include <asm/tdx.h>
+#include <asm/coco.h>
 #include <uapi/asm/tdx.h>
 
 #define DRIVER_NAME "tdx-attest"
 
 /* TDREPORT module call leaf ID */
 #define TDX_GET_REPORT			4
+/* GetQuote hypercall leaf ID */
+#define TDVMCALL_GET_QUOTE             0x10002
+
+/* Used for buffer allocation in GetQuote request */
+struct quote_buf {
+	void *vmaddr;
+	int count;
+};
+
+/* List entry of quote_list*/
+struct quote_entry {
+	bool valid;
+	struct quote_buf *buf;
+	struct list_head list;
+	struct completion compl;
+};
 
 static struct miscdevice miscdev;
 
+/*
+ * To support parallel GetQuote requests, use the list
+ * to track active GetQuote requests.
+ */
+static LIST_HEAD(quote_list);
+
+/* Lock to protect quote_list */
+static DEFINE_MUTEX(quote_lock);
+
+/*
+ * Workqueue to handle Quote data after Quote generation
+ * notification from VMM.
+ */
+struct workqueue_struct *quote_wq;
+struct work_struct quote_work;
+
 static long tdx_get_report(void __user *argp)
 {
 	void *reportdata = NULL, *tdreport = NULL;
@@ -71,6 +106,254 @@ static long tdx_get_report(void __user *argp)
 	return ret;
 }
 
+/* tdx_get_quote_hypercall() - Request to get TD Quote using TDREPORT */
+static long tdx_get_quote_hypercall(struct quote_buf *buf)
+{
+	struct tdx_hypercall_args args = {0};
+
+	args.r10 = TDX_HYPERCALL_STANDARD;
+	args.r11 = TDVMCALL_GET_QUOTE;
+	args.r12 = cc_mkdec(page_to_phys(vmalloc_to_page(buf->vmaddr)));
+	args.r13 = buf->count * PAGE_SIZE;
+
+	/*
+	 * Pass the physical address of TDREPORT to the VMM and
+	 * trigger the Quote generation. It is not a blocking
+	 * call, hence completion of this request will be notified to
+	 * the TD guest via a callback interrupt. More info about ABI
+	 * can be found in TDX Guest-Host-Communication Interface
+	 * (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
+	 */
+	return __tdx_hypercall(&args, 0);
+}
+
+/*
+ * alloc_quote_buf() - Used to allocate a shared buffer of
+ *		       given size.
+ *
+ * Size is page aligned and the allocated memory is decrypted
+ * to allow VMM access to it. Uses VMAP to create a shared mapping
+ * for the buffer to not affect the direct map.
+ */
+static struct quote_buf *alloc_quote_buf(u64 req_size)
+{
+	int size = PAGE_ALIGN(req_size);
+	void *addr = NULL, *vmaddr = NULL;
+	int count = size >> PAGE_SHIFT;
+	struct page **pages = NULL;
+	struct quote_buf *buf;
+	int i;
+
+	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return NULL;
+
+	addr = alloc_pages_exact(size, GFP_KERNEL);
+	if (!addr)
+		goto alloc_failed;
+
+	/* Allocate mem for array of page ptrs */
+	pages = kcalloc(count, sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		goto alloc_failed;
+
+	for (i = 0; i < count; i++)
+		pages[i] = virt_to_page(addr + i * PAGE_SIZE);
+
+	/*
+	 * Use VMAP to create shared mapping without affecting
+	 * the direct map. Use VM_MAP_PUT_PAGES to allow vmap()
+	 * responsible for freeing the pages when using vfree().
+	 */
+	vmaddr = vmap(pages, count, VM_MAP_PUT_PAGES, PAGE_KERNEL);
+	if (!vmaddr)
+		goto alloc_failed;
+
+	/* Use noalias variant to not affect the direct mapping */
+	if (set_memory_decrypted_noalias((unsigned long)vmaddr, count))
+		goto alloc_failed;
+
+	buf->vmaddr = vmaddr;
+	buf->count = count;
+
+	return buf;
+
+alloc_failed:
+	if (!vmaddr) {
+		kfree(pages);
+		if (addr)
+			free_pages_exact(addr, size);
+	}
+	vfree(vmaddr);
+	kfree(buf);
+	return NULL;
+}
+
+/* Remove the shared mapping and free the buffer */
+static void free_quote_buf(struct quote_buf *buf)
+{
+	if (!buf)
+		return;
+
+	/* Mark pages private */
+	if (set_memory_encrypted_noalias((unsigned long)buf->vmaddr,
+				buf->count)) {
+		pr_warn("Failed to encrypt %d pages at %p", buf->count,
+				buf->vmaddr);
+		return;
+	}
+
+	vfree(buf->vmaddr);
+	kfree(buf);
+}
+
+static struct quote_entry *alloc_quote_entry(u64 buf_len)
+{
+	struct quote_entry *entry = NULL;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return NULL;
+
+	/* Allocate buffer for quote request */
+	entry->buf = alloc_quote_buf(buf_len);
+	if (!entry->buf) {
+		kfree(entry);
+		return NULL;
+	}
+
+	init_completion(&entry->compl);
+	entry->valid = true;
+
+	return entry;
+}
+
+static void free_quote_entry(struct quote_entry *entry)
+{
+	free_quote_buf(entry->buf);
+	kfree(entry);
+}
+
+static void add_quote_entry(struct quote_entry *entry)
+{
+	mutex_lock(&quote_lock);
+	list_add_tail(&entry->list, &quote_list);
+	mutex_unlock(&quote_lock);
+}
+
+static void del_quote_entry(struct quote_entry *entry)
+{
+	mutex_lock(&quote_lock);
+	list_del(&entry->list);
+	mutex_unlock(&quote_lock);
+}
+
+static void invalidate_quote_request(struct quote_entry *entry)
+{
+	mutex_lock(&quote_lock);
+	entry->valid = false;
+	mutex_unlock(&quote_lock);
+}
+
+static long tdx_get_quote(void __user *argp)
+{
+	struct quote_entry *entry;
+	struct tdx_quote_req req;
+	struct quote_buf *buf;
+	long ret;
+
+	/* Copy GetQuote request struct from user buffer */
+	if (copy_from_user(&req, argp, sizeof(struct tdx_quote_req)))
+		return -EFAULT;
+
+	/* Make sure the length is valid */
+	if (!req.len)
+		return -EINVAL;
+
+	entry = alloc_quote_entry(req.len);
+	if (!entry)
+		return -ENOMEM;
+
+	buf = entry->buf;
+
+	/* Copy TDREPORT from user buffer to kernel Quote buffer */
+	if (copy_from_user(buf->vmaddr, (void __user *)req.buf, req.len)) {
+		free_quote_entry(entry);
+		return -EFAULT;
+	}
+
+	/* Submit GetQuote Request */
+	ret = tdx_get_quote_hypercall(buf);
+	if (ret) {
+		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
+		free_quote_entry(entry);
+		return -EIO;
+	}
+
+	/* Add current quote entry to quote_list to track active requests */
+	add_quote_entry(entry);
+
+	/* Wait for attestation completion */
+	ret = wait_for_completion_interruptible(&entry->compl);
+	if (ret < 0) {
+		/*
+		 * For premature termination, since VMM still owns the
+		 * shared buffer, mark the request invalid to let
+		 * quote_callback_handler() handle the memory cleanup
+		 * function.
+		 */
+		invalidate_quote_request(entry);
+		return -EINTR;
+	}
+
+	/*
+	 * If GetQuote request completed successfully, copy the result
+	 * back to the user and do the cleanup.
+	 */
+	if (copy_to_user((void __user *)req.buf, buf->vmaddr, req.len))
+		ret = -EIO;
+
+	/*
+	 * Reaching here means GetQuote request is processed
+	 * successfully. So do the cleanup.
+	 */
+	del_quote_entry(entry);
+	free_quote_entry(entry);
+
+	return 0;
+}
+
+static void attestation_callback_handler(void)
+{
+	queue_work(quote_wq, &quote_work);
+}
+
+static void quote_callback_handler(struct work_struct *work)
+{
+	struct tdx_quote_hdr *quote_hdr;
+	struct quote_entry *entry, *next;
+
+	/* Find processed quote request and mark it complete */
+	mutex_lock(&quote_lock);
+	list_for_each_entry_safe(entry, next, &quote_list, list) {
+		quote_hdr = (struct tdx_quote_hdr *)entry->buf->vmaddr;
+		if (quote_hdr->status == GET_QUOTE_IN_FLIGHT)
+			continue;
+		/*
+		 * If user invalidated the current request, remove the
+		 * entry from the quote list and free it. If the request
+		 * is still valid, mark it complete.
+		 */
+		if (entry->valid) {
+			complete(&entry->compl);
+		} else {
+			list_del(&entry->list);
+			free_quote_entry(entry);
+		}
+	}
+	mutex_unlock(&quote_lock);
+}
+
 static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
 			     unsigned long arg)
 {
@@ -81,6 +364,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
 	case TDX_CMD_GET_REPORT:
 		ret = tdx_get_report(argp);
 		break;
+	case TDX_CMD_GET_QUOTE:
+		ret = tdx_get_quote(argp);
+		break;
 	default:
 		pr_debug("cmd %d not supported\n", cmd);
 		break;
@@ -103,6 +389,13 @@ static int __init tdx_attestation_init(void)
 	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
 		return -EIO;
 
+	quote_wq = create_singlethread_workqueue("tdx_quote_handler");
+
+	INIT_WORK(&quote_work, quote_callback_handler);
+
+	/* Register attestation event notify handler */
+	tdx_setup_ev_notify_handler(attestation_callback_handler);
+
 	miscdev.name = DRIVER_NAME;
 	miscdev.minor = MISC_DYNAMIC_MINOR;
 	miscdev.fops = &tdx_attest_fops;
diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
index e06da56058a1..76a9e71e7b39 100644
--- a/arch/x86/include/uapi/asm/tdx.h
+++ b/arch/x86/include/uapi/asm/tdx.h
@@ -39,4 +39,49 @@ struct tdx_report_req {
  */
 #define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, struct tdx_report_req)
 
+/* struct tdx_quote_req: Request to generate TD Quote using TDREPORT
+ *
+ * @buf		: Pass user data that includes TDREPORT as input. Upon
+ *		  successful completion of IOCTL, output is copied
+ *		  back to the same buffer.
+ * @len		: Length of the buffer.
+ */
+struct tdx_quote_req {
+	__u64 buf;
+	__u64 len;
+};
+
+/*
+ * TDX_CMD_GET_QUOTE - Get TD Quote from QE/QGS using GetQuote
+ *		       TDVMCALL.
+ *
+ * Returns 0 on success, -EINTR for interrupted request, and
+ * standard errono on other failures.
+ */
+#define TDX_CMD_GET_QUOTE		_IOR('T', 0x02, struct tdx_quote_req)
+
+/* TD Quote status codes */
+#define GET_QUOTE_SUCCESS		0
+#define GET_QUOTE_IN_FLIGHT		0xffffffffffffffff
+#define GET_QUOTE_ERROR			0x8000000000000000
+#define GET_QUOTE_SERVICE_UNAVAILABLE	0x8000000000000001
+
+/*
+ * Format of Quote data header. More details can be found in TDX
+ * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
+ * section titled "TDG.VP.VMCALL<GetQuote>"
+ */
+struct tdx_quote_hdr {
+	/* Quote version, filled by TD */
+	__u64 version;
+	/* Status code of Quote request, filled by VMM */
+	__u64 status;
+	/* Length of TDREPORT, filled by TD */
+	__u32 in_len;
+	/* Length of Quote, filled by VMM */
+	__u32 out_len;
+	/* Actual Quote data */
+	__u64 data[0];
+};
+
 #endif /* _UAPI_ASM_X86_TDX_H */
-- 
2.25.1

Re: [PATCH v6 5/5] x86/tdx: Add Quote generation support

[Isaku Yamahata]

On Thu, May 12, 2022 at 03:19:52PM -0700,
Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:

> In TDX guest, the second stage in attestation process is to send the
> TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
> not support communication channels like vsock or TCP/IP, implement
> support to get TD Quote using hypercall. GetQuote hypercall can be used
> by the TD guest to request VMM facilitate the Quote generation via
> QE/QGS. More details about GetQuote hypercall can be found in TDX
> Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
> titled "TDG.VP.VMCALL<GetQuote>.
> 
> Since GetQuote is an asynchronous request hypercall, it will not block
> till the TD Quote is generated. So VMM uses callback interrupt vector
> configured by SetupEventNotifyInterrupt hypercall to notify the guest
> about Quote generation completion or failure.
> 
> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> with TDREPORT data as input, which is further used by the VMM to copy
> the TD Quote result after successful Quote generation. To create the
> shared buffer without breaking the direct map, allocate physically
> contiguous kernel memory and create a virtual mapping for it using
> vmap(). set_memory_*crypted_noalias() functions can be used to share or
> unshare the vmapped page without affecting the direct map.
> 
> Also note that, shared buffer allocation is currently handled in IOCTL
> handler, although it will increase the TDX_CMD_GET_QUOTE IOCTL response
> time, it is negligible compared to the time required for the quote
> generation completion. So IOCTL performance optimization is not
> considered at this time.
> 
> For shared buffer allocation, alternatives like using the DMA API is
> also considered. Although it simpler to use, it is not preferred because
> dma_alloc_*() APIs require a valid bus device as argument, which would
> need converting the attestation driver into a platform device driver.
> This is unnecessary, and since the attestation driver does not do real
> DMA, there is no need to use real DMA APIs.
> 
> Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
> submit GetQuote requests from the user space. Since Quote generation
> is an asynchronous request, IOCTL will block indefinitely for the VMM
> response in wait_for_completion_interruptible() call. Using this call
> will also add an option for the user to end the current request
> prematurely by raising any signals. This can be used by attestation
> agent to implement Quote generation timeout feature. If attestation
> agent is aware of time it can validly wait for QE/QGS response, then
> a possible timeout support can be implemented in the user application
> using signals. Quote generation timeout feature is currently not
> implemented in the driver because the current TDX specification does
> not have any recommendation for it.
> 
> After submitting the GetQuote request using hypercall, the shared buffer
> allocated for the current request is owned by the VMM. So, during this
> wait window, if the user terminates the request by raising a signal or
> by terminating the application, add a logic to do the memory cleanup
> after receiving the VMM response at a later time. Such memory cleanup
> support requires accepting the page again using TDX_ACCEPT_PAGE TDX
> Module call. So to not overload the callback IRQ handler, move the
> callback handler logic to a separate work queue.
> 
> To support parallel GetQuote requests, use linked list to track the
> active GetQuote requests and upon receiving the callback IRQ, loop
> through the active requests and mark the processed requests complete.
> Users can open multiple instances of the attestation device and send
> GetQuote requests in parallel.
> 
> 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>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  arch/x86/coco/tdx/attest.c      | 293 ++++++++++++++++++++++++++++++++
>  arch/x86/include/uapi/asm/tdx.h |  45 +++++
>  2 files changed, 338 insertions(+)
> 
> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
> index a5f4111f9b18..5531a1834f8c 100644
> --- a/arch/x86/coco/tdx/attest.c
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -13,16 +13,51 @@
>  #include <linux/miscdevice.h>
>  #include <linux/mm.h>
>  #include <linux/io.h>
> +#include <linux/set_memory.h>
> +#include <linux/mutex.h>
>  #include <asm/tdx.h>
> +#include <asm/coco.h>
>  #include <uapi/asm/tdx.h>
>  
>  #define DRIVER_NAME "tdx-attest"
>  
>  /* TDREPORT module call leaf ID */
>  #define TDX_GET_REPORT			4
> +/* GetQuote hypercall leaf ID */
> +#define TDVMCALL_GET_QUOTE             0x10002
> +
> +/* Used for buffer allocation in GetQuote request */
> +struct quote_buf {
> +	void *vmaddr;
> +	int count;
> +};
> +
> +/* List entry of quote_list*/
> +struct quote_entry {
> +	bool valid;
> +	struct quote_buf *buf;
> +	struct list_head list;
> +	struct completion compl;
> +};
>  
>  static struct miscdevice miscdev;
>  
> +/*
> + * To support parallel GetQuote requests, use the list
> + * to track active GetQuote requests.
> + */
> +static LIST_HEAD(quote_list);
> +
> +/* Lock to protect quote_list */
> +static DEFINE_MUTEX(quote_lock);
> +
> +/*
> + * Workqueue to handle Quote data after Quote generation
> + * notification from VMM.
> + */
> +struct workqueue_struct *quote_wq;
> +struct work_struct quote_work;
> +
>  static long tdx_get_report(void __user *argp)
>  {
>  	void *reportdata = NULL, *tdreport = NULL;
> @@ -71,6 +106,254 @@ static long tdx_get_report(void __user *argp)
>  	return ret;
>  }
>  
> +/* tdx_get_quote_hypercall() - Request to get TD Quote using TDREPORT */
> +static long tdx_get_quote_hypercall(struct quote_buf *buf)
> +{
> +	struct tdx_hypercall_args args = {0};
> +
> +	args.r10 = TDX_HYPERCALL_STANDARD;
> +	args.r11 = TDVMCALL_GET_QUOTE;
> +	args.r12 = cc_mkdec(page_to_phys(vmalloc_to_page(buf->vmaddr)));
> +	args.r13 = buf->count * PAGE_SIZE;
> +
> +	/*
> +	 * Pass the physical address of TDREPORT to the VMM and
> +	 * trigger the Quote generation. It is not a blocking
> +	 * call, hence completion of this request will be notified to
> +	 * the TD guest via a callback interrupt. More info about ABI
> +	 * can be found in TDX Guest-Host-Communication Interface
> +	 * (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
> +	 */
> +	return __tdx_hypercall(&args, 0);
> +}
> +
> +/*
> + * alloc_quote_buf() - Used to allocate a shared buffer of
> + *		       given size.
> + *
> + * Size is page aligned and the allocated memory is decrypted
> + * to allow VMM access to it. Uses VMAP to create a shared mapping
> + * for the buffer to not affect the direct map.
> + */
> +static struct quote_buf *alloc_quote_buf(u64 req_size)
> +{
> +	int size = PAGE_ALIGN(req_size);
> +	void *addr = NULL, *vmaddr = NULL;
> +	int count = size >> PAGE_SHIFT;
> +	struct page **pages = NULL;
> +	struct quote_buf *buf;
> +	int i;
> +
> +	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return NULL;
> +
> +	addr = alloc_pages_exact(size, GFP_KERNEL);
> +	if (!addr)
> +		goto alloc_failed;
> +
> +	/* Allocate mem for array of page ptrs */
> +	pages = kcalloc(count, sizeof(*pages), GFP_KERNEL);
> +	if (!pages)
> +		goto alloc_failed;
> +
> +	for (i = 0; i < count; i++)
> +		pages[i] = virt_to_page(addr + i * PAGE_SIZE);
> +
> +	/*
> +	 * Use VMAP to create shared mapping without affecting
> +	 * the direct map. Use VM_MAP_PUT_PAGES to allow vmap()
> +	 * responsible for freeing the pages when using vfree().
> +	 */
> +	vmaddr = vmap(pages, count, VM_MAP_PUT_PAGES, PAGE_KERNEL);
> +	if (!vmaddr)
> +		goto alloc_failed;
> +
> +	/* Use noalias variant to not affect the direct mapping */
> +	if (set_memory_decrypted_noalias((unsigned long)vmaddr, count))
> +		goto alloc_failed;
> +
> +	buf->vmaddr = vmaddr;
> +	buf->count = count;
> +
> +	return buf;
> +
> +alloc_failed:
> +	if (!vmaddr) {
> +		kfree(pages);
> +		if (addr)
> +			free_pages_exact(addr, size);
> +	}
> +	vfree(vmaddr);
> +	kfree(buf);
> +	return NULL;
> +}
> +
> +/* Remove the shared mapping and free the buffer */
> +static void free_quote_buf(struct quote_buf *buf)
> +{
> +	if (!buf)
> +		return;
> +
> +	/* Mark pages private */
> +	if (set_memory_encrypted_noalias((unsigned long)buf->vmaddr,
> +				buf->count)) {
> +		pr_warn("Failed to encrypt %d pages at %p", buf->count,
> +				buf->vmaddr);
> +		return;
> +	}
> +
> +	vfree(buf->vmaddr);
> +	kfree(buf);
> +}
> +
> +static struct quote_entry *alloc_quote_entry(u64 buf_len)
> +{
> +	struct quote_entry *entry = NULL;
> +
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return NULL;
> +
> +	/* Allocate buffer for quote request */
> +	entry->buf = alloc_quote_buf(buf_len);
> +	if (!entry->buf) {
> +		kfree(entry);
> +		return NULL;
> +	}
> +
> +	init_completion(&entry->compl);
> +	entry->valid = true;
> +
> +	return entry;
> +}
> +
> +static void free_quote_entry(struct quote_entry *entry)
> +{
> +	free_quote_buf(entry->buf);
> +	kfree(entry);
> +}
> +
> +static void add_quote_entry(struct quote_entry *entry)
> +{
> +	mutex_lock(&quote_lock);
> +	list_add_tail(&entry->list, &quote_list);
> +	mutex_unlock(&quote_lock);
> +}
> +
> +static void del_quote_entry(struct quote_entry *entry)
> +{
> +	mutex_lock(&quote_lock);
> +	list_del(&entry->list);
> +	mutex_unlock(&quote_lock);
> +}
> +
> +static void invalidate_quote_request(struct quote_entry *entry)
> +{
> +	mutex_lock(&quote_lock);
> +	entry->valid = false;
> +	mutex_unlock(&quote_lock);
> +}
> +
> +static long tdx_get_quote(void __user *argp)
> +{
> +	struct quote_entry *entry;
> +	struct tdx_quote_req req;
> +	struct quote_buf *buf;
> +	long ret;
> +
> +	/* Copy GetQuote request struct from user buffer */
> +	if (copy_from_user(&req, argp, sizeof(struct tdx_quote_req)))
> +		return -EFAULT;
> +
> +	/* Make sure the length is valid */
> +	if (!req.len)
> +		return -EINVAL;
> +
> +	entry = alloc_quote_entry(req.len);
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	buf = entry->buf;
> +
> +	/* Copy TDREPORT from user buffer to kernel Quote buffer */
> +	if (copy_from_user(buf->vmaddr, (void __user *)req.buf, req.len)) {
> +		free_quote_entry(entry);
> +		return -EFAULT;
> +	}
> +
> +	/* Submit GetQuote Request */
> +	ret = tdx_get_quote_hypercall(buf);
> +	if (ret) {
> +		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
> +		free_quote_entry(entry);
> +		return -EIO;
> +	}
> +
> +	/* Add current quote entry to quote_list to track active requests */
> +	add_quote_entry(entry);

There is a race condition. Interrupt can arrive after hypercall and before
list_add_tail and workqueue can run before reaching add_quote_entry().  lock
around both hypercall and list_add_tail. i.e. lock, hypercall, list_add_tail,
unlock.


> +
> +	/* Wait for attestation completion */
> +	ret = wait_for_completion_interruptible(&entry->compl);
> +	if (ret < 0) {
> +		/*
> +		 * For premature termination, since VMM still owns the
> +		 * shared buffer, mark the request invalid to let
> +		 * quote_callback_handler() handle the memory cleanup
> +		 * function.
> +		 */
> +		invalidate_quote_request(entry);

Interrupt can arrive after signal interrupt.  So invalidate_quote_request()
should check if the request is already processed, and return 0 or -EINTR.
Probably check the state always and del_list under single lock/unlock pair.


> +		return -EINTR;
> +	}
> +
> +	/*
> +	 * If GetQuote request completed successfully, copy the result
> +	 * back to the user and do the cleanup.
> +	 */
> +	if (copy_to_user((void __user *)req.buf, buf->vmaddr, req.len))
> +		ret = -EIO;

-EFAULT.


> +
> +	/*
> +	 * Reaching here means GetQuote request is processed
> +	 * successfully. So do the cleanup.
> +	 */
> +	del_quote_entry(entry);
> +	free_quote_entry(entry);
> +
> +	return 0;
> +}
> +
> +static void attestation_callback_handler(void)
> +{
> +	queue_work(quote_wq, &quote_work);
> +}
> +
> +static void quote_callback_handler(struct work_struct *work)
> +{
> +	struct tdx_quote_hdr *quote_hdr;
> +	struct quote_entry *entry, *next;
> +
> +	/* Find processed quote request and mark it complete */
> +	mutex_lock(&quote_lock);
> +	list_for_each_entry_safe(entry, next, &quote_list, list) {
> +		quote_hdr = (struct tdx_quote_hdr *)entry->buf->vmaddr;
> +		if (quote_hdr->status == GET_QUOTE_IN_FLIGHT)
> +			continue;
> +		/*
> +		 * If user invalidated the current request, remove the
> +		 * entry from the quote list and free it. If the request
> +		 * is still valid, mark it complete.
> +		 */
> +		if (entry->valid) {
> +			complete(&entry->compl);
> +		} else {
> +			list_del(&entry->list);
> +			free_quote_entry(entry);
> +		}
> +	}
> +	mutex_unlock(&quote_lock);
> +}
> +
>  static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>  			     unsigned long arg)
>  {
> @@ -81,6 +364,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>  	case TDX_CMD_GET_REPORT:
>  		ret = tdx_get_report(argp);
>  		break;
> +	case TDX_CMD_GET_QUOTE:
> +		ret = tdx_get_quote(argp);
> +		break;
>  	default:
>  		pr_debug("cmd %d not supported\n", cmd);
>  		break;
> @@ -103,6 +389,13 @@ static int __init tdx_attestation_init(void)
>  	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>  		return -EIO;
>  
> +	quote_wq = create_singlethread_workqueue("tdx_quote_handler");
> +
> +	INIT_WORK(&quote_work, quote_callback_handler);
> +
> +	/* Register attestation event notify handler */
> +	tdx_setup_ev_notify_handler(attestation_callback_handler);
> +
>  	miscdev.name = DRIVER_NAME;
>  	miscdev.minor = MISC_DYNAMIC_MINOR;
>  	miscdev.fops = &tdx_attest_fops;
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> index e06da56058a1..76a9e71e7b39 100644
> --- a/arch/x86/include/uapi/asm/tdx.h
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -39,4 +39,49 @@ struct tdx_report_req {
>   */
>  #define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, struct tdx_report_req)
>  
> +/* struct tdx_quote_req: Request to generate TD Quote using TDREPORT
> + *
> + * @buf		: Pass user data that includes TDREPORT as input. Upon
> + *		  successful completion of IOCTL, output is copied
> + *		  back to the same buffer.
> + * @len		: Length of the buffer.
> + */
> +struct tdx_quote_req {
> +	__u64 buf;
> +	__u64 len;
> +};
> +
> +/*
> + * TDX_CMD_GET_QUOTE - Get TD Quote from QE/QGS using GetQuote
> + *		       TDVMCALL.
> + *
> + * Returns 0 on success, -EINTR for interrupted request, and
> + * standard errono on other failures.
> + */
> +#define TDX_CMD_GET_QUOTE		_IOR('T', 0x02, struct tdx_quote_req)
> +
> +/* TD Quote status codes */
> +#define GET_QUOTE_SUCCESS		0
> +#define GET_QUOTE_IN_FLIGHT		0xffffffffffffffff
> +#define GET_QUOTE_ERROR			0x8000000000000000
> +#define GET_QUOTE_SERVICE_UNAVAILABLE	0x8000000000000001
> +
> +/*
> + * Format of Quote data header. More details can be found in TDX
> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> + * section titled "TDG.VP.VMCALL<GetQuote>"
> + */
> +struct tdx_quote_hdr {
> +	/* Quote version, filled by TD */
> +	__u64 version;
> +	/* Status code of Quote request, filled by VMM */
> +	__u64 status;
> +	/* Length of TDREPORT, filled by TD */
> +	__u32 in_len;
> +	/* Length of Quote, filled by VMM */
> +	__u32 out_len;
> +	/* Actual Quote data */
> +	__u64 data[0];
> +};
> +
>  #endif /* _UAPI_ASM_X86_TDX_H */
> -- 
> 2.25.1
> 

-- 
Isaku Yamahata <isaku.yamahata@gmail.com>

Re: [PATCH v6 5/5] x86/tdx: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

On 5/13/22 11:58 AM, Isaku Yamahata wrote:
> On Thu, May 12, 2022 at 03:19:52PM -0700,
> Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 
>> In TDX guest, the second stage in attestation process is to send the
>> TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
>> not support communication channels like vsock or TCP/IP, implement
>> support to get TD Quote using hypercall. GetQuote hypercall can be used
>> by the TD guest to request VMM facilitate the Quote generation via
>> QE/QGS. More details about GetQuote hypercall can be found in TDX
>> Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
>> titled "TDG.VP.VMCALL<GetQuote>.
>>
>> Since GetQuote is an asynchronous request hypercall, it will not block
>> till the TD Quote is generated. So VMM uses callback interrupt vector
>> configured by SetupEventNotifyInterrupt hypercall to notify the guest
>> about Quote generation completion or failure.
>>
>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
>> with TDREPORT data as input, which is further used by the VMM to copy
>> the TD Quote result after successful Quote generation. To create the
>> shared buffer without breaking the direct map, allocate physically
>> contiguous kernel memory and create a virtual mapping for it using
>> vmap(). set_memory_*crypted_noalias() functions can be used to share or
>> unshare the vmapped page without affecting the direct map.
>>
>> Also note that, shared buffer allocation is currently handled in IOCTL
>> handler, although it will increase the TDX_CMD_GET_QUOTE IOCTL response
>> time, it is negligible compared to the time required for the quote
>> generation completion. So IOCTL performance optimization is not
>> considered at this time.
>>
>> For shared buffer allocation, alternatives like using the DMA API is
>> also considered. Although it simpler to use, it is not preferred because
>> dma_alloc_*() APIs require a valid bus device as argument, which would
>> need converting the attestation driver into a platform device driver.
>> This is unnecessary, and since the attestation driver does not do real
>> DMA, there is no need to use real DMA APIs.
>>
>> Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
>> submit GetQuote requests from the user space. Since Quote generation
>> is an asynchronous request, IOCTL will block indefinitely for the VMM
>> response in wait_for_completion_interruptible() call. Using this call
>> will also add an option for the user to end the current request
>> prematurely by raising any signals. This can be used by attestation
>> agent to implement Quote generation timeout feature. If attestation
>> agent is aware of time it can validly wait for QE/QGS response, then
>> a possible timeout support can be implemented in the user application
>> using signals. Quote generation timeout feature is currently not
>> implemented in the driver because the current TDX specification does
>> not have any recommendation for it.
>>
>> After submitting the GetQuote request using hypercall, the shared buffer
>> allocated for the current request is owned by the VMM. So, during this
>> wait window, if the user terminates the request by raising a signal or
>> by terminating the application, add a logic to do the memory cleanup
>> after receiving the VMM response at a later time. Such memory cleanup
>> support requires accepting the page again using TDX_ACCEPT_PAGE TDX
>> Module call. So to not overload the callback IRQ handler, move the
>> callback handler logic to a separate work queue.
>>
>> To support parallel GetQuote requests, use linked list to track the
>> active GetQuote requests and upon receiving the callback IRQ, loop
>> through the active requests and mark the processed requests complete.
>> Users can open multiple instances of the attestation device and send
>> GetQuote requests in parallel.
>>
>> 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>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   arch/x86/coco/tdx/attest.c      | 293 ++++++++++++++++++++++++++++++++
>>   arch/x86/include/uapi/asm/tdx.h |  45 +++++
>>   2 files changed, 338 insertions(+)
>>
>> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
>> index a5f4111f9b18..5531a1834f8c 100644
>> --- a/arch/x86/coco/tdx/attest.c
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -13,16 +13,51 @@
>>   #include <linux/miscdevice.h>
>>   #include <linux/mm.h>
>>   #include <linux/io.h>
>> +#include <linux/set_memory.h>
>> +#include <linux/mutex.h>
>>   #include <asm/tdx.h>
>> +#include <asm/coco.h>
>>   #include <uapi/asm/tdx.h>
>>   
>>   #define DRIVER_NAME "tdx-attest"
>>   
>>   /* TDREPORT module call leaf ID */
>>   #define TDX_GET_REPORT			4
>> +/* GetQuote hypercall leaf ID */
>> +#define TDVMCALL_GET_QUOTE             0x10002
>> +
>> +/* Used for buffer allocation in GetQuote request */
>> +struct quote_buf {
>> +	void *vmaddr;
>> +	int count;
>> +};
>> +
>> +/* List entry of quote_list*/
>> +struct quote_entry {
>> +	bool valid;
>> +	struct quote_buf *buf;
>> +	struct list_head list;
>> +	struct completion compl;
>> +};
>>   
>>   static struct miscdevice miscdev;
>>   
>> +/*
>> + * To support parallel GetQuote requests, use the list
>> + * to track active GetQuote requests.
>> + */
>> +static LIST_HEAD(quote_list);
>> +
>> +/* Lock to protect quote_list */
>> +static DEFINE_MUTEX(quote_lock);
>> +
>> +/*
>> + * Workqueue to handle Quote data after Quote generation
>> + * notification from VMM.
>> + */
>> +struct workqueue_struct *quote_wq;
>> +struct work_struct quote_work;
>> +
>>   static long tdx_get_report(void __user *argp)
>>   {
>>   	void *reportdata = NULL, *tdreport = NULL;
>> @@ -71,6 +106,254 @@ static long tdx_get_report(void __user *argp)
>>   	return ret;
>>   }
>>   
>> +/* tdx_get_quote_hypercall() - Request to get TD Quote using TDREPORT */
>> +static long tdx_get_quote_hypercall(struct quote_buf *buf)
>> +{
>> +	struct tdx_hypercall_args args = {0};
>> +
>> +	args.r10 = TDX_HYPERCALL_STANDARD;
>> +	args.r11 = TDVMCALL_GET_QUOTE;
>> +	args.r12 = cc_mkdec(page_to_phys(vmalloc_to_page(buf->vmaddr)));
>> +	args.r13 = buf->count * PAGE_SIZE;
>> +
>> +	/*
>> +	 * Pass the physical address of TDREPORT to the VMM and
>> +	 * trigger the Quote generation. It is not a blocking
>> +	 * call, hence completion of this request will be notified to
>> +	 * the TD guest via a callback interrupt. More info about ABI
>> +	 * can be found in TDX Guest-Host-Communication Interface
>> +	 * (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
>> +	 */
>> +	return __tdx_hypercall(&args, 0);
>> +}
>> +
>> +/*
>> + * alloc_quote_buf() - Used to allocate a shared buffer of
>> + *		       given size.
>> + *
>> + * Size is page aligned and the allocated memory is decrypted
>> + * to allow VMM access to it. Uses VMAP to create a shared mapping
>> + * for the buffer to not affect the direct map.
>> + */
>> +static struct quote_buf *alloc_quote_buf(u64 req_size)
>> +{
>> +	int size = PAGE_ALIGN(req_size);
>> +	void *addr = NULL, *vmaddr = NULL;
>> +	int count = size >> PAGE_SHIFT;
>> +	struct page **pages = NULL;
>> +	struct quote_buf *buf;
>> +	int i;
>> +
>> +	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
>> +	if (!buf)
>> +		return NULL;
>> +
>> +	addr = alloc_pages_exact(size, GFP_KERNEL);
>> +	if (!addr)
>> +		goto alloc_failed;
>> +
>> +	/* Allocate mem for array of page ptrs */
>> +	pages = kcalloc(count, sizeof(*pages), GFP_KERNEL);
>> +	if (!pages)
>> +		goto alloc_failed;
>> +
>> +	for (i = 0; i < count; i++)
>> +		pages[i] = virt_to_page(addr + i * PAGE_SIZE);
>> +
>> +	/*
>> +	 * Use VMAP to create shared mapping without affecting
>> +	 * the direct map. Use VM_MAP_PUT_PAGES to allow vmap()
>> +	 * responsible for freeing the pages when using vfree().
>> +	 */
>> +	vmaddr = vmap(pages, count, VM_MAP_PUT_PAGES, PAGE_KERNEL);
>> +	if (!vmaddr)
>> +		goto alloc_failed;
>> +
>> +	/* Use noalias variant to not affect the direct mapping */
>> +	if (set_memory_decrypted_noalias((unsigned long)vmaddr, count))
>> +		goto alloc_failed;
>> +
>> +	buf->vmaddr = vmaddr;
>> +	buf->count = count;
>> +
>> +	return buf;
>> +
>> +alloc_failed:
>> +	if (!vmaddr) {
>> +		kfree(pages);
>> +		if (addr)
>> +			free_pages_exact(addr, size);
>> +	}
>> +	vfree(vmaddr);
>> +	kfree(buf);
>> +	return NULL;
>> +}
>> +
>> +/* Remove the shared mapping and free the buffer */
>> +static void free_quote_buf(struct quote_buf *buf)
>> +{
>> +	if (!buf)
>> +		return;
>> +
>> +	/* Mark pages private */
>> +	if (set_memory_encrypted_noalias((unsigned long)buf->vmaddr,
>> +				buf->count)) {
>> +		pr_warn("Failed to encrypt %d pages at %p", buf->count,
>> +				buf->vmaddr);
>> +		return;
>> +	}
>> +
>> +	vfree(buf->vmaddr);
>> +	kfree(buf);
>> +}
>> +
>> +static struct quote_entry *alloc_quote_entry(u64 buf_len)
>> +{
>> +	struct quote_entry *entry = NULL;
>> +
>> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>> +	if (!entry)
>> +		return NULL;
>> +
>> +	/* Allocate buffer for quote request */
>> +	entry->buf = alloc_quote_buf(buf_len);
>> +	if (!entry->buf) {
>> +		kfree(entry);
>> +		return NULL;
>> +	}
>> +
>> +	init_completion(&entry->compl);
>> +	entry->valid = true;
>> +
>> +	return entry;
>> +}
>> +
>> +static void free_quote_entry(struct quote_entry *entry)
>> +{
>> +	free_quote_buf(entry->buf);
>> +	kfree(entry);
>> +}
>> +
>> +static void add_quote_entry(struct quote_entry *entry)
>> +{
>> +	mutex_lock(&quote_lock);
>> +	list_add_tail(&entry->list, &quote_list);
>> +	mutex_unlock(&quote_lock);
>> +}
>> +
>> +static void del_quote_entry(struct quote_entry *entry)
>> +{
>> +	mutex_lock(&quote_lock);
>> +	list_del(&entry->list);
>> +	mutex_unlock(&quote_lock);
>> +}
>> +
>> +static void invalidate_quote_request(struct quote_entry *entry)
>> +{
>> +	mutex_lock(&quote_lock);
>> +	entry->valid = false;
>> +	mutex_unlock(&quote_lock);
>> +}
>> +
>> +static long tdx_get_quote(void __user *argp)
>> +{
>> +	struct quote_entry *entry;
>> +	struct tdx_quote_req req;
>> +	struct quote_buf *buf;
>> +	long ret;
>> +
>> +	/* Copy GetQuote request struct from user buffer */
>> +	if (copy_from_user(&req, argp, sizeof(struct tdx_quote_req)))
>> +		return -EFAULT;
>> +
>> +	/* Make sure the length is valid */
>> +	if (!req.len)
>> +		return -EINVAL;
>> +
>> +	entry = alloc_quote_entry(req.len);
>> +	if (!entry)
>> +		return -ENOMEM;
>> +
>> +	buf = entry->buf;
>> +
>> +	/* Copy TDREPORT from user buffer to kernel Quote buffer */
>> +	if (copy_from_user(buf->vmaddr, (void __user *)req.buf, req.len)) {
>> +		free_quote_entry(entry);
>> +		return -EFAULT;
>> +	}
>> +
>> +	/* Submit GetQuote Request */
>> +	ret = tdx_get_quote_hypercall(buf);
>> +	if (ret) {
>> +		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
>> +		free_quote_entry(entry);
>> +		return -EIO;
>> +	}
>> +
>> +	/* Add current quote entry to quote_list to track active requests */
>> +	add_quote_entry(entry);
> 
> There is a race condition. Interrupt can arrive after hypercall and before
> list_add_tail and workqueue can run before reaching add_quote_entry().  lock
> around both hypercall and list_add_tail. i.e. lock, hypercall, list_add_tail,
> unlock.

Good catch!. I will hold the hold the lock before hypercall as you have
suggested.

> 
> 
>> +
>> +	/* Wait for attestation completion */
>> +	ret = wait_for_completion_interruptible(&entry->compl);
>> +	if (ret < 0) {
>> +		/*
>> +		 * For premature termination, since VMM still owns the
>> +		 * shared buffer, mark the request invalid to let
>> +		 * quote_callback_handler() handle the memory cleanup
>> +		 * function.
>> +		 */
>> +		invalidate_quote_request(entry);
> 
> Interrupt can arrive after signal interrupt.  So invalidate_quote_request()
> should check if the request is already processed, and return 0 or -EINTR.
> Probably check the state always and del_list under single lock/unlock pair.

Agree. But I think we should return -EINTR for the interrupted case
irrespective of the processed status (so no return 0).

I will hold the lock and handle the cleanup for the processed
status.

> 
> 
>> +		return -EINTR;
>> +	}
>> +
>> +	/*
>> +	 * If GetQuote request completed successfully, copy the result
>> +	 * back to the user and do the cleanup.
>> +	 */
>> +	if (copy_to_user((void __user *)req.buf, buf->vmaddr, req.len))
>> +		ret = -EIO;
> 
> -EFAULT.
> 
> 
>> +
>> +	/*
>> +	 * Reaching here means GetQuote request is processed
>> +	 * successfully. So do the cleanup.
>> +	 */
>> +	del_quote_entry(entry);
>> +	free_quote_entry(entry);
>> +
>> +	return 0;
>> +}
>> +
>> +static void attestation_callback_handler(void)
>> +{
>> +	queue_work(quote_wq, &quote_work);
>> +}
>> +
>> +static void quote_callback_handler(struct work_struct *work)
>> +{
>> +	struct tdx_quote_hdr *quote_hdr;
>> +	struct quote_entry *entry, *next;
>> +
>> +	/* Find processed quote request and mark it complete */
>> +	mutex_lock(&quote_lock);
>> +	list_for_each_entry_safe(entry, next, &quote_list, list) {
>> +		quote_hdr = (struct tdx_quote_hdr *)entry->buf->vmaddr;
>> +		if (quote_hdr->status == GET_QUOTE_IN_FLIGHT)
>> +			continue;
>> +		/*
>> +		 * If user invalidated the current request, remove the
>> +		 * entry from the quote list and free it. If the request
>> +		 * is still valid, mark it complete.
>> +		 */
>> +		if (entry->valid) {
>> +			complete(&entry->compl);
>> +		} else {
>> +			list_del(&entry->list);
>> +			free_quote_entry(entry);
>> +		}
>> +	}
>> +	mutex_unlock(&quote_lock);
>> +}
>> +
>>   static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>>   			     unsigned long arg)
>>   {
>> @@ -81,6 +364,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>>   	case TDX_CMD_GET_REPORT:
>>   		ret = tdx_get_report(argp);
>>   		break;
>> +	case TDX_CMD_GET_QUOTE:
>> +		ret = tdx_get_quote(argp);
>> +		break;
>>   	default:
>>   		pr_debug("cmd %d not supported\n", cmd);
>>   		break;
>> @@ -103,6 +389,13 @@ static int __init tdx_attestation_init(void)
>>   	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>>   		return -EIO;
>>   
>> +	quote_wq = create_singlethread_workqueue("tdx_quote_handler");
>> +
>> +	INIT_WORK(&quote_work, quote_callback_handler);
>> +
>> +	/* Register attestation event notify handler */
>> +	tdx_setup_ev_notify_handler(attestation_callback_handler);
>> +
>>   	miscdev.name = DRIVER_NAME;
>>   	miscdev.minor = MISC_DYNAMIC_MINOR;
>>   	miscdev.fops = &tdx_attest_fops;
>> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
>> index e06da56058a1..76a9e71e7b39 100644
>> --- a/arch/x86/include/uapi/asm/tdx.h
>> +++ b/arch/x86/include/uapi/asm/tdx.h
>> @@ -39,4 +39,49 @@ struct tdx_report_req {
>>    */
>>   #define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, struct tdx_report_req)
>>   
>> +/* struct tdx_quote_req: Request to generate TD Quote using TDREPORT
>> + *
>> + * @buf		: Pass user data that includes TDREPORT as input. Upon
>> + *		  successful completion of IOCTL, output is copied
>> + *		  back to the same buffer.
>> + * @len		: Length of the buffer.
>> + */
>> +struct tdx_quote_req {
>> +	__u64 buf;
>> +	__u64 len;
>> +};
>> +
>> +/*
>> + * TDX_CMD_GET_QUOTE - Get TD Quote from QE/QGS using GetQuote
>> + *		       TDVMCALL.
>> + *
>> + * Returns 0 on success, -EINTR for interrupted request, and
>> + * standard errono on other failures.
>> + */
>> +#define TDX_CMD_GET_QUOTE		_IOR('T', 0x02, struct tdx_quote_req)
>> +
>> +/* TD Quote status codes */
>> +#define GET_QUOTE_SUCCESS		0
>> +#define GET_QUOTE_IN_FLIGHT		0xffffffffffffffff
>> +#define GET_QUOTE_ERROR			0x8000000000000000
>> +#define GET_QUOTE_SERVICE_UNAVAILABLE	0x8000000000000001
>> +
>> +/*
>> + * Format of Quote data header. More details can be found in TDX
>> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
>> + * section titled "TDG.VP.VMCALL<GetQuote>"
>> + */
>> +struct tdx_quote_hdr {
>> +	/* Quote version, filled by TD */
>> +	__u64 version;
>> +	/* Status code of Quote request, filled by VMM */
>> +	__u64 status;
>> +	/* Length of TDREPORT, filled by TD */
>> +	__u32 in_len;
>> +	/* Length of Quote, filled by VMM */
>> +	__u32 out_len;
>> +	/* Actual Quote data */
>> +	__u64 data[0];
>> +};
>> +
>>   #endif /* _UAPI_ASM_X86_TDX_H */
>> -- 
>> 2.25.1
>>
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Wander Lairson Costa]

On Thu, May 12, 2022 at 03:19:48PM -0700, Kuppuswamy Sathyanarayanan wrote:

[snip]

> +
> +static int __init tdx_attestation_init(void)
> +{
> +	long ret;
nit: the type of ret should be int
> +
> +	/* Make sure we are in a valid TDX platform */
> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EIO;
> +
> +	miscdev.name = DRIVER_NAME;
> +	miscdev.minor = MISC_DYNAMIC_MINOR;
> +	miscdev.fops = &tdx_attest_fops;
> +
> +	ret = misc_register(&miscdev);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[snip]

Re: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 5/16/22 11:08 AM, Wander Lairson Costa wrote:
>> +
>> +static int __init tdx_attestation_init(void)
>> +{
>> +	long ret;
> nit: the type of ret should be int

Agree. I will fix it in next version.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Kai Huang]

On Thu, 2022-05-12 at 15:19 -0700, Kuppuswamy Sathyanarayanan wrote:
> In TDX guest, attestation is used to verify the trustworthiness of a TD
> to other entities before provisioning secrets to the TD.
> 
> One usage example is, when a TD guest uses encrypted drive and if the
> decryption keys required to access the drive are stored in a secure 3rd
> party keyserver, the key server can use attestation to verify TD's
> trustworthiness and release the decryption keys to the TD.
> 
> The attestation process consists of two steps: TDREPORT generation and
> Quote generation.
> 
> TDREPORT (TDREPORT_STRUCT) is a fixed-size data structure generated by
> the TDX module which contains TD-specific information (such as TD
> measurements), platform security version, and the MAC to protect the
> integrity of the TDREPORT. The TD kernel uses TDCALL[TDG.MR.REPORT] to
> get the TDREPORT from the TDX module. A user-provided 64-Byte
> REPORTDATA is used as input and included in the TDREPORT. Typically it
> can be some nonce provided by attestation service so the TDREPORT can
> be verified uniquely. More details about TDREPORT can be found in
> Intel TDX Module specification, section titled "TDG.MR.REPORT Leaf".
> 
> Also note that the MAC added to the TDREPORT is bound to the platform.
> So TDREPORT can only be verified locally. 
> 

"bound to the platform, so ...".

> Intel SGX Quote Enclave (QE)
> is leveraged to verify the TDREPORT locally and convert it to a remote
> verifiable Quote to support remote attestation of the TDREPORT.


First of all, sorry for having to modify this back and forth for multi-times.

However this sounds like besides the SGX QE, there are other ways can be
leveraged to generate the Quote.  This isn't true based on current TDX
architecture.

So I still think below is slightly better:

TDREPORT can only be verified on local platform as the MAC key is bound to the
platform.  To support remote verification of the TDREPORT, TDX leverages Intel
SGX Quote Enclave (QE) to verify the TDREPORT locally and convert it to a remote
verifiable Quote.

> 
> After getting the TDREPORT, the second step of the attestation process
> is to send it to QE or Quote Generation Service (QGS) to generate a TD
> Quote. The QE sends the Quote back after it is generated. How is the
> data sent and received is QE implementation and deployment specific.TD
> userspace attestation software can use whatever communication channel
> available (i.e. tcp/ip, vsock) to communicate with the QE using whatever
> data format. TDX also defines TDVMCALLs to allow TD to ask VMM to
> facilitate sending/receiving data between TD attestation software and
> the QE. This support is documented in GHCI 1.0 spec "5.4 TD attestation".

This paragraph is used to provide more information to help to justify the
decision to provide a way to let userspace to get the TDREPORT.  Now looks this
paragraph has details not quite related to this patch.  For instance, I am not
sure whether we need to mention TDVMCALL at all.

Also, it _may_ be helpful to point out we cannot have QE inside the TD since TDX
doesn't support SGX within the TD, otherwise it's completely possible that the
TD attestation agent can just implement the QE by itself therefore doesn't need
any communication channel at all.

Anyway, perhaps just:

"
After getting the TDREPORT, the second step of the attestation process is to
send it to the QE to generate the Quote.  TDX doesn't support SGX inside the TD,
so the QE can be deployed in the host, or in another legacy VM with SGX support.
How to send the TDREPORT to QE and receive the Quote is implementation and
deployment specific.  

Implement a basic attestation driver to allow TD userspace to get the TDREPORT.
The TD userspace attestation software can get the TDREPORT and then choose
whatever communication channel available (i.e. vsock) to send the TDREPORT to QE
and receive the Quote.
"

Anyway I am not good at writing changelog so will leave to others in the future.

> 
> Implement a basic attestation driver to allow TD userspace to get the
> TDREPORT, which is sent to QE by the attestation software to generate
> a Quote for remote verification.
> 
> 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.
> 
> Operations like getting TDREPORT or Quote generation involves sending
> a blob of data as input and getting another blob of data as output. 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 for this use case. Also, the REPORTDATA used
> in TDREPORT generation can possibly come from attestation service to
> uniquely verify the Quote (like per instance verification). In such
> case, since REPORTDATA is a secret, using sysfs to share it is insecure
> compared to sending it via IOCTL.
> 
> 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>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  arch/x86/coco/tdx/Makefile      |   2 +-
>  arch/x86/coco/tdx/attest.c      | 118 ++++++++++++++++++++++++++++++++
>  arch/x86/include/uapi/asm/tdx.h |  42 ++++++++++++
>  3 files changed, 161 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/coco/tdx/attest.c
>  create mode 100644 arch/x86/include/uapi/asm/tdx.h
> 
> diff --git a/arch/x86/coco/tdx/Makefile b/arch/x86/coco/tdx/Makefile
> index 46c55998557d..d2db3e6770e5 100644
> --- a/arch/x86/coco/tdx/Makefile
> +++ b/arch/x86/coco/tdx/Makefile
> @@ -1,3 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -obj-y += tdx.o tdcall.o
> +obj-y += tdx.o tdcall.o attest.o
> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
> new file mode 100644
> index 000000000000..a5f4111f9b18
> --- /dev/null
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process.
> + *
> + * Copyright (C) 2022 Intel Corporation
> + *
> + */
> +
> +#define pr_fmt(fmt) "x86/tdx: attest: " fmt
> +
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/io.h>
> +#include <asm/tdx.h>
> +#include <uapi/asm/tdx.h>
> +
> +#define DRIVER_NAME "tdx-attest"
> +
> +/* TDREPORT module call leaf ID */
> +#define TDX_GET_REPORT			4
> +

Looks more white spaces than needed.

> +static struct miscdevice miscdev;
> +
> +static long tdx_get_report(void __user *argp)
> +{
> +	void *reportdata = NULL, *tdreport = NULL;
> +	long ret = 0;
> +
> +	/* Allocate buffer space for REPORTDATA */
> +	reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
> +	if (!reportdata)
> +		return -ENOMEM;
> +
> +	/* Allocate buffer space for TDREPORT */
> +	tdreport = kmalloc(TDX_REPORT_LEN, GFP_KERNEL);
> +	if (!tdreport) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}

Perhaps you can allocate a single page, and use the first half as REPORTDATA and
the second part as TDREPORT.  In this case, you can save one memory allocation
to simplify the code.  The page will be freed anyway after this IOCTL.

> +
> +	/* Copy REPORTDATA from the user buffer */
> +	if (copy_from_user(reportdata, argp, TDX_REPORTDATA_LEN)) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
> +	 *
> +	 * Get the TDREPORT using REPORTDATA as input. Refer to
> +	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
> +	 * Specification for detailed information.
> +	 */
> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
> +				virt_to_phys(reportdata), 0, 0, NULL);
> +	if (ret) {
> +		pr_debug("TDREPORT TDCALL failed, status:%lx\n", ret);
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	/* Copy TDREPORT back to the user buffer */
> +	if (copy_to_user(argp, tdreport, TDX_REPORT_LEN))
> +		ret = -EFAULT;
> +
> +out:
> +	kfree(reportdata);
> +	kfree(tdreport);
> +	return ret;
> +}
> +
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	long ret = -EINVAL;
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_REPORT:
> +		ret = tdx_get_report(argp);
> +		break;
> +	default:
> +		pr_debug("cmd %d not supported\n", cmd);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct file_operations tdx_attest_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= tdx_attest_ioctl,
> +	.llseek		= no_llseek,
> +};
> +
> +static int __init tdx_attestation_init(void)
> +{
> +	long ret;
> +
> +	/* Make sure we are in a valid TDX platform */
> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EIO;
> +
> +	miscdev.name = DRIVER_NAME;
> +	miscdev.minor = MISC_DYNAMIC_MINOR;
> +	miscdev.fops = &tdx_attest_fops;
> +
> +	ret = misc_register(&miscdev);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +device_initcall(tdx_attestation_init)
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> new file mode 100644
> index 000000000000..e06da56058a1
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_ASM_X86_TDX_H
> +#define _UAPI_ASM_X86_TDX_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
> +#define TDX_REPORTDATA_LEN		64
> +
> +/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
> +#define TDX_REPORT_LEN			1024
> +
> +/**
> + * struct tdx_report_req: Get TDREPORT using REPORTDATA as input.
> + *
> + * @reportdata : User-defined 64-Byte REPORTDATA to be included into
> + *		 TDREPORT. Typically it can be some nonce provided by
> + *		 attestation software so the generated TDREPORT can be

"attestation software" -> "attestation service" or "verification service".  Or
just remove the second sentence.  Anyway, it's user-defined.

> + *		 uniquely verified.
> + * @tdreport   : TDREPORT output from TDCALL[TDG.MR.REPORT] of size
> + *		 TDX_REPORT_LEN.
> + *
> + * Used in TDX_CMD_GET_REPORT IOCTL request.
> + */
> +struct tdx_report_req {
> +	union {
> +		__u8 reportdata[TDX_REPORTDATA_LEN];
> +		__u8 tdreport[TDX_REPORT_LEN];
> +	};
> +};

As a userspace ABI, one concern is this doesn't provide any space for future
extension.  But probably it's OK since I don't see any possible additional input
for now.  And although TDREPORT may have additional information in future
generation of TDX but the spec says the size is 1024 so perhaps this won't
change even in the future.

Anyway will leave to others.

> +
> +/*
> + * TDX_CMD_GET_REPORT - Get TDREPORT using TDCALL[TDG.MR.REPORT)

TDCALL[TDG.MR.REPORT) -> TDCALL[TDG.MR.REPORT]

> + *
> + * Return 0 on success, -EIO on TDCALL execution failure, and
> + * standard errno on other general error cases.
> + *
> + */
> +#define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, struct tdx_report_req)
> +
> +#endif /* _UAPI_ASM_X86_TDX_H */

Re: [PATCH v6 5/5] x86/tdx: Add Quote generation support

[Kai Huang]

On Fri, 2022-05-13 at 12:29 -0700, Sathyanarayanan Kuppuswamy wrote:
> > 
> > 
> > > +
> > > +	/* Wait for attestation completion */
> > > +	ret = wait_for_completion_interruptible(&entry->compl);
> > > +	if (ret < 0) {
> > > +		/*
> > > +		 * For premature termination, since VMM still owns the
> > > +		 * shared buffer, mark the request invalid to let
> > > +		 * quote_callback_handler() handle the memory cleanup
> > > +		 * function.
> > > +		 */
> > > +		invalidate_quote_request(entry);
> > 
> > Interrupt can arrive after signal interrupt.  So invalidate_quote_request()
> > should check if the request is already processed, and return 0 or -EINTR.
> > Probably check the state always and del_list under single lock/unlock pair.
> 
> Agree. But I think we should return -EINTR for the interrupted case
> irrespective of the processed status (so no return 0).
> 
> I will hold the lock and handle the cleanup for the processed
> status.

Even if we check the buffer status in invalidate_quote_request(), there's no
guarantee the VMM won't change the buffer status _after_ we do the check, so
looks such check isn't necessary.

-- 
Thanks,
-Kai

Re: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Kai,

On 5/16/22 7:54 PM, Kai Huang wrote:
> On Thu, 2022-05-12 at 15:19 -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, attestation is used to verify the trustworthiness of a TD
>> to other entities before provisioning secrets to the TD.
>>
>> One usage example is, when a TD guest uses encrypted drive and if the
>> decryption keys required to access the drive are stored in a secure 3rd
>> party keyserver, the key server can use attestation to verify TD's
>> trustworthiness and release the decryption keys to the TD.
>>
>> The attestation process consists of two steps: TDREPORT generation and
>> Quote generation.
>>
>> TDREPORT (TDREPORT_STRUCT) is a fixed-size data structure generated by
>> the TDX module which contains TD-specific information (such as TD
>> measurements), platform security version, and the MAC to protect the
>> integrity of the TDREPORT. The TD kernel uses TDCALL[TDG.MR.REPORT] to
>> get the TDREPORT from the TDX module. A user-provided 64-Byte
>> REPORTDATA is used as input and included in the TDREPORT. Typically it
>> can be some nonce provided by attestation service so the TDREPORT can
>> be verified uniquely. More details about TDREPORT can be found in
>> Intel TDX Module specification, section titled "TDG.MR.REPORT Leaf".
>>
>> Also note that the MAC added to the TDREPORT is bound to the platform.
>> So TDREPORT can only be verified locally.
>>
> 
> "bound to the platform, so ...".
> 
>> Intel SGX Quote Enclave (QE)
>> is leveraged to verify the TDREPORT locally and convert it to a remote
>> verifiable Quote to support remote attestation of the TDREPORT.
> 
> 
> First of all, sorry for having to modify this back and forth for multi-times.
> 
> However this sounds like besides the SGX QE, there are other ways can be
> leveraged to generate the Quote.  This isn't true based on current TDX
> architecture.
> 
> So I still think below is slightly better:
> 
> TDREPORT can only be verified on local platform as the MAC key is bound to the
> platform.  To support remote verification of the TDREPORT, TDX leverages Intel
> SGX Quote Enclave (QE) to verify the TDREPORT locally and convert it to a remote
> verifiable Quote.
> 
>>
>> After getting the TDREPORT, the second step of the attestation process
>> is to send it to QE or Quote Generation Service (QGS) to generate a TD
>> Quote. The QE sends the Quote back after it is generated. How is the
>> data sent and received is QE implementation and deployment specific.TD
>> userspace attestation software can use whatever communication channel
>> available (i.e. tcp/ip, vsock) to communicate with the QE using whatever
>> data format. TDX also defines TDVMCALLs to allow TD to ask VMM to
>> facilitate sending/receiving data between TD attestation software and
>> the QE. This support is documented in GHCI 1.0 spec "5.4 TD attestation".
> 
> This paragraph is used to provide more information to help to justify the
> decision to provide a way to let userspace to get the TDREPORT.  Now looks this
> paragraph has details not quite related to this patch.  For instance, I am not
> sure whether we need to mention TDVMCALL at all.
> 
> Also, it _may_ be helpful to point out we cannot have QE inside the TD since TDX
> doesn't support SGX within the TD, otherwise it's completely possible that the
> TD attestation agent can just implement the QE by itself therefore doesn't need
> any communication channel at all.
> 
> Anyway, perhaps just:
> 
> "
> After getting the TDREPORT, the second step of the attestation process is to
> send it to the QE to generate the Quote.  TDX doesn't support SGX inside the TD,
> so the QE can be deployed in the host, or in another legacy VM with SGX support.
> How to send the TDREPORT to QE and receive the Quote is implementation and
> deployment specific.
> 
> Implement a basic attestation driver to allow TD userspace to get the TDREPORT.
> The TD userspace attestation software can get the TDREPORT and then choose
> whatever communication channel available (i.e. vsock) to send the TDREPORT to QE
> and receive the Quote.
> "
> 
> Anyway I am not good at writing changelog so will leave to others in the future.

Ok. I am fine with suggested changes. I will include them.

> 
>>
>> Implement a basic attestation driver to allow TD userspace to get the
>> TDREPORT, which is sent to QE by the attestation software to generate
>> a Quote for remote verification.
>>
>> 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.
>>
>> Operations like getting TDREPORT or Quote generation involves sending
>> a blob of data as input and getting another blob of data as output. 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 for this use case. Also, the REPORTDATA used
>> in TDREPORT generation can possibly come from attestation service to
>> uniquely verify the Quote (like per instance verification). In such
>> case, since REPORTDATA is a secret, using sysfs to share it is insecure
>> compared to sending it via IOCTL.
>>
>> 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>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   arch/x86/coco/tdx/Makefile      |   2 +-
>>   arch/x86/coco/tdx/attest.c      | 118 ++++++++++++++++++++++++++++++++
>>   arch/x86/include/uapi/asm/tdx.h |  42 ++++++++++++
>>   3 files changed, 161 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/x86/coco/tdx/attest.c
>>   create mode 100644 arch/x86/include/uapi/asm/tdx.h
>>
>> diff --git a/arch/x86/coco/tdx/Makefile b/arch/x86/coco/tdx/Makefile
>> index 46c55998557d..d2db3e6770e5 100644
>> --- a/arch/x86/coco/tdx/Makefile
>> +++ b/arch/x86/coco/tdx/Makefile
>> @@ -1,3 +1,3 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>   
>> -obj-y += tdx.o tdcall.o
>> +obj-y += tdx.o tdcall.o attest.o
>> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
>> new file mode 100644
>> index 000000000000..a5f4111f9b18
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * attest.c - TDX guest attestation interface driver.
>> + *
>> + * Implements user interface to trigger attestation process.
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) "x86/tdx: attest: " fmt
>> +
>> +#include <linux/miscdevice.h>
>> +#include <linux/mm.h>
>> +#include <linux/io.h>
>> +#include <asm/tdx.h>
>> +#include <uapi/asm/tdx.h>
>> +
>> +#define DRIVER_NAME "tdx-attest"
>> +
>> +/* TDREPORT module call leaf ID */
>> +#define TDX_GET_REPORT			4
>> +
> 
> Looks more white spaces than needed.

I left some extra space to align with future macro additions.

> 
>> +static struct miscdevice miscdev;
>> +
>> +static long tdx_get_report(void __user *argp)
>> +{
>> +	void *reportdata = NULL, *tdreport = NULL;
>> +	long ret = 0;
>> +
>> +	/* Allocate buffer space for REPORTDATA */
>> +	reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
>> +	if (!reportdata)
>> +		return -ENOMEM;
>> +
>> +	/* Allocate buffer space for TDREPORT */
>> +	tdreport = kmalloc(TDX_REPORT_LEN, GFP_KERNEL);
>> +	if (!tdreport) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
> 
> Perhaps you can allocate a single page, and use the first half as REPORTDATA and
> the second part as TDREPORT.  In this case, you can save one memory allocation
> to simplify the code.  The page will be freed anyway after this IOCTL.

IMO, I think it is more clear doing it this way (one for input and and
other for output). We only need ~1K(+ 64B) space for our use case. So
there is no need allocate a separate page for it. Also, it is much
easier to understand compared to allocating single buffer and
diving the buffer in half between them. So if it is not a big probelem 
lets leave it this way.


> 
>> +
>> +	/* Copy REPORTDATA from the user buffer */
>> +	if (copy_from_user(reportdata, argp, TDX_REPORTDATA_LEN)) {
>> +		ret = -EFAULT;
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
>> +	 *
>> +	 * Get the TDREPORT using REPORTDATA as input. Refer to
>> +	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
>> +	 * Specification for detailed information.
>> +	 */
>> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
>> +				virt_to_phys(reportdata), 0, 0, NULL);
>> +	if (ret) {
>> +		pr_debug("TDREPORT TDCALL failed, status:%lx\n", ret);
>> +		ret = -EIO;
>> +		goto out;
>> +	}
>> +
>> +	/* Copy TDREPORT back to the user buffer */
>> +	if (copy_to_user(argp, tdreport, TDX_REPORT_LEN))
>> +		ret = -EFAULT;
>> +
>> +out:
>> +	kfree(reportdata);
>> +	kfree(tdreport);
>> +	return ret;
>> +}
>> +
>> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>> +			     unsigned long arg)
>> +{
>> +	void __user *argp = (void __user *)arg;
>> +	long ret = -EINVAL;
>> +
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_REPORT:
>> +		ret = tdx_get_report(argp);
>> +		break;
>> +	default:
>> +		pr_debug("cmd %d not supported\n", cmd);
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations tdx_attest_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.unlocked_ioctl	= tdx_attest_ioctl,
>> +	.llseek		= no_llseek,
>> +};
>> +
>> +static int __init tdx_attestation_init(void)
>> +{
>> +	long ret;
>> +
>> +	/* Make sure we are in a valid TDX platform */
>> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EIO;
>> +
>> +	miscdev.name = DRIVER_NAME;
>> +	miscdev.minor = MISC_DYNAMIC_MINOR;
>> +	miscdev.fops = &tdx_attest_fops;
>> +
>> +	ret = misc_register(&miscdev);
>> +	if (ret) {
>> +		pr_err("misc device registration failed\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +device_initcall(tdx_attestation_init)
>> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
>> new file mode 100644
>> index 000000000000..e06da56058a1
>> --- /dev/null
>> +++ b/arch/x86/include/uapi/asm/tdx.h
>> @@ -0,0 +1,42 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef _UAPI_ASM_X86_TDX_H
>> +#define _UAPI_ASM_X86_TDX_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/ioctl.h>
>> +
>> +/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
>> +#define TDX_REPORTDATA_LEN		64
>> +
>> +/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
>> +#define TDX_REPORT_LEN			1024
>> +
>> +/**
>> + * struct tdx_report_req: Get TDREPORT using REPORTDATA as input.
>> + *
>> + * @reportdata : User-defined 64-Byte REPORTDATA to be included into
>> + *		 TDREPORT. Typically it can be some nonce provided by
>> + *		 attestation software so the generated TDREPORT can be
> 
> "attestation software" -> "attestation service" or "verification service".  Or
> just remove the second sentence.  Anyway, it's user-defined.

I will change it to attestation service. We don't need to remove it.
Anyway more info is better.

> 
>> + *		 uniquely verified.
>> + * @tdreport   : TDREPORT output from TDCALL[TDG.MR.REPORT] of size
>> + *		 TDX_REPORT_LEN.
>> + *
>> + * Used in TDX_CMD_GET_REPORT IOCTL request.
>> + */
>> +struct tdx_report_req {
>> +	union {
>> +		__u8 reportdata[TDX_REPORTDATA_LEN];
>> +		__u8 tdreport[TDX_REPORT_LEN];
>> +	};
>> +};
> 
> As a userspace ABI, one concern is this doesn't provide any space for future
> extension.  But probably it's OK since I don't see any possible additional input
> for now.  And although TDREPORT may have additional information in future
> generation of TDX but the spec says the size is 1024 so perhaps this won't
> change even in the future.
> 
> Anyway will leave to others.

IMO, if the spec changes in future we can revisit it.

> 
>> +
>> +/*
>> + * TDX_CMD_GET_REPORT - Get TDREPORT using TDCALL[TDG.MR.REPORT)
> 
> TDCALL[TDG.MR.REPORT) -> TDCALL[TDG.MR.REPORT]

Ok.

> 
>> + *
>> + * Return 0 on success, -EIO on TDCALL execution failure, and
>> + * standard errno on other general error cases.
>> + *
>> + */
>> +#define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, struct tdx_report_req)
>> +
>> +#endif /* _UAPI_ASM_X86_TDX_H */
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v6 5/5] x86/tdx: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

On 5/16/22 7:58 PM, Kai Huang wrote:
> On Fri, 2022-05-13 at 12:29 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>
>>>
>>>> +
>>>> +	/* Wait for attestation completion */
>>>> +	ret = wait_for_completion_interruptible(&entry->compl);
>>>> +	if (ret < 0) {
>>>> +		/*
>>>> +		 * For premature termination, since VMM still owns the
>>>> +		 * shared buffer, mark the request invalid to let
>>>> +		 * quote_callback_handler() handle the memory cleanup
>>>> +		 * function.
>>>> +		 */
>>>> +		invalidate_quote_request(entry);
>>>
>>> Interrupt can arrive after signal interrupt.  So invalidate_quote_request()
>>> should check if the request is already processed, and return 0 or -EINTR.
>>> Probably check the state always and del_list under single lock/unlock pair.
>>
>> Agree. But I think we should return -EINTR for the interrupted case
>> irrespective of the processed status (so no return 0).
>>
>> I will hold the lock and handle the cleanup for the processed
>> status.
> 
> Even if we check the buffer status in invalidate_quote_request(), there's no
> guarantee the VMM won't change the buffer status _after_ we do the check, so
> looks such check isn't necessary.
> 

Consider the case where we get a callback interrupt, and before we
complete the processing for it, user terminates the request. In this
scenario,  quote_callback_handler() will consider the request is
still valid and no do the memory cleanup. To handle this case,
we need to check the status in invalidate_quote_request() and do
the cleanup if required.

/* Handles early termination of GetQuote requests */
void invalidate_quote_request(struct quote_entry *entry)
{
         struct tdx_quote_hdr *quote_hdr;

         /*
          * For early termination, if the request is not yet
          * processed by VMM (GET_QUOTE_IN_FLIGHT), the VMM
          * still owns the shared buffer, so mark the request
          * invalid to let quote_callback_handler() handle the
          * memory cleanup function. If the request is already
          * processed, then do the cleanup and return.
          */

         mutex_lock(&quote_lock);
         quote_hdr = (struct tdx_quote_hdr *)entry->buf->vmaddr;
         if (quote_hdr->status == GET_QUOTE_IN_FLIGHT) {
                 entry->valid = false;
                 mutex_unlock(&quote_lock);
                 return;
         }
         _del_quote_entry(entry);
         mutex_unlock(&quote_lock);
}


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v6 5/5] x86/tdx: Add Quote generation support

[Kai Huang]

On Tue, 2022-05-17 at 13:08 -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> On 5/16/22 7:58 PM, Kai Huang wrote:
> > On Fri, 2022-05-13 at 12:29 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > > 
> > > > 
> > > > > +
> > > > > +	/* Wait for attestation completion */
> > > > > +	ret = wait_for_completion_interruptible(&entry->compl);
> > > > > +	if (ret < 0) {
> > > > > +		/*
> > > > > +		 * For premature termination, since VMM still owns the
> > > > > +		 * shared buffer, mark the request invalid to let
> > > > > +		 * quote_callback_handler() handle the memory cleanup
> > > > > +		 * function.
> > > > > +		 */
> > > > > +		invalidate_quote_request(entry);
> > > > 
> > > > Interrupt can arrive after signal interrupt.  So invalidate_quote_request()
> > > > should check if the request is already processed, and return 0 or -EINTR.
> > > > Probably check the state always and del_list under single lock/unlock pair.
> > > 
> > > Agree. But I think we should return -EINTR for the interrupted case
> > > irrespective of the processed status (so no return 0).
> > > 
> > > I will hold the lock and handle the cleanup for the processed
> > > status.
> > 
> > Even if we check the buffer status in invalidate_quote_request(), there's no
> > guarantee the VMM won't change the buffer status _after_ we do the check, so
> > looks such check isn't necessary.
> > 
> 
> Consider the case where we get a callback interrupt, and before we
> complete the processing for it, user terminates the request. In this
> scenario,  quote_callback_handler() will consider the request is
> still valid and no do the memory cleanup. To handle this case,
> we need to check the status in invalidate_quote_request() and do
> the cleanup if required.
> 
> /* Handles early termination of GetQuote requests */
> void invalidate_quote_request(struct quote_entry *entry)
> {
>          struct tdx_quote_hdr *quote_hdr;
> 
>          /*
>           * For early termination, if the request is not yet
>           * processed by VMM (GET_QUOTE_IN_FLIGHT), the VMM
>           * still owns the shared buffer, so mark the request
>           * invalid to let quote_callback_handler() handle the
>           * memory cleanup function. If the request is already
>           * processed, then do the cleanup and return.
>           */
> 
>          mutex_lock(&quote_lock);
>          quote_hdr = (struct tdx_quote_hdr *)entry->buf->vmaddr;
>          if (quote_hdr->status == GET_QUOTE_IN_FLIGHT) {

What prevents VMM from updating quote_hdr->status from IN_FLIGHT to DONE _after_
this check?

If you want to add such check, you should check against GET_QUOTE_DONE, but not
IN_FLIGHT.  Only after status is DONE,  VMM will not update the buffer.  Perhaps
something like below:

	mutex_lock(&quote_lock);
	/* Skip invalidate the buffer if VMM has done with the buffer */
	if (quote_hdr->status == GET_QUOTE_DONE) {
		mutex_unlock(&quote_lock);
		return 0;
	}

And in the IOCTL, you can perhaps to choose to return 0, instead of -EINTR in
this case, as the Quote has been finished already.

But I am not sure whether this is necessary.  The worst case is one finished
Quote is wasted I guess.

>                  entry->valid = false;
>                  mutex_unlock(&quote_lock);
>                  return;
>          }
>          _del_quote_entry(entry);
>          mutex_unlock(&quote_lock);
> }
> 
> 

-- 
Thanks,
-Kai

Re: [PATCH v6 5/5] x86/tdx: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

On 5/17/22 4:06 PM, Kai Huang wrote:
> On Tue, 2022-05-17 at 13:08 -0700, Sathyanarayanan Kuppuswamy wrote:
>>
>> On 5/16/22 7:58 PM, Kai Huang wrote:
>>> On Fri, 2022-05-13 at 12:29 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>>>
>>>>>
>>>>>> +
>>>>>> +	/* Wait for attestation completion */
>>>>>> +	ret = wait_for_completion_interruptible(&entry->compl);
>>>>>> +	if (ret < 0) {
>>>>>> +		/*
>>>>>> +		 * For premature termination, since VMM still owns the
>>>>>> +		 * shared buffer, mark the request invalid to let
>>>>>> +		 * quote_callback_handler() handle the memory cleanup
>>>>>> +		 * function.
>>>>>> +		 */
>>>>>> +		invalidate_quote_request(entry);
>>>>>
>>>>> Interrupt can arrive after signal interrupt.  So invalidate_quote_request()
>>>>> should check if the request is already processed, and return 0 or -EINTR.
>>>>> Probably check the state always and del_list under single lock/unlock pair.
>>>>
>>>> Agree. But I think we should return -EINTR for the interrupted case
>>>> irrespective of the processed status (so no return 0).
>>>>
>>>> I will hold the lock and handle the cleanup for the processed
>>>> status.
>>>
>>> Even if we check the buffer status in invalidate_quote_request(), there's no
>>> guarantee the VMM won't change the buffer status _after_ we do the check, so
>>> looks such check isn't necessary.
>>>
>>
>> Consider the case where we get a callback interrupt, and before we
>> complete the processing for it, user terminates the request. In this
>> scenario,  quote_callback_handler() will consider the request is
>> still valid and no do the memory cleanup. To handle this case,
>> we need to check the status in invalidate_quote_request() and do
>> the cleanup if required.
>>
>> /* Handles early termination of GetQuote requests */
>> void invalidate_quote_request(struct quote_entry *entry)
>> {
>>           struct tdx_quote_hdr *quote_hdr;
>>
>>           /*
>>            * For early termination, if the request is not yet
>>            * processed by VMM (GET_QUOTE_IN_FLIGHT), the VMM
>>            * still owns the shared buffer, so mark the request
>>            * invalid to let quote_callback_handler() handle the
>>            * memory cleanup function. If the request is already
>>            * processed, then do the cleanup and return.
>>            */
>>
>>           mutex_lock(&quote_lock);
>>           quote_hdr = (struct tdx_quote_hdr *)entry->buf->vmaddr;
>>           if (quote_hdr->status == GET_QUOTE_IN_FLIGHT) {
> 
> What prevents VMM from updating quote_hdr->status from IN_FLIGHT to DONE _after_
> this check?

If IN_FLIGHT -> DONE happens at this point, you will get a separate
callback interrupt for it after this change. Since we hold quote_lock
here, processing of that callback work will wait till we make the
request invalid. For invalid requests, quote_callback_handler() will
handle the cleanup.

This logic is mainly for the case where quote_callback_handler() in
process and user terminates the request. In this case,
quote_callback_handler() will not do the cleanup, and we should add
logic here to handle it.

> 
> If you want to add such check, you should check against GET_QUOTE_DONE, but not
> IN_FLIGHT.  Only after status is DONE,  VMM will not update the buffer.  Perhaps
> something like below:
> 
> 	mutex_lock(&quote_lock);
> 	/* Skip invalidate the buffer if VMM has done with the buffer */
> 	if (quote_hdr->status == GET_QUOTE_DONE) {
> 		mutex_unlock(&quote_lock);
> 		return 0;
> 	}

There is no DONE status. Status can be either one of the following values.

GET_QUOTE_ERROR
GET_QUOTE_SUCCESS
GET_QUOTE_SERVICE_UNAVAILABLE
GET_QUOTE_IN_FLIGHT

> 
> And in the IOCTL, you can perhaps to choose to return 0, instead of -EINTR in
> this case, as the Quote has been finished already.

I think your logic above is to return 0 for the completed request even
if it is interrupted. But IMO, since the user already interrupted the
request, then I think it is more appropriate to just return -EINTR for
this case.

> 
> But I am not sure whether this is necessary.  The worst case is one finished
> Quote is wasted I guess.

> 
>>                   entry->valid = false;
>>                   mutex_unlock(&quote_lock);
>>                   return;
>>           }
>>           _del_quote_entry(entry);
>>           mutex_unlock(&quote_lock);
>> }
>>
>>
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Kai Huang]

On Tue, 2022-05-17 at 07:54 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > +struct tdx_report_req {
> > > +	union {
> > > +		__u8 reportdata[TDX_REPORTDATA_LEN];
> > > +		__u8 tdreport[TDX_REPORT_LEN];
> > > +	};
> > > +};
> > 
> > As a userspace ABI, one concern is this doesn't provide any space for future
> > extension.  But probably it's OK since I don't see any possible additional
> > input
> > for now.  And although TDREPORT may have additional information in future
> > generation of TDX but the spec says the size is 1024 so perhaps this won't
> > change even in the future.
> > 
> > Anyway will leave to others.
> 
> IMO, if the spec changes in future we can revisit it.

I don't think the problem is how to revisit _this_ ABI.  The problem is, once it
is introduced, you cannot break the ABI for the compatibility of supporting the
userspace software written for old platforms.  So basically you cannot just
increase the TDX_REPORT_LEN to a larger value.  This means if we have a larger
than 1024B TDREPORT in future, the old userspace TD attestation software which
uses this ABI will not work anymore on the new platforms.

If we need to make sure this ABI work for _ANY_ TDX platforms, I think we either
need to make sure TDREPORT will always be 1024B for _ANY_ TDX platforms, or we
need to have a flexible ABI which doesn't assume TDREPORT size.

For instance, we might need another IOCTL (or other interfaces such as /sysfs)
to query the TDREPORT size, and make this IOCTL like below:

	struct tdx_report_req {
		__u8 reportdata[TDX_REPORTDATA_LEN];
		__u8 reserved[...];
		__u8 tdreport[0];
	};

The actual TDREPORT buffer size is allocated by userspace after it queries the
TDREPORT size.

-- 
Thanks,
-Kai

Re: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 5/22/22 7:52 PM, Kai Huang wrote:
> On Tue, 2022-05-17 at 07:54 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>> +struct tdx_report_req {
>>>> +	union {
>>>> +		__u8 reportdata[TDX_REPORTDATA_LEN];
>>>> +		__u8 tdreport[TDX_REPORT_LEN];
>>>> +	};
>>>> +};
>>>
>>> As a userspace ABI, one concern is this doesn't provide any space for future
>>> extension.  But probably it's OK since I don't see any possible additional
>>> input
>>> for now.  And although TDREPORT may have additional information in future
>>> generation of TDX but the spec says the size is 1024 so perhaps this won't
>>> change even in the future.
>>>
>>> Anyway will leave to others.
>>
>> IMO, if the spec changes in future we can revisit it.
> 
> I don't think the problem is how to revisit _this_ ABI.  The problem is, once it
> is introduced, you cannot break the ABI for the compatibility of supporting the
> userspace software written for old platforms.  So basically you cannot just
> increase the TDX_REPORT_LEN to a larger value.  This means if we have a larger
> than 1024B TDREPORT in future, the old userspace TD attestation software which
> uses this ABI will not work anymore on the new platforms.
> 
> If we need to make sure this ABI work for _ANY_ TDX platforms, I think we either
> need to make sure TDREPORT will always be 1024B for _ANY_ TDX platforms, or we
> need to have a flexible ABI which doesn't assume TDREPORT size.
> 
> For instance, we might need another IOCTL (or other interfaces such as /sysfs)
> to query the TDREPORT size, and make this IOCTL like below:
> 
> 	struct tdx_report_req {
> 		__u8 reportdata[TDX_REPORTDATA_LEN];
> 		__u8 reserved[...];
> 		__u8 tdreport[0];
> 	};
> 
> The actual TDREPORT buffer size is allocated by userspace after it queries the
> TDREPORT size.

I don't want to over design it just based on the assumption that length
will change in the future. I don't see any statement in spec supporting
the possibility of length changes. IMO, since the possibility is very
small, we don't need to overthink about it.

@maintainers, please let me know if you think otherwise.
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer