[PATCH v4 0/3] Add TDX Guest Attestation support

[PATCH v4 0/3] 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).

Dependencies:
--------------

This feature has dependency on TDX guest core patch set series.

https://lore.kernel.org/all/20220218161718.67148-1-kirill.shutemov@linux.intel.com/T/

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 (3):
  x86/tdx: Add TDX Guest attestation interface driver
  x86/tdx: Add TDX Guest event notify interrupt support
  x86/tdx: Add Quote generation support

 arch/x86/coco/tdx/Makefile         |   2 +-
 arch/x86/coco/tdx/attest.c         | 305 +++++++++++++++++++++++++++++
 arch/x86/coco/tdx/tdx.c            | 159 +++++++++++++++
 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         |   8 +
 arch/x86/include/uapi/asm/tdx.h    |  59 ++++++
 arch/x86/kernel/irq.c              |   7 +
 9 files changed, 552 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/coco/tdx/attest.c
 create mode 100644 arch/x86/include/uapi/asm/tdx.h

-- 
2.25.1

[PATCH v4 1/3] 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 making any secure communication.

One usage example is, when a TD guest uses encrypted drive and the
decryption keys required to access the drive are stored in a secure
3rd party keyserver, TD guest can use the quote generated via the
attestation process to prove its trustworthiness with keyserver and
get access to the storage keys.

General steps involved in attestation process are,

  1. TD guest generates the TDREPORT that contains version information
     about the Intel TDX module, measurement of the TD, along with a
     TD-specified nonce.
  2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
     which is used by the host to generate a quote via quoting
     enclave (QE).
  3. Quote generation completion notification is sent to TD OS via
     callback interrupt vector configured by TD using
     SetupEventNotifyInterrupt hypercall.
  4. After receiving the generated TDQUOTE, a remote verifier can be
     used to verify the quote and confirm the trustworthiness of the
     TD.
     
More details on above mentioned steps can be found in TDX Guest-Host
Communication Interface (GHCI) for Intel TDX 1.0, section titled
"TD attestation".

To allow the attestation agent (user application) to implement this
feature, add an IOCTL interface to get TDREPORT and TDQUOTE from the
user space. Since attestation agent can also use methods like vosck or
TCP/IP to get the TDQUOTE, adding an IOCTL interface for it is an
optional feature. So to simplify the driver, first add support for
TDX_CMD_GET_TDREPORT IOCTL. Support for TDQUOTE IOCTL will be added by
follow-on patches.

TDREPORT can be generated by sending a TDCALL with leaf ID as 0x04.
More details about the TDREPORT TDCALL can be found in Intel Trust
Domain Extensions (Intel TDX) Module specification, section titled
"TDG.MR.REPORT Leaf". Add a wrapper function (tdx_mcall_tdreport())
to get the TDREPORT from the TDX Module. This API will be used by the
interface driver to request for TDREPORT.

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.

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      | 191 ++++++++++++++++++++++++++++++++
 arch/x86/coco/tdx/tdx.c         |  45 ++++++++
 arch/x86/include/asm/tdx.h      |   2 +
 arch/x86/include/uapi/asm/tdx.h |  23 ++++
 5 files changed, 262 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..b776e81f6c20
--- /dev/null
+++ b/arch/x86/coco/tdx/attest.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * attest.c - TDX guest attestation interface driver.
+ *
+ * Implements user interface to trigger attestation process and
+ * read the TD Quote result.
+ *
+ * Copyright (C) 2022 Intel Corporation
+ *
+ */
+
+#define pr_fmt(fmt) "x86/tdx: attest: " fmt
+
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/uaccess.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/set_memory.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/jiffies.h>
+#include <linux/io.h>
+#include <asm/apic.h>
+#include <asm/tdx.h>
+#include <asm/irq_vectors.h>
+#include <uapi/asm/tdx.h>
+
+#define DRIVER_NAME "tdx-attest"
+
+static struct platform_device *pdev;
+static struct miscdevice miscdev;
+
+static long tdx_get_tdreport(void __user *argp)
+{
+	void *report_buf = NULL, *tdreport_buf = NULL;
+	long ret = 0, err;
+
+	/* Allocate space for report data */
+	report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
+	if (!report_buf)
+		return -ENOMEM;
+
+	/*
+	 * Allocate space for TDREPORT buffer (1024-byte aligned).
+	 * Full page alignment is more than enough.
+	 */
+	tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!tdreport_buf) {
+		ret = -ENOMEM;
+		goto tdreport_failed;
+	}
+
+	/* Copy report data to kernel buffer */
+	if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) {
+		ret = -EFAULT;
+		goto tdreport_failed;
+	}
+
+	/* Generate TDREPORT using report data in report_buf */
+	err = tdx_mcall_tdreport(tdreport_buf, report_buf);
+	if (err) {
+		/* If failed, pass TDCALL error code back to user */
+		ret = put_user(err, (long __user *)argp);
+		ret = -EIO;
+		goto tdreport_failed;
+	}
+
+	/* Copy TDREPORT data back to user buffer */
+	if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN))
+		ret = -EFAULT;
+
+tdreport_failed:
+	kfree(report_buf);
+	if (tdreport_buf)
+		free_pages((unsigned long)tdreport_buf, 0);
+
+	return ret;
+}
+
+static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
+			     unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	long ret = 0;
+
+	switch (cmd) {
+	case TDX_CMD_GET_TDREPORT:
+		ret = tdx_get_tdreport(argp);
+		break;
+	default:
+		pr_err("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 tdx_attest_probe(struct platform_device *attest_pdev)
+{
+	struct device *dev = &attest_pdev->dev;
+	long ret = 0;
+
+	/* Only single device is allowed */
+	if (pdev)
+		return -EBUSY;
+
+	pdev = attest_pdev;
+
+	miscdev.name = DRIVER_NAME;
+	miscdev.minor = MISC_DYNAMIC_MINOR;
+	miscdev.fops = &tdx_attest_fops;
+	miscdev.parent = dev;
+
+	ret = misc_register(&miscdev);
+	if (ret) {
+		pr_err("misc device registration failed\n");
+		goto failed;
+	}
+
+	pr_debug("module initialization success\n");
+
+	return 0;
+
+failed:
+	misc_deregister(&miscdev);
+
+	pr_debug("module initialization failed\n");
+
+	return ret;
+}
+
+static int tdx_attest_remove(struct platform_device *attest_pdev)
+{
+	misc_deregister(&miscdev);
+	pr_debug("module is successfully removed\n");
+	return 0;
+}
+
+static struct platform_driver tdx_attest_driver = {
+	.probe		= tdx_attest_probe,
+	.remove		= tdx_attest_remove,
+	.driver		= {
+		.name	= DRIVER_NAME,
+	},
+};
+
+static int __init tdx_attest_init(void)
+{
+	int ret;
+
+	/* Make sure we are in a valid TDX platform */
+	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return -EIO;
+
+	ret = platform_driver_register(&tdx_attest_driver);
+	if (ret) {
+		pr_err("failed to register driver, err=%d\n", ret);
+		return ret;
+	}
+
+	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
+	if (IS_ERR(pdev)) {
+		ret = PTR_ERR(pdev);
+		pr_err("failed to allocate device, err=%d\n", ret);
+		platform_driver_unregister(&tdx_attest_driver);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit tdx_attest_exit(void)
+{
+	platform_device_unregister(pdev);
+	platform_driver_unregister(&tdx_attest_driver);
+}
+
+module_init(tdx_attest_init);
+module_exit(tdx_attest_exit);
+
+MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
+MODULE_DESCRIPTION("TDX attestation driver");
+MODULE_LICENSE("GPL");
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 03deb4d6920d..2a79ca92a52d 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -11,10 +11,12 @@
 #include <asm/insn.h>
 #include <asm/insn-eval.h>
 #include <asm/pgtable.h>
+#include <asm/io.h>
 
 /* TDX module Call Leaf IDs */
 #define TDX_GET_INFO			1
 #define TDX_GET_VEINFO			3
+#define TDX_GET_REPORT			4
 #define TDX_ACCEPT_PAGE			6
 
 /* TDX hypercall Leaf IDs */
@@ -34,6 +36,10 @@
 #define VE_GET_PORT_NUM(e)	((e) >> 16)
 #define VE_IS_IO_STRING(e)	((e) & BIT(4))
 
+/* TDX Module call error codes */
+#define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
+#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
+
 /*
  * Wrapper for standard use of __tdx_hypercall with no output aside from
  * return code.
@@ -98,6 +104,45 @@ 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_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
+ *
+ * @data        : Address of 1024B aligned data to store
+ *                TDREPORT_STRUCT.
+ * @reportdata  : Address of 64B aligned report data
+ *
+ * return 0 on success or failure error number.
+ */
+long tdx_mcall_tdreport(void *data, void *reportdata)
+{
+	u64 ret;
+
+	/*
+	 * Check for a valid TDX guest to ensure this API is only
+	 * used by TDX guest platform. Also make sure "data" and
+	 * "reportdata" pointers are valid.
+	 */
+	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return -EINVAL;
+
+	/*
+	 * Pass the physical address of user generated report data
+	 * and the physical address of output buffer to the TDX module
+	 * to generate the TD report. Generated data contains
+	 * measurements/configuration data of the TD guest. More info
+	 * about ABI can be found in TDX 1.0 Module specification, sec
+	 * titled "TDG.MR.REPORT".
+	 */
+	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
+				virt_to_phys(reportdata), 0, 0, NULL);
+
+	if (ret)
+		return TDCALL_RETURN_CODE(ret);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
+
 static u64 get_cc_mask(void)
 {
 	struct tdx_module_output out;
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 020c81a7c729..a151f69dd6ef 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -67,6 +67,8 @@ void tdx_safe_halt(void);
 
 bool tdx_early_handle_ve(struct pt_regs *regs);
 
+long tdx_mcall_tdreport(void *data, void *reportdata);
+
 #else
 
 static inline void tdx_early_init(void) { };
diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
new file mode 100644
index 000000000000..c21f9d6fe88b
--- /dev/null
+++ b/arch/x86/include/uapi/asm/tdx.h
@@ -0,0 +1,23 @@
+/* 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>
+
+/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
+#define TDX_REPORT_DATA_LEN		64
+
+/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
+#define TDX_TDREPORT_LEN		1024
+
+/*
+ * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
+ * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
+ * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
+ * TDREPORT data is copied to the user buffer. On failure, TDCALL error
+ * code is copied back to the user buffer.
+ */
+#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
+
+#endif /* _UAPI_ASM_X86_TDX_H */
-- 
2.25.1

[PATCH v4 2/3] 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>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/tdx/tdx.c            | 77 ++++++++++++++++++++++++++++++
 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, 101 insertions(+), 1 deletion(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 2a79ca92a52d..d0c62b94a1f6 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -12,6 +12,10 @@
 #include <asm/insn-eval.h>
 #include <asm/pgtable.h>
 #include <asm/io.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
@@ -21,6 +25,7 @@
 
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
+#define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
 
 /* MMIO direction */
 #define EPT_READ	0
@@ -40,6 +45,28 @@
 #define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
 #define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
 
+/*
+ * 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.
@@ -104,6 +131,21 @@ 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_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
  *
@@ -143,6 +185,35 @@ long tdx_mcall_tdreport(void *data, void *reportdata)
 }
 EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
 
+/*
+ * 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)
+{
+	u64 ret;
+
+	/* 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>".
+	 */
+	ret = _tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, vector, 0, 0, 0);
+
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static u64 get_cc_mask(void)
 {
 	struct tdx_module_output out;
@@ -733,5 +804,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..331d343e1d46 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
 
+#ifdef 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 a151f69dd6ef..89ed09809c13 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -69,6 +69,10 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
 
 long tdx_mcall_tdreport(void *data, void *reportdata);
 
+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 v4 3/3] x86/tdx: Add Quote generation support

[Kuppuswamy Sathyanarayanan]

In TDX guest, the second stage in attestation process is quote
generation and signing. GetQuote hypercall can be used by the TD guest
to request VMM facilitate the quote generation via a Quoting Enclave
(QE). 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 quote is generated. So VMM uses callback interrupt vector
configured by SetupEventNotifyInterrupt hypercall to notify the guest
about quote generation completion or failure. Upon receiving the
completion notification, status can be found in the Quote data header.

Add tdx_hcall_get_quote() helper function to implement the GetQuote
hypercall and add TDX_CMD_GEN_QUOTE IOCTL support to allow the user
agent request for quote generation.

When a user agent requests for quote generation, it is expected that
the user agent knows about the Quoting Enclave response time, and sets
a valid timeout value for the quote generation completion. Timeout
support is added to make sure the kernel does not wait for the
quote completion indefinitely.

Although GHCI specification does not restrict parallel GetQuote
requests, since quote generation is not in performance critical path
and the frequency of attestation requests are expected to be low, only
support serialized quote generation requests. Serialization support is
added via a mutex lock (attest_lock). Parallel quote request support
can be added once demand arises.

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      | 118 +++++++++++++++++++++++++++++++-
 arch/x86/coco/tdx/tdx.c         |  37 ++++++++++
 arch/x86/include/asm/tdx.h      |   2 +
 arch/x86/include/uapi/asm/tdx.h |  36 ++++++++++
 4 files changed, 191 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
index b776e81f6c20..d485163d3222 100644
--- a/arch/x86/coco/tdx/attest.c
+++ b/arch/x86/coco/tdx/attest.c
@@ -32,6 +32,11 @@
 static struct platform_device *pdev;
 static struct miscdevice miscdev;
 
+/* Completion object to track GetQuote completion status */
+static DECLARE_COMPLETION(req_compl);
+/* Mutex to serialize GetQuote requests */
+static DEFINE_MUTEX(quote_lock);
+
 static long tdx_get_tdreport(void __user *argp)
 {
 	void *report_buf = NULL, *tdreport_buf = NULL;
@@ -79,6 +84,95 @@ static long tdx_get_tdreport(void __user *argp)
 	return ret;
 }
 
+static long tdx_get_tdquote(void __user *argp)
+{
+	struct tdx_quote_hdr *quote_hdr;
+	struct tdx_quote_req quote_req;
+	void *quote_buf = NULL;
+	dma_addr_t handle;
+	long ret = 0, err;
+	u64 quote_buf_len;
+
+	mutex_lock(&quote_lock);
+
+	reinit_completion(&req_compl);
+
+	/* Copy Quote request struct from user buffer */
+	if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
+		return -EFAULT;
+
+	/* Make sure the length & timeout is valid */
+	if (quote_req.len <= 0 || quote_req.timeout <= 0)
+		return -EINVAL;
+
+	/* Align with page size to meet 4K alignment */
+	quote_buf_len = PAGE_ALIGN(quote_req.len);
+
+	/*
+	 * Allocate DMA buffer to get TDQUOTE data from the VMM.
+	 * dma_alloc_coherent() API internally marks allocated
+	 * memory as shared with VMM. So explicit shared mapping is
+	 * not required.
+	 */
+	quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
+					GFP_KERNEL | __GFP_ZERO);
+	if (!quote_buf) {
+		ret = -ENOMEM;
+		goto quote_failed;
+	}
+
+	/* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
+	if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
+		ret = -EFAULT;
+		goto quote_failed;
+	}
+
+	/* Submit GetQuote Request */
+	err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
+	if (err) {
+		/* if failed, copy hypercall error code to user buffer */
+		ret = put_user(err, (long __user *)argp);
+		ret = -EIO;
+		goto quote_failed;
+	}
+
+	/* Wait for attestation completion */
+	ret = wait_for_completion_interruptible_timeout(
+			&req_compl,
+			msecs_to_jiffies(quote_req.timeout));
+	if (ret <= 0) {
+		ret = -EIO;
+		goto quote_failed;
+	}
+
+	/* Copy generated Quote data back to user buffer */
+	if (copy_to_user((void __user *)quote_req.buf, quote_buf, quote_buf_len)) {
+		ret = -EFAULT;
+		goto quote_failed;
+	}
+
+	quote_hdr = (struct tdx_quote_hdr *)quote_buf;
+
+	/* Make sure quote generation is successful */
+	if (!quote_hdr->status)
+		ret = 0;
+	else
+		ret = -EIO;
+
+quote_failed:
+	if (quote_buf)
+		dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);
+
+	mutex_unlock(&quote_lock);
+
+	return ret;
+}
+
+static void attestation_callback_handler(void)
+{
+	complete(&req_compl);
+}
+
 static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
 			     unsigned long arg)
 {
@@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
 	case TDX_CMD_GET_TDREPORT:
 		ret = tdx_get_tdreport(argp);
 		break;
+	case TDX_CMD_GEN_QUOTE:
+		ret = tdx_get_tdquote(argp);
+		break;
 	default:
 		pr_err("cmd %d not supported\n", cmd);
 		break;
@@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
 	.llseek		= no_llseek,
 };
 
+/* Helper function to cleanup attestation related allocations */
+static void _tdx_attest_remove(void)
+{
+	misc_deregister(&miscdev);
+
+	tdx_remove_ev_notify_handler();
+}
+
 static int tdx_attest_probe(struct platform_device *attest_pdev)
 {
 	struct device *dev = &attest_pdev->dev;
@@ -114,6 +219,15 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
 
 	pdev = attest_pdev;
 
+	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+	if (ret) {
+		pr_err("dma set coherent mask failed\n");
+		goto failed;
+	}
+
+	/* 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;
@@ -130,7 +244,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
 	return 0;
 
 failed:
-	misc_deregister(&miscdev);
+	_tdx_attest_remove();
 
 	pr_debug("module initialization failed\n");
 
@@ -139,7 +253,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
 
 static int tdx_attest_remove(struct platform_device *attest_pdev)
 {
-	misc_deregister(&miscdev);
+	_tdx_attest_remove();
 	pr_debug("module is successfully removed\n");
 	return 0;
 }
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index d0c62b94a1f6..cba22a8d4084 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -25,6 +25,7 @@
 
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
+#define TDVMCALL_GET_QUOTE		0x10002
 #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
 
 /* MMIO direction */
@@ -214,6 +215,42 @@ static long tdx_hcall_set_notify_intr(u8 vector)
 	return 0;
 }
 
+/*
+ * tdx_hcall_get_quote() - Request for TDQUOTE using TDREPORT.
+ *
+ * @data        : Address of 4KB aligned GPA memory which contains
+ *                TDREPORT_STRUCT.
+ * @len		: Length of the GPA in bytes.
+ *
+ * return 0 on success or failure error number.
+ */
+long tdx_hcall_get_quote(void *data, u64 len)
+{
+	u64 ret;
+
+	/*
+	 * Use confidential guest TDX check to ensure this API is only
+	 * used by TDX guest platforms.
+	 */
+	if (!data || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return -EINVAL;
+
+	/*
+	 * Pass the physical address of tdreport data to the VMM
+	 * and trigger the TDQUOTE 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>".
+	 */
+	ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
+			     len, 0, 0);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static u64 get_cc_mask(void)
 {
 	struct tdx_module_output out;
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 89ed09809c13..90c2a5f6c40c 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));
 
 void tdx_remove_ev_notify_handler(void);
 
+long tdx_hcall_get_quote(void *data, u64 len);
+
 #else
 
 static inline void tdx_early_init(void) { };
diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
index c21f9d6fe88b..69259b7841a9 100644
--- a/arch/x86/include/uapi/asm/tdx.h
+++ b/arch/x86/include/uapi/asm/tdx.h
@@ -20,4 +20,40 @@
  */
 #define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
 
+/*
+ * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
+ * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
+ * buffer of quote size. Once IOCTL is successful quote data is copied back to
+ * the user buffer. On failure, TDCALL error code is copied back to the user
+ * buffer.
+ */
+#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
+
+struct tdx_quote_req {
+	/* Buffer address to store Quote data */
+	__u64 buf;
+	/* Length of the Quote buffer */
+	__u64 len;
+	/* Quote generation timeout value in ms */
+	__u32 timeout;
+};
+
+/*
+ * 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;
+};
+
 #endif /* _UAPI_ASM_X86_TDX_H */
-- 
2.25.1

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Kai Huang]

On Fri, 2022-04-22 at 16:34 -0700, Kuppuswamy Sathyanarayanan wrote:
> In TDX guest, attestation is used to verify the trustworthiness of a TD
> to other entities before making any secure communication.

Before provisioning secrets to the TD.

> 
> One usage example is, when a TD guest uses encrypted drive and the
> decryption keys required to access the drive are stored in a secure
> 3rd party keyserver, TD guest can use the quote generated via the
> attestation process to prove its trustworthiness with keyserver and
> get access to the storage keys.

"The key server can use attestation to verify TD's trustworthiness and release
the decryption keys to the TD".

It is the key server who starts the attestation request, but not the TD.

> 
> General steps involved in attestation process are,
> 
>   1. TD guest generates the TDREPORT that contains version information
>      about the Intel TDX module, measurement of the TD, along with a
>      TD-specified nonce.
>   2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
>      which is used by the host to generate a quote via quoting
>      enclave (QE).
>   3. Quote generation completion notification is sent to TD OS via
>      callback interrupt vector configured by TD using
>      SetupEventNotifyInterrupt hypercall.
>   4. After receiving the generated TDQUOTE, a remote verifier can be
>      used to verify the quote and confirm the trustworthiness of the
>      TD.

Let's separate TDREPORT generation and Quote generation, and get rid of details
of how to get Quote part for now. Detail of GetQuote belongs to the patch which
implements it.  Here I think we should focus on explaining why we need such a
basic driver to allow userspace to get the TDREPORT.

Below is for your reference:

"
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 to contain the TD-specific informatin (such as TD
measurements), platform information such as the security version of the platform
and the TDX module and the MAC to protect the integrity of the TDREPORT. 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.

TDREPORT can only be verified locally as the MAC key is bound to the platform. 
TDX attestation leverages Intel SGX Quote Enclave (QE) 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
attestation process is to send the TDREPORT to QE to generate the Quote.

How is the QE, or Quote Generation Service (QGS) in general, implemented and
deployed is implementation specific.  As a result, how to send the TDREPORT to
QE/QGS also depends on QE/QGS implementation and the deployment.  TDX doesn't
support SGX inside a TD, so the QE/QGS can be deployed in the host, or inside a
dedicated legacy VM.

A typical implementation is TD userspace attestation software gets the TDREPORT
from TD kernel, sends it to QE/QGS, and QE/QGS returns the Quote.  The data and
data format that TD userspace attestation software sends to the QE/QGS is also
implementation specific, but not necessarily just the raw TDREPORT.  TD
attestation software can use any available communication channel to talk to
QE/QGS, such as using vsock and tcp/ip.

To support the case that those communication channels are not directly available
to the TD, TDX also defines TDVMCALLs to allow TD to use TDVMCALL to ask VMM to
help with sending the TDREPORT and receiving the Quote.  This support is
documented in the GHCI spec "5.4 TD attestation".

Implement a basic attestation driver to allow TD userspace to get the TDREPORT
so the attestation software can send it to the QE to generate a Quote for remote
verification.
"


>      
> More details on above mentioned steps can be found in TDX Guest-Host
> Communication Interface (GHCI) for Intel TDX 1.0, section titled
> "TD attestation".
> 
> To allow the attestation agent (user application) to implement this
> feature, add an IOCTL interface to get TDREPORT and TDQUOTE from the
> user space. Since attestation agent can also use methods like vosck or
> TCP/IP to get the TDQUOTE, adding an IOCTL interface for it is an
> optional feature. So to simplify the driver, first add support for
> TDX_CMD_GET_TDREPORT IOCTL. Support for TDQUOTE IOCTL will be added by
> follow-on patches.
> 
> TDREPORT can be generated by sending a TDCALL with leaf ID as 0x04.
> More details about the TDREPORT TDCALL can be found in Intel Trust
> Domain Extensions (Intel TDX) Module specification, section titled
> "TDG.MR.REPORT Leaf". Add a wrapper function (tdx_mcall_tdreport())
> to get the TDREPORT from the TDX Module. This API will be used by the
> interface driver to request for TDREPORT.
> 
> 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.
> 
> 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      | 191 ++++++++++++++++++++++++++++++++
>  arch/x86/coco/tdx/tdx.c         |  45 ++++++++
>  arch/x86/include/asm/tdx.h      |   2 +
>  arch/x86/include/uapi/asm/tdx.h |  23 ++++
>  5 files changed, 262 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..b776e81f6c20
> --- /dev/null
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process and
> + * read the TD Quote result.

Read TDREPORT.  You can extend it in later patch if you want to call out
TDREPORT, Quote.  But I don't think it's even necessary.  Perhaps just say "TDX
attestation support" in general is enough.

> + *
> + * Copyright (C) 2022 Intel Corporation
> + *
> + */
> +
> +#define pr_fmt(fmt) "x86/tdx: attest: " fmt
> +
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +#include <linux/uaccess.h>
> +#include <linux/fs.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/set_memory.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +#include <linux/jiffies.h>
> +#include <linux/io.h>
> +#include <asm/apic.h>
> +#include <asm/tdx.h>
> +#include <asm/irq_vectors.h>
> +#include <uapi/asm/tdx.h>
> +
> +#define DRIVER_NAME "tdx-attest"
> +
> +static struct platform_device *pdev;
> +static struct miscdevice miscdev;
> +
> +static long tdx_get_tdreport(void __user *argp)
> +{
> +	void *report_buf = NULL, *tdreport_buf = NULL;
> +	long ret = 0, err;
> +
> +	/* Allocate space for report data */
> +	report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
> +	if (!report_buf)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Allocate space for TDREPORT buffer (1024-byte aligned).
> +	 * Full page alignment is more than enough.
> +	 */
> +	tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
> +	if (!tdreport_buf) {
> +		ret = -ENOMEM;
> +		goto tdreport_failed;
> +	}
> +
> +	/* Copy report data to kernel buffer */
> +	if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) {
> +		ret = -EFAULT;
> +		goto tdreport_failed;
> +	}
> +
> +	/* Generate TDREPORT using report data in report_buf */
> +	err = tdx_mcall_tdreport(tdreport_buf, report_buf);
> +	if (err) {
> +		/* If failed, pass TDCALL error code back to user */
> +		ret = put_user(err, (long __user *)argp);
> +		ret = -EIO;
> +		goto tdreport_failed;
> +	}

If you want support this, I guess it's better to explicitly use some data
structure so userspace software can be very clear about what does this IOCTL do:

	struct tdx_get_tdreport {
		union {
			/* Input: REPORTDATA for TDCALL[TDG.MR.REPORT] */
			__u8 reportdata[TDX_REPORT_DATA_LEN];
			/* Output when TDCALL[TDG.MR.REPORT] fails */
			__u64 tdcall_err;
		} buf;
	};

And you need to explain in the comment saying -EIO means TDCALL failed, and the
error code is returned to userspace.

But I am not sure whether this is necessary.  The spec says TDG.MR.REPORT can
return: TDX_OPERAND_BUSY, TDX_OPERAND_INVALID, TDX_SUCCESS.  

TDX_OPERAND_INVALID basically happens when buffer alignment doesn't meet, GPA is
wrong, etc, so this case is kernel bug.  I don't see there's a need to expose it
to userspace as userspace won't be able to do anything anyway.

The BUSY case is (if I understand correctly, I took a look at the SEAM module
code) basically there's another thread updating the TDMR (registers for runtime
measurement update).  Since it seems current kernel doesn't support
TDG.MR.RTMR.EXTEND, TDX_OPERAND_BUSY should not happen either.  Even considering
the support of TDG.MR.RTMR.EXTEND in the future, I think kernel should use some
mutex to serialize it with TDG.MR.REPORT? 

The bottom line is even we want to report BUSY to userspace, we choose to 
return -EBUSY to indicate this case.  So basically I don't see the value of
exposing TDCALL error to userspace.


> +
> +	/* Copy TDREPORT data back to user buffer */
> +	if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN))
> +		ret = -EFAULT;
> +
> +tdreport_failed:
> +	kfree(report_buf);
> +	if (tdreport_buf)
> +		free_pages((unsigned long)tdreport_buf, 0);
> +
> +	return ret;
> +}
> +
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	long ret = 0;
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_TDREPORT:
> +		ret = tdx_get_tdreport(argp);
> +		break;
> +	default:
> +		pr_err("cmd %d not supported\n", cmd);
> +		break;

Seems you are returning 0 in the default case.

> +	}
> +
> +	return ret;
> +}
> +
> +static const struct file_operations tdx_attest_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= tdx_attest_ioctl,
> +	.llseek		= no_llseek,
> +};
> +
> +static int tdx_attest_probe(struct platform_device *attest_pdev)
> +{
> +	struct device *dev = &attest_pdev->dev;
> +	long ret = 0;
> +
> +	/* Only single device is allowed */
> +	if (pdev)
> +		return -EBUSY;
> +
> +	pdev = attest_pdev;
> +
> +	miscdev.name = DRIVER_NAME;
> +	miscdev.minor = MISC_DYNAMIC_MINOR;
> +	miscdev.fops = &tdx_attest_fops;
> +	miscdev.parent = dev;
> +
> +	ret = misc_register(&miscdev);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		goto failed;
> +	}
> +
> +	pr_debug("module initialization success\n");
> +
> +	return 0;
> +
> +failed:
> +	misc_deregister(&miscdev);
> +
> +	pr_debug("module initialization failed\n");
> +
> +	return ret;
> +}
> +
> +static int tdx_attest_remove(struct platform_device *attest_pdev)
> +{
> +	misc_deregister(&miscdev);
> +	pr_debug("module is successfully removed\n");
> +	return 0;
> +}
> +
> +static struct platform_driver tdx_attest_driver = {
> +	.probe		= tdx_attest_probe,
> +	.remove		= tdx_attest_remove,
> +	.driver		= {
> +		.name	= DRIVER_NAME,
> +	},
> +};
> +
> +static int __init tdx_attest_init(void)
> +{
> +	int ret;
> +
> +	/* Make sure we are in a valid TDX platform */
> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EIO;
> +
> +	ret = platform_driver_register(&tdx_attest_driver);
> +	if (ret) {
> +		pr_err("failed to register driver, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		ret = PTR_ERR(pdev);
> +		pr_err("failed to allocate device, err=%d\n", ret);
> +		platform_driver_unregister(&tdx_attest_driver);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit tdx_attest_exit(void)
> +{
> +	platform_device_unregister(pdev);
> +	platform_driver_unregister(&tdx_attest_driver);
> +}
> +
> +module_init(tdx_attest_init);
> +module_exit(tdx_attest_exit);

Is there any particular reason to use platform driver and support it as a
module?

SGX driver uses misc_register() to register /dev/sgx_enclave during boot.  Looks
it would be simpler.  

> +
> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
> +MODULE_DESCRIPTION("TDX attestation driver");
> +MODULE_LICENSE("GPL");
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 03deb4d6920d..2a79ca92a52d 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -11,10 +11,12 @@
>  #include <asm/insn.h>
>  #include <asm/insn-eval.h>
>  #include <asm/pgtable.h>
> +#include <asm/io.h>
>  
>  /* TDX module Call Leaf IDs */
>  #define TDX_GET_INFO			1
>  #define TDX_GET_VEINFO			3
> +#define TDX_GET_REPORT			4
>  #define TDX_ACCEPT_PAGE			6
>  
>  /* TDX hypercall Leaf IDs */
> @@ -34,6 +36,10 @@
>  #define VE_GET_PORT_NUM(e)	((e) >> 16)
>  #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>  
> +/* TDX Module call error codes */
> +#define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
> +#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
> +
>  /*
>   * Wrapper for standard use of __tdx_hypercall with no output aside from
>   * return code.
> @@ -98,6 +104,45 @@ 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_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
> + *
> + * @data        : Address of 1024B aligned data to store
> + *                TDREPORT_STRUCT.
> + * @reportdata  : Address of 64B aligned report data
> + *
> + * return 0 on success or failure error number.
> + */
> +long tdx_mcall_tdreport(void *data, void *reportdata)

Change 'data' to be something more meaningful, i.e. tdreport?

> +{
> +	u64 ret;
> +
> +	/*
> +	 * Check for a valid TDX guest to ensure this API is only
> +	 * used by TDX guest platform. Also make sure "data" and
> +	 * "reportdata" pointers are valid.
> +	 */
> +	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EINVAL;

Other TDCALL wrappers such as tdx_get_ve_info() doesn't have
X86_FEATURE_TDX_GUEST check.  Why is it needed in this particular one?

> +
> +	/*
> +	 * Pass the physical address of user generated report data
> +	 * and the physical address of output buffer to the TDX module
> +	 * to generate the TD report. Generated data contains
> +	 * measurements/configuration data of the TD guest. More info
> +	 * about ABI can be found in TDX 1.0 Module specification, sec
> +	 * titled "TDG.MR.REPORT".
> +	 */
> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
> +				virt_to_phys(reportdata), 0, 0, NULL);
> +
> +	if (ret)
> +		return TDCALL_RETURN_CODE(ret);

If we don't expose TDCALL error to userspace, I don't think this is required?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);

If you use SGX similar way to use misc_register() at boot but don't support the
driver as module, then you don't have to export this symbol.

> +
>  static u64 get_cc_mask(void)
>  {
>  	struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 020c81a7c729..a151f69dd6ef 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -67,6 +67,8 @@ void tdx_safe_halt(void);
>  
>  bool tdx_early_handle_ve(struct pt_regs *regs);
>  
> +long tdx_mcall_tdreport(void *data, void *reportdata);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> new file mode 100644
> index 000000000000..c21f9d6fe88b
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -0,0 +1,23 @@
> +/* 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>
> +
> +/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
> +#define TDX_REPORT_DATA_LEN		64

I'd change to TDX_REPORTDATA_LEN to make it consistent with spec.

REPORT_DATA can be vague.

> +
> +/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
> +#define TDX_TDREPORT_LEN		1024
> +
> +/*
> + * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX

"TDREPORT data" -> "TDREPORT", or "TDREPORT_STRUCT".

> + * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
> + * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
> + * TDREPORT data is copied to the user buffer. On failure, TDCALL error
> + * code is copied back to the user buffer.

As I commented above, I am not convinced we need to copy TDCALL error code back
to userspace.  If we want to do that, we need to explicitly tell on what error
code (-EIO, i.e.), the TDCALL error code is copied back.

> + */
> +#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)

I personally think it's better to define a structure to represent the argument
used by this IOCTL:

	struct tdx_get_tdreport {
		__u8 reportdata[TDX_REPORTDATA_LEN];
	};

You might add some padding for  future expending too.

Then we can have a clearer comment explaining what the reportdata.  Below is
just for your reference.  You may come up with better words.

"User-defined 64-Byte REPORTDATA to be included into the TDREPORT. Typically it
can be some nonce provided by attestation software so the generated TDREPORT can
be uniquely verified."

> +
> +#endif /* _UAPI_ASM_X86_TDX_H */

Re: [PATCH v4 3/3] x86/tdx: Add Quote generation support

[Kai Huang]

On Fri, 2022-04-22 at 16:34 -0700, Kuppuswamy Sathyanarayanan wrote:
> In TDX guest, the second stage in attestation process is quote
> generation and signing. GetQuote hypercall can be used by the TD guest
> to request VMM facilitate the quote generation via a Quoting Enclave
> (QE). 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 quote is generated. So VMM uses callback interrupt vector
> configured by SetupEventNotifyInterrupt hypercall to notify the guest
> about quote generation completion or failure. Upon receiving the
> completion notification, status can be found in the Quote data header.
> 
> Add tdx_hcall_get_quote() helper function to implement the GetQuote
> hypercall and add TDX_CMD_GEN_QUOTE IOCTL support to allow the user
> agent request for quote generation.
> 
> When a user agent requests for quote generation, it is expected that
> the user agent knows about the Quoting Enclave response time, and sets
> a valid timeout value for the quote generation completion. Timeout
> support is added to make sure the kernel does not wait for the
> quote completion indefinitely.
> 
> Although GHCI specification does not restrict parallel GetQuote
> requests, since quote generation is not in performance critical path
> and the frequency of attestation requests are expected to be low, only
> support serialized quote generation requests. Serialization support is
> added via a mutex lock (attest_lock). Parallel quote request support
> can be added once demand arises.
> 
> 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      | 118 +++++++++++++++++++++++++++++++-
>  arch/x86/coco/tdx/tdx.c         |  37 ++++++++++
>  arch/x86/include/asm/tdx.h      |   2 +
>  arch/x86/include/uapi/asm/tdx.h |  36 ++++++++++
>  4 files changed, 191 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
> index b776e81f6c20..d485163d3222 100644
> --- a/arch/x86/coco/tdx/attest.c
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -32,6 +32,11 @@
>  static struct platform_device *pdev;
>  static struct miscdevice miscdev;
>  
> +/* Completion object to track GetQuote completion status */
> +static DECLARE_COMPLETION(req_compl);
> +/* Mutex to serialize GetQuote requests */
> +static DEFINE_MUTEX(quote_lock);
> +
>  static long tdx_get_tdreport(void __user *argp)
>  {
>  	void *report_buf = NULL, *tdreport_buf = NULL;
> @@ -79,6 +84,95 @@ static long tdx_get_tdreport(void __user *argp)
>  	return ret;
>  }
>  
> +static long tdx_get_tdquote(void __user *argp)
> +{
> +	struct tdx_quote_hdr *quote_hdr;
> +	struct tdx_quote_req quote_req;
> +	void *quote_buf = NULL;
> +	dma_addr_t handle;
> +	long ret = 0, err;
> +	u64 quote_buf_len;
> +
> +	mutex_lock(&quote_lock);
> +
> +	reinit_completion(&req_compl);
> +
> +	/* Copy Quote request struct from user buffer */
> +	if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
> +		return -EFAULT;
> +
> +	/* Make sure the length & timeout is valid */
> +	if (quote_req.len <= 0 || quote_req.timeout <= 0)
> +		return -EINVAL;
> +
> +	/* Align with page size to meet 4K alignment */
> +	quote_buf_len = PAGE_ALIGN(quote_req.len);
> +
> +	/*
> +	 * Allocate DMA buffer to get TDQUOTE data from the VMM.
> +	 * dma_alloc_coherent() API internally marks allocated
> +	 * memory as shared with VMM. So explicit shared mapping is
> +	 * not required.
> +	 */
> +	quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
> +					GFP_KERNEL | __GFP_ZERO);
> +	if (!quote_buf) {
> +		ret = -ENOMEM;
> +		goto quote_failed;
> +	}
> +
> +	/* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
> +	if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
> +		ret = -EFAULT;
> +		goto quote_failed;
> +	}

So if I read correctly, you are depending on userspace to prepare the
tdx_quote_hdr, right? 

If so, should the driver check the correctness of the hdr? For instance, whether
hdr.version == 1, etc?

> +
> +	/* Submit GetQuote Request */
> +	err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
> +	if (err) {
> +		/* if failed, copy hypercall error code to user buffer */
> +		ret = put_user(err, (long __user *)argp);
> +		ret = -EIO;
> +		goto quote_failed;
> +	}

Similar to getting TDREPORT, is there any particular case that needs to pass
TDVMCALL error code back to userspace?

> +
> +	/* Wait for attestation completion */
> +	ret = wait_for_completion_interruptible_timeout(
> +			&req_compl,
> +			msecs_to_jiffies(quote_req.timeout));
> +	if (ret <= 0) {
> +		ret = -EIO;
> +		goto quote_failed;
> +	}
> +
> +	/* Copy generated Quote data back to user buffer */
> +	if (copy_to_user((void __user *)quote_req.buf, quote_buf, quote_buf_len)) {
> +		ret = -EFAULT;
> +		goto quote_failed;
> +	}
> +
> +	quote_hdr = (struct tdx_quote_hdr *)quote_buf;
> +
> +	/* Make sure quote generation is successful */
> +	if (!quote_hdr->status)
> +		ret = 0;
> +	else
> +		ret = -EIO;
> +
> +quote_failed:
> +	if (quote_buf)
> +		dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);

Will dma_free_coherent() convert the shared buffer back to private (using
MapGPA)?

If so, since it's possible to timeout, if the buffer still have IN_FLIGHT flag
set (VMM is still using it), can we do it?

Isaku, what will happen if guest uses MapGPA to convert a buffer back to private
while it still has IN_FLIGHT set?

> +
> +	mutex_unlock(&quote_lock);
> +
> +	return ret;
> +}
> +
> +static void attestation_callback_handler(void)
> +{
> +	complete(&req_compl);
> +}
> +
>  static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>  			     unsigned long arg)
>  {
> @@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>  	case TDX_CMD_GET_TDREPORT:
>  		ret = tdx_get_tdreport(argp);
>  		break;
> +	case TDX_CMD_GEN_QUOTE:
> +		ret = tdx_get_tdquote(argp);
> +		break;
>  	default:
>  		pr_err("cmd %d not supported\n", cmd);
>  		break;
> @@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
>  	.llseek		= no_llseek,
>  };
>  
> +/* Helper function to cleanup attestation related allocations */
> +static void _tdx_attest_remove(void)
> +{
> +	misc_deregister(&miscdev);
> +
> +	tdx_remove_ev_notify_handler();
> +}
> +
>  static int tdx_attest_probe(struct platform_device *attest_pdev)
>  {
>  	struct device *dev = &attest_pdev->dev;
> @@ -114,6 +219,15 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>  
>  	pdev = attest_pdev;
>  
> +	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> +	if (ret) {
> +		pr_err("dma set coherent mask failed\n");
> +		goto failed;
> +	}
> +
> +	/* 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;
> @@ -130,7 +244,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>  	return 0;
>  
>  failed:
> -	misc_deregister(&miscdev);
> +	_tdx_attest_remove();
>  
>  	pr_debug("module initialization failed\n");
>  
> @@ -139,7 +253,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>  
>  static int tdx_attest_remove(struct platform_device *attest_pdev)
>  {
> -	misc_deregister(&miscdev);
> +	_tdx_attest_remove();
>  	pr_debug("module is successfully removed\n");
>  	return 0;
>  }
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index d0c62b94a1f6..cba22a8d4084 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -25,6 +25,7 @@
>  
>  /* TDX hypercall Leaf IDs */
>  #define TDVMCALL_MAP_GPA		0x10001
> +#define TDVMCALL_GET_QUOTE		0x10002
>  #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
>  
>  /* MMIO direction */
> @@ -214,6 +215,42 @@ static long tdx_hcall_set_notify_intr(u8 vector)
>  	return 0;
>  }
>  
> +/*
> + * tdx_hcall_get_quote() - Request for TDQUOTE using TDREPORT.
> + *
> + * @data        : Address of 4KB aligned GPA memory which contains
> + *                TDREPORT_STRUCT.
> + * @len		: Length of the GPA in bytes.
> + *
> + * return 0 on success or failure error number.
> + */
> +long tdx_hcall_get_quote(void *data, u64 len)

Rename data/len to something meaningful, i.e. quote_buf, quote_len ?

> +{
> +	u64 ret;
> +
> +	/*
> +	 * Use confidential guest TDX check to ensure this API is only
> +	 * used by TDX guest platforms.
> +	 */
> +	if (!data || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EINVAL;

The same comment to tdx_mcall_get_tdreport().  You can have a single check of
X86_FEATURE_TDX_GUEST during driver initialization and refuse to initialize the
driver if it's not.

> +
> +	/*
> +	 * Pass the physical address of tdreport data to the VMM
> +	 * and trigger the TDQUOTE generation. It is not a blocking

I see there is inconsistency regarding to how to spell TD Quote.  I have seen
TDQUOTE, TD QUOTE, quote, and Quote.  I guess we can have a unified way for
this.  How about: Quote, or TD Quote (when you want to highlight TD)?

> +	 * 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>".
> +	 */
> +	ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
> +			     len, 0, 0);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static u64 get_cc_mask(void)
>  {
>  	struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 89ed09809c13..90c2a5f6c40c 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));
>  
>  void tdx_remove_ev_notify_handler(void);
>  
> +long tdx_hcall_get_quote(void *data, u64 len);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> index c21f9d6fe88b..69259b7841a9 100644
> --- a/arch/x86/include/uapi/asm/tdx.h
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -20,4 +20,40 @@
>   */
>  #define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
>  
> +/*
> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User

Replace "TD QUOTE" to some consistent name.

> + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
> + * buffer of quote size. 
> 

This is not correct.  The data userspace put into the buffer is transparent to
driver.  It's between userspace attestation agent and QE/QGS.  In fact, Intel's
implementation has an additional header besides TDREPORT, and the whole data is
encoded in proto2 format.

> Once IOCTL is successful quote data is copied back to
> + * the user buffer. On failure, TDCALL error code is copied back to the user
> + * buffer.

The output data may be more than the Quote, the same as above.

> + */
> +#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
> +
> +struct tdx_quote_req {
> +	/* Buffer address to store Quote data */
> +	__u64 buf;
> +	/* Length of the Quote buffer */
> +	__u64 len;
> +	/* Quote generation timeout value in ms */
> +	__u32 timeout;
> +};
> +
> +/*
> + * 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;
> +};

Needs to be '__u64 data[0]'.  The first 8B data isn't header.



-- 
Thanks,
-Kai

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Kai,

Thanks for the review.

On 4/24/22 10:44 PM, Kai Huang wrote:
> On Fri, 2022-04-22 at 16:34 -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, attestation is used to verify the trustworthiness of a TD
>> to other entities before making any secure communication.
> 
> Before provisioning secrets to the TD.

Ok. Will change it.

> 
>>
>> One usage example is, when a TD guest uses encrypted drive and the
>> decryption keys required to access the drive are stored in a secure
>> 3rd party keyserver, TD guest can use the quote generated via the
>> attestation process to prove its trustworthiness with keyserver and
>> get access to the storage keys.
> 
> "The key server can use attestation to verify TD's trustworthiness and release
> the decryption keys to the TD".
> 
> It is the key server who starts the attestation request, but not the TD.
> 
>>
>> General steps involved in attestation process are,
>>
>>    1. TD guest generates the TDREPORT that contains version information
>>       about the Intel TDX module, measurement of the TD, along with a
>>       TD-specified nonce.
>>    2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
>>       which is used by the host to generate a quote via quoting
>>       enclave (QE).
>>    3. Quote generation completion notification is sent to TD OS via
>>       callback interrupt vector configured by TD using
>>       SetupEventNotifyInterrupt hypercall.
>>    4. After receiving the generated TDQUOTE, a remote verifier can be
>>       used to verify the quote and confirm the trustworthiness of the
>>       TD.
> 
> Let's separate TDREPORT generation and Quote generation, and get rid of details
> of how to get Quote part for now. Detail of GetQuote belongs to the patch which
> implements it.  Here I think we should focus on explaining why we need such a
> basic driver to allow userspace to get the TDREPORT.
> 
> Below is for your reference:
> 
> "
> 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 to contain the TD-specific informatin (such as TD
> measurements), platform information such as the security version of the platform
> and the TDX module and the MAC to protect the integrity of the TDREPORT. 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.
> 
> TDREPORT can only be verified locally as the MAC key is bound to the platform.
> TDX attestation leverages Intel SGX Quote Enclave (QE) 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
> attestation process is to send the TDREPORT to QE to generate the Quote.
> 
> How is the QE, or Quote Generation Service (QGS) in general, implemented and
> deployed is implementation specific.  As a result, how to send the TDREPORT to
> QE/QGS also depends on QE/QGS implementation and the deployment.  TDX doesn't
> support SGX inside a TD, so the QE/QGS can be deployed in the host, or inside a
> dedicated legacy VM.
> 
> A typical implementation is TD userspace attestation software gets the TDREPORT
> from TD kernel, sends it to QE/QGS, and QE/QGS returns the Quote.  The data and
> data format that TD userspace attestation software sends to the QE/QGS is also
> implementation specific, but not necessarily just the raw TDREPORT.  TD
> attestation software can use any available communication channel to talk to
> QE/QGS, such as using vsock and tcp/ip.
> 
> To support the case that those communication channels are not directly available
> to the TD, TDX also defines TDVMCALLs to allow TD to use TDVMCALL to ask VMM to
> help with sending the TDREPORT and receiving the Quote.  This support is
> documented in the GHCI spec "5.4 TD attestation".
> 
> Implement a basic attestation driver to allow TD userspace to get the TDREPORT
> so the attestation software can send it to the QE to generate a Quote for remote
> verification.
> "
> 

Thanks. I will use it with some addition about the driver mentioned
below.

> 
>>       
>> More details on above mentioned steps can be found in TDX Guest-Host
>> Communication Interface (GHCI) for Intel TDX 1.0, section titled
>> "TD attestation".
>>
>> To allow the attestation agent (user application) to implement this
>> feature, add an IOCTL interface to get TDREPORT and TDQUOTE from the
>> user space. Since attestation agent can also use methods like vosck or
>> TCP/IP to get the TDQUOTE, adding an IOCTL interface for it is an
>> optional feature. So to simplify the driver, first add support for
>> TDX_CMD_GET_TDREPORT IOCTL. Support for TDQUOTE IOCTL will be added by
>> follow-on patches.
>>
>> TDREPORT can be generated by sending a TDCALL with leaf ID as 0x04.
>> More details about the TDREPORT TDCALL can be found in Intel Trust
>> Domain Extensions (Intel TDX) Module specification, section titled
>> "TDG.MR.REPORT Leaf". Add a wrapper function (tdx_mcall_tdreport())
>> to get the TDREPORT from the TDX Module. This API will be used by the
>> interface driver to request for TDREPORT.
>>
>> 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.
>>
>> 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      | 191 ++++++++++++++++++++++++++++++++
>>   arch/x86/coco/tdx/tdx.c         |  45 ++++++++
>>   arch/x86/include/asm/tdx.h      |   2 +
>>   arch/x86/include/uapi/asm/tdx.h |  23 ++++
>>   5 files changed, 262 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..b776e81f6c20
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * attest.c - TDX guest attestation interface driver.
>> + *
>> + * Implements user interface to trigger attestation process and
>> + * read the TD Quote result.
> 
> Read TDREPORT.  You can extend it in later patch if you want to call out
> TDREPORT, Quote.  But I don't think it's even necessary.  Perhaps just say "TDX
> attestation support" in general is enough.

Ok. I will fix it.

> 
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) "x86/tdx: attest: " fmt
>> +
>> +#include <linux/module.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/fs.h>
>> +#include <linux/mm.h>
>> +#include <linux/slab.h>
>> +#include <linux/set_memory.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/io.h>
>> +#include <asm/apic.h>
>> +#include <asm/tdx.h>
>> +#include <asm/irq_vectors.h>
>> +#include <uapi/asm/tdx.h>
>> +
>> +#define DRIVER_NAME "tdx-attest"
>> +
>> +static struct platform_device *pdev;
>> +static struct miscdevice miscdev;
>> +
>> +static long tdx_get_tdreport(void __user *argp)
>> +{
>> +	void *report_buf = NULL, *tdreport_buf = NULL;
>> +	long ret = 0, err;
>> +
>> +	/* Allocate space for report data */
>> +	report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
>> +	if (!report_buf)
>> +		return -ENOMEM;
>> +
>> +	/*
>> +	 * Allocate space for TDREPORT buffer (1024-byte aligned).
>> +	 * Full page alignment is more than enough.
>> +	 */
>> +	tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
>> +	if (!tdreport_buf) {
>> +		ret = -ENOMEM;
>> +		goto tdreport_failed;
>> +	}
>> +
>> +	/* Copy report data to kernel buffer */
>> +	if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) {
>> +		ret = -EFAULT;
>> +		goto tdreport_failed;
>> +	}
>> +
>> +	/* Generate TDREPORT using report data in report_buf */
>> +	err = tdx_mcall_tdreport(tdreport_buf, report_buf);
>> +	if (err) {
>> +		/* If failed, pass TDCALL error code back to user */
>> +		ret = put_user(err, (long __user *)argp);
>> +		ret = -EIO;
>> +		goto tdreport_failed;
>> +	}
> 
> If you want support this, I guess it's better to explicitly use some data
> structure so userspace software can be very clear about what does this IOCTL do:
> 
> 	struct tdx_get_tdreport {
> 		union {
> 			/* Input: REPORTDATA for TDCALL[TDG.MR.REPORT] */
> 			__u8 reportdata[TDX_REPORT_DATA_LEN];
> 			/* Output when TDCALL[TDG.MR.REPORT] fails */
> 			__u64 tdcall_err;
> 		} buf;
> 	};
> 
> And you need to explain in the comment saying -EIO means TDCALL failed, and the
> error code is returned to userspace.
> 
> But I am not sure whether this is necessary.  The spec says TDG.MR.REPORT can
> return: TDX_OPERAND_BUSY, TDX_OPERAND_INVALID, TDX_SUCCESS.
> 
> TDX_OPERAND_INVALID basically happens when buffer alignment doesn't meet, GPA is
> wrong, etc, so this case is kernel bug.  I don't see there's a need to expose it
> to userspace as userspace won't be able to do anything anyway.
> 
> The BUSY case is (if I understand correctly, I took a look at the SEAM module
> code) basically there's another thread updating the TDMR (registers for runtime
> measurement update).  Since it seems current kernel doesn't support
> TDG.MR.RTMR.EXTEND, TDX_OPERAND_BUSY should not happen either.  Even considering
> the support of TDG.MR.RTMR.EXTEND in the future, I think kernel should use some
> mutex to serialize it with TDG.MR.REPORT?

Ok. I will keep this in mind when adding RTMR support in future.

> 
> The bottom line is even we want to report BUSY to userspace, we choose to
> return -EBUSY to indicate this case.  So basically I don't see the value of
> exposing TDCALL error to userspace.

I have mainly added it to get more debug info about the failure in user
app. But I agree with your point that this not necessary. I will remove 
this in next version.

> 
> 
>> +
>> +	/* Copy TDREPORT data back to user buffer */
>> +	if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN))
>> +		ret = -EFAULT;
>> +
>> +tdreport_failed:
>> +	kfree(report_buf);
>> +	if (tdreport_buf)
>> +		free_pages((unsigned long)tdreport_buf, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>> +			     unsigned long arg)
>> +{
>> +	void __user *argp = (void __user *)arg;
>> +	long ret = 0;
>> +
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_TDREPORT:
>> +		ret = tdx_get_tdreport(argp);
>> +		break;
>> +	default:
>> +		pr_err("cmd %d not supported\n", cmd);
>> +		break;
> 
> Seems you are returning 0 in the default case.

Good catch. I will fix it in next version.

> 
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations tdx_attest_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.unlocked_ioctl	= tdx_attest_ioctl,
>> +	.llseek		= no_llseek,
>> +};
>> +
>> +static int tdx_attest_probe(struct platform_device *attest_pdev)
>> +{
>> +	struct device *dev = &attest_pdev->dev;
>> +	long ret = 0;
>> +
>> +	/* Only single device is allowed */
>> +	if (pdev)
>> +		return -EBUSY;
>> +
>> +	pdev = attest_pdev;
>> +
>> +	miscdev.name = DRIVER_NAME;
>> +	miscdev.minor = MISC_DYNAMIC_MINOR;
>> +	miscdev.fops = &tdx_attest_fops;
>> +	miscdev.parent = dev;
>> +
>> +	ret = misc_register(&miscdev);
>> +	if (ret) {
>> +		pr_err("misc device registration failed\n");
>> +		goto failed;
>> +	}
>> +
>> +	pr_debug("module initialization success\n");
>> +
>> +	return 0;
>> +
>> +failed:
>> +	misc_deregister(&miscdev);
>> +
>> +	pr_debug("module initialization failed\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int tdx_attest_remove(struct platform_device *attest_pdev)
>> +{
>> +	misc_deregister(&miscdev);
>> +	pr_debug("module is successfully removed\n");
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver tdx_attest_driver = {
>> +	.probe		= tdx_attest_probe,
>> +	.remove		= tdx_attest_remove,
>> +	.driver		= {
>> +		.name	= DRIVER_NAME,
>> +	},
>> +};
>> +
>> +static int __init tdx_attest_init(void)
>> +{
>> +	int ret;
>> +
>> +	/* Make sure we are in a valid TDX platform */
>> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EIO;
>> +
>> +	ret = platform_driver_register(&tdx_attest_driver);
>> +	if (ret) {
>> +		pr_err("failed to register driver, err=%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
>> +	if (IS_ERR(pdev)) {
>> +		ret = PTR_ERR(pdev);
>> +		pr_err("failed to allocate device, err=%d\n", ret);
>> +		platform_driver_unregister(&tdx_attest_driver);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit tdx_attest_exit(void)
>> +{
>> +	platform_device_unregister(pdev);
>> +	platform_driver_unregister(&tdx_attest_driver);
>> +}
>> +
>> +module_init(tdx_attest_init);
>> +module_exit(tdx_attest_exit);
> 
> Is there any particular reason to use platform driver and support it as a
> module?
> 
> SGX driver uses misc_register() to register /dev/sgx_enclave during boot.  Looks
> it would be simpler.

Main reason is to use a proper device in dma_alloc* APIs.

My initial version only used misc device as you have mentioned. But
Hans raised a concern about using proper struct device in dma_alloc*
APIs and suggested modifying the driver to use platform device
model. You can find relevant discussion here.

https://lore.kernel.org/all/47d06f45-c1b5-2c8f-d937-3abacbf10321@redhat.com/
> 
>> +
>> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
>> +MODULE_DESCRIPTION("TDX attestation driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 03deb4d6920d..2a79ca92a52d 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -11,10 +11,12 @@
>>   #include <asm/insn.h>
>>   #include <asm/insn-eval.h>
>>   #include <asm/pgtable.h>
>> +#include <asm/io.h>
>>   
>>   /* TDX module Call Leaf IDs */
>>   #define TDX_GET_INFO			1
>>   #define TDX_GET_VEINFO			3
>> +#define TDX_GET_REPORT			4
>>   #define TDX_ACCEPT_PAGE			6
>>   
>>   /* TDX hypercall Leaf IDs */
>> @@ -34,6 +36,10 @@
>>   #define VE_GET_PORT_NUM(e)	((e) >> 16)
>>   #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>>   
>> +/* TDX Module call error codes */
>> +#define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
>> +#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
>> +
>>   /*
>>    * Wrapper for standard use of __tdx_hypercall with no output aside from
>>    * return code.
>> @@ -98,6 +104,45 @@ 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_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
>> + *
>> + * @data        : Address of 1024B aligned data to store
>> + *                TDREPORT_STRUCT.
>> + * @reportdata  : Address of 64B aligned report data
>> + *
>> + * return 0 on success or failure error number.
>> + */
>> +long tdx_mcall_tdreport(void *data, void *reportdata)
> 
> Change 'data' to be something more meaningful, i.e. tdreport?

Ok. I will change it.

> 
>> +{
>> +	u64 ret;
>> +
>> +	/*
>> +	 * Check for a valid TDX guest to ensure this API is only
>> +	 * used by TDX guest platform. Also make sure "data" and
>> +	 * "reportdata" pointers are valid.
>> +	 */
>> +	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EINVAL;
> 
> Other TDCALL wrappers such as tdx_get_ve_info() doesn't have
> X86_FEATURE_TDX_GUEST check.  Why is it needed in this particular one?

Previously the attestation driver can be compiled as a module so I have
exported this function. But I was not sure whether this function will be
called *only* from the attestation driver. So to protect against any
incorrect usage, I have added check for valid TDX guest with in this
function.

But I think this is no longer required. I will remove it in the next
version.

> 
>> +
>> +	/*
>> +	 * Pass the physical address of user generated report data
>> +	 * and the physical address of output buffer to the TDX module
>> +	 * to generate the TD report. Generated data contains
>> +	 * measurements/configuration data of the TD guest. More info
>> +	 * about ABI can be found in TDX 1.0 Module specification, sec
>> +	 * titled "TDG.MR.REPORT".
>> +	 */
>> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
>> +				virt_to_phys(reportdata), 0, 0, NULL);
>> +
>> +	if (ret)
>> +		return TDCALL_RETURN_CODE(ret);
> 
> If we don't expose TDCALL error to userspace, I don't think this is required?
> 
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
> 
> If you use SGX similar way to use misc_register() at boot but don't support the
> driver as module, then you don't have to export this symbol.

Now that we don't have separate config for attestation, module
compilation is not possible. I will remove it.

> 
>> +
>>   static u64 get_cc_mask(void)
>>   {
>>   	struct tdx_module_output out;
>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>> index 020c81a7c729..a151f69dd6ef 100644
>> --- a/arch/x86/include/asm/tdx.h
>> +++ b/arch/x86/include/asm/tdx.h
>> @@ -67,6 +67,8 @@ void tdx_safe_halt(void);
>>   
>>   bool tdx_early_handle_ve(struct pt_regs *regs);
>>   
>> +long tdx_mcall_tdreport(void *data, void *reportdata);
>> +
>>   #else
>>   
>>   static inline void tdx_early_init(void) { };
>> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
>> new file mode 100644
>> index 000000000000..c21f9d6fe88b
>> --- /dev/null
>> +++ b/arch/x86/include/uapi/asm/tdx.h
>> @@ -0,0 +1,23 @@
>> +/* 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>
>> +
>> +/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
>> +#define TDX_REPORT_DATA_LEN		64
> 
> I'd change to TDX_REPORTDATA_LEN to make it consistent with spec.
> 
> REPORT_DATA can be vague.

Ok.

> 
>> +
>> +/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
>> +#define TDX_TDREPORT_LEN		1024
>> +
>> +/*
>> + * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
> 
> "TDREPORT data" -> "TDREPORT", or "TDREPORT_STRUCT".

Ok. I will make it consistent.

> 
>> + * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
>> + * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
>> + * TDREPORT data is copied to the user buffer. On failure, TDCALL error
>> + * code is copied back to the user buffer.
> 
> As I commented above, I am not convinced we need to copy TDCALL error code back
> to userspace.  If we want to do that, we need to explicitly tell on what error
> code (-EIO, i.e.), the TDCALL error code is copied back.
> 

I will remove the error code return support.



-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Kai,

On 4/24/22 10:44 PM, Kai Huang wrote:
>> In TDX guest, attestation is used to verify the trustworthiness of a TD
>> to other entities before making any secure communication.
> Before provisioning secrets to the TD.
> 
>> One usage example is, when a TD guest uses encrypted drive and the
>> decryption keys required to access the drive are stored in a secure
>> 3rd party keyserver, TD guest can use the quote generated via the
>> attestation process to prove its trustworthiness with keyserver and
>> get access to the storage keys.
> "The key server can use attestation to verify TD's trustworthiness and release
> the decryption keys to the TD".
> 
> It is the key server who starts the attestation request, but not the TD.
> 
>> General steps involved in attestation process are,
>>
>>    1. TD guest generates the TDREPORT that contains version information
>>       about the Intel TDX module, measurement of the TD, along with a
>>       TD-specified nonce.
>>    2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
>>       which is used by the host to generate a quote via quoting
>>       enclave (QE).
>>    3. Quote generation completion notification is sent to TD OS via
>>       callback interrupt vector configured by TD using
>>       SetupEventNotifyInterrupt hypercall.
>>    4. After receiving the generated TDQUOTE, a remote verifier can be
>>       used to verify the quote and confirm the trustworthiness of the
>>       TD.
> Let's separate TDREPORT generation and Quote generation, and get rid of details
> of how to get Quote part for now. Detail of GetQuote belongs to the patch which
> implements it.  Here I think we should focus on explaining why we need such a
> basic driver to allow userspace to get the TDREPORT.
> 
> Below is for your reference:
> 
> "
> 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 to contain the TD-specific informatin (such as TD
> measurements), platform information such as the security version of the platform
> and the TDX module and the MAC to protect the integrity of the TDREPORT. 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.
> 
> TDREPORT can only be verified locally as the MAC key is bound to the platform.
> TDX attestation leverages Intel SGX Quote Enclave (QE) 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
> attestation process is to send the TDREPORT to QE to generate the Quote.
> 
> How is the QE, or Quote Generation Service (QGS) in general, implemented and
> deployed is implementation specific.  As a result, how to send the TDREPORT to
> QE/QGS also depends on QE/QGS implementation and the deployment.  TDX doesn't
> support SGX inside a TD, so the QE/QGS can be deployed in the host, or inside a
> dedicated legacy VM.
> 
> A typical implementation is TD userspace attestation software gets the TDREPORT
> from TD kernel, sends it to QE/QGS, and QE/QGS returns the Quote.  The data and
> data format that TD userspace attestation software sends to the QE/QGS is also
> implementation specific, but not necessarily just the raw TDREPORT.  TD
> attestation software can use any available communication channel to talk to
> QE/QGS, such as using vsock and tcp/ip.
> 
> To support the case that those communication channels are not directly available
> to the TD, TDX also defines TDVMCALLs to allow TD to use TDVMCALL to ask VMM to
> help with sending the TDREPORT and receiving the Quote.  This support is
> documented in the GHCI spec "5.4 TD attestation".
> 
> Implement a basic attestation driver to allow TD userspace to get the TDREPORT
> so the attestation software can send it to the QE to generate a Quote for remote
> verification.
> "
> 
> 
>>       
>> More details on above mentioned steps can be found in TDX Guest-Host
>> Communication Interface (GHCI) for Intel TDX 1.0, section titled
>> "TD attestation".
>>
>> To allow the attestation agent (user application) to implement this
>> feature, add an IOCTL interface to get TDREPORT and TDQUOTE from the
>> user space. Since attestation agent can also use methods like vosck or
>> TCP/IP to get the TDQUOTE, adding an IOCTL interface for it is an
>> optional feature. So to simplify the driver, first add support for
>> TDX_CMD_GET_TDREPORT IOCTL. Support for TDQUOTE IOCTL will be added by
>> follow-on patches.
>>
>> TDREPORT can be generated by sending a TDCALL with leaf ID as 0x04.
>> More details about the TDREPORT TDCALL can be found in Intel Trust
>> Domain Extensions (Intel TDX) Module specification, section titled
>> "TDG.MR.REPORT Leaf". Add a wrapper function (tdx_mcall_tdreport())
>> to get the TDREPORT from the TDX Module. This API will be used by the
>> interface driver to request for TDREPORT.
>>
>> 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.

How about the following summary? It includes important notes mentioned
by you and some more driver info.

x86/tdx: Add TDX Guest attestation interface driver

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 key server, 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".

After getting the TDREPORT, the second step of the attestation process
is to send the TDREPORT to Quoting Enclave (QE) or Quote Generation
Service (QGS) to generate the Quote. However, the method of sending the
TDREPORT to QE/QGS, communication channel used and data format used is
specific to the implementation of QE/QGS.

A typical implementation is, TD userspace attestation software gets the
TDREPORT from TD kernel, sends it to QE/QGS, and QE/QGS returns the
Quote. TD attestation software can use any available communication
channel to talk to QE/QGS, such as using vsock and tcp/ip.

To support the case that those communication channels are not directly
available to the TD, TDX also defines TDVMCALL
(TDG.VP.VMCALL<GetQuote>) to allow TD to ask VMM to help with sending
the TDREPORT and receiving the Quote. This support is documented in the
GHCI spec section titled "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. Add a wrapper function
(tdx_mcall_tdreport()) to get the TDREPORT from the TDX Module. This
API will be used by the interface driver to request for TDREPORT.

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.




-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Kai Huang]

> 
> How about the following summary? It includes important notes mentioned
> by you and some more driver info.

Yes fine to me, except minor comments below:

> 
> x86/tdx: Add TDX Guest attestation interface driver
> 
> 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 key server, 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".
> 
> After getting the TDREPORT, the second step of the attestation process
> is to send the TDREPORT to Quoting Enclave (QE) or Quote Generation
> Service (QGS) to generate the Quote. However, the method of sending the
> TDREPORT to QE/QGS, communication channel used and data format used is
> specific to the implementation of QE/QGS.
> 
> A typical implementation is, TD userspace attestation software gets the
> TDREPORT from TD kernel, sends it to QE/QGS, and QE/QGS returns the
> Quote. TD attestation software can use any available communication
> channel to talk to QE/QGS, such as using vsock and tcp/ip.
> 
> To support the case that those communication channels are not directly
> available to the TD, TDX also defines TDVMCALL
> (TDG.VP.VMCALL<GetQuote>) to allow TD to ask VMM to help with sending
> the TDREPORT and receiving the Quote. This support is documented in the
> GHCI spec section titled "5.4 TD attestation".

I intentionally omitted to mention TDG.VP.VMCALL<GetQuote> as I personally
believe there are still couple issues around GetQuote that we haven't discussed
thoroughly (timeout, etc).  I am still considering whether we should change GHCI
to use TDG.VP.VMCALL<Service> defined in GHCI 1.5 for attestation.  And the name
of TDVMCALL doesn't actually matter here, so I think we don't need to mention
GetQuote here but just say we have TDVMCALLs for that.

> 
> 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. Add a wrapper function
> (tdx_mcall_tdreport()) to get the TDREPORT from the TDX Module. This
> API will be used by the interface driver to request for TDREPORT.

I don't think you need to mention tdx_mcall_tdreport().

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

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Kai Huang]

On Tue, 2022-04-26 at 12:07 -0700, Sathyanarayanan Kuppuswamy wrote:
> > Is there any particular reason to use platform driver and support it as a
> > module?
> > 
> > SGX driver uses misc_register() to register /dev/sgx_enclave during boot. 
> > Looks
> > it would be simpler.
> 
> Main reason is to use a proper device in dma_alloc* APIs.
> 
> My initial version only used misc device as you have mentioned. But
> Hans raised a concern about using proper struct device in dma_alloc*
> APIs and suggested modifying the driver to use platform device
> model. You can find relevant discussion here.
> 
> https://lore.kernel.org/all/47d06f45-c1b5-2c8f-d937-3abacbf10321@redhat.com/

Thanks for the info.  Sorry I didn't dig review comments for previous version.

However, after digging more, I am wondering why do you need to use DMA API at
the first place.

Firstly, for this basic driver to report TDREPORT to userspace, there's no need
to use any DMA API, nor we need to use shared memory, as we just get the report
into some buffer (doesn't need to be shared) and copy the report back to
userspace.  So it doesn't make a lot sense to use platform device here.

Secondly, in terms of GetQuote support, it seems Dave/Andi suggested you can use
vmalloc()/vmap() and just use set_memory_decrypted() to convert it to shared:

https://lore.kernel.org/all/ce0feeec-a949-35f8-3010-b0d69acbbc2e@linux.intel.com/

I don't see why it cannot work?  Then wouldn't this approach be simpler than
using DMA API?  Any reason to choose platform device?

Btw, a side topic:

Andy suggested we don't do memory allocation and private-shared conversion at
IOCTL time as the conversion is expensive:

https://lore.kernel.org/all/06c85c19-e16c-3121-ed47-075cfa779b67@kernel.org/

This is reasonable (and sorry I didn't see this when I commented in v3).

To avoid IOCTL time private-shared conversion, and yet support dynamic Quote
length, can we do following:

- Allocate a *default* size buffer at driver loading time (i.e. 4 pages), and
convert to shared.  This default size should cover 99% cases as Intel QGS
currently generates Quote smaller than 8K, and Intel attestation agent hard-code
a 4 pages buffer for Quote.

- In GetQuote IOCTL, when the len is larger than default size, we discard the
original one and allocate a larger buffer.

How does this sound?


-- 
Thanks,
-Kai

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Isaku Yamahata]

On Fri, Apr 22, 2022 at 04:34:16PM -0700,
Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:

> In TDX guest, attestation is used to verify the trustworthiness of a TD
> to other entities before making any secure communication.
> 
> One usage example is, when a TD guest uses encrypted drive and the
> decryption keys required to access the drive are stored in a secure
> 3rd party keyserver, TD guest can use the quote generated via the
> attestation process to prove its trustworthiness with keyserver and
> get access to the storage keys.
> 
> General steps involved in attestation process are,
> 
>   1. TD guest generates the TDREPORT that contains version information
>      about the Intel TDX module, measurement of the TD, along with a
>      TD-specified nonce.
>   2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
>      which is used by the host to generate a quote via quoting
>      enclave (QE).
>   3. Quote generation completion notification is sent to TD OS via
>      callback interrupt vector configured by TD using
>      SetupEventNotifyInterrupt hypercall.
>   4. After receiving the generated TDQUOTE, a remote verifier can be
>      used to verify the quote and confirm the trustworthiness of the
>      TD.
>      
> More details on above mentioned steps can be found in TDX Guest-Host
> Communication Interface (GHCI) for Intel TDX 1.0, section titled
> "TD attestation".
> 
> To allow the attestation agent (user application) to implement this
> feature, add an IOCTL interface to get TDREPORT and TDQUOTE from the
> user space. Since attestation agent can also use methods like vosck or
> TCP/IP to get the TDQUOTE, adding an IOCTL interface for it is an
> optional feature. So to simplify the driver, first add support for
> TDX_CMD_GET_TDREPORT IOCTL. Support for TDQUOTE IOCTL will be added by
> follow-on patches.
> 
> TDREPORT can be generated by sending a TDCALL with leaf ID as 0x04.
> More details about the TDREPORT TDCALL can be found in Intel Trust
> Domain Extensions (Intel TDX) Module specification, section titled
> "TDG.MR.REPORT Leaf". Add a wrapper function (tdx_mcall_tdreport())
> to get the TDREPORT from the TDX Module. This API will be used by the
> interface driver to request for TDREPORT.
> 
> 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.
> 
> 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      | 191 ++++++++++++++++++++++++++++++++
>  arch/x86/coco/tdx/tdx.c         |  45 ++++++++
>  arch/x86/include/asm/tdx.h      |   2 +
>  arch/x86/include/uapi/asm/tdx.h |  23 ++++
>  5 files changed, 262 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..b776e81f6c20
> --- /dev/null
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process and
> + * read the TD Quote result.
> + *
> + * Copyright (C) 2022 Intel Corporation
> + *
> + */
> +
> +#define pr_fmt(fmt) "x86/tdx: attest: " fmt
> +
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +#include <linux/uaccess.h>
> +#include <linux/fs.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/set_memory.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +#include <linux/jiffies.h>
> +#include <linux/io.h>
> +#include <asm/apic.h>
> +#include <asm/tdx.h>
> +#include <asm/irq_vectors.h>
> +#include <uapi/asm/tdx.h>
> +
> +#define DRIVER_NAME "tdx-attest"
> +
> +static struct platform_device *pdev;
> +static struct miscdevice miscdev;
> +
> +static long tdx_get_tdreport(void __user *argp)
> +{
> +	void *report_buf = NULL, *tdreport_buf = NULL;
> +	long ret = 0, err;
> +
> +	/* Allocate space for report data */
> +	report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
> +	if (!report_buf)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Allocate space for TDREPORT buffer (1024-byte aligned).
> +	 * Full page alignment is more than enough.
> +	 */
> +	tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
> +	if (!tdreport_buf) {
> +		ret = -ENOMEM;
> +		goto tdreport_failed;
> +	}
> +
> +	/* Copy report data to kernel buffer */
> +	if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) {
> +		ret = -EFAULT;
> +		goto tdreport_failed;
> +	}
> +
> +	/* Generate TDREPORT using report data in report_buf */
> +	err = tdx_mcall_tdreport(tdreport_buf, report_buf);
> +	if (err) {
> +		/* If failed, pass TDCALL error code back to user */
> +		ret = put_user(err, (long __user *)argp);

ret is ignored.  ret should be checked or ignore it explicitly by (void).


> +		ret = -EIO;
> +		goto tdreport_failed;
> +	}
> +
> +	/* Copy TDREPORT data back to user buffer */
> +	if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN))
> +		ret = -EFAULT;
> +
> +tdreport_failed:
> +	kfree(report_buf);
> +	if (tdreport_buf)
> +		free_pages((unsigned long)tdreport_buf, 0);
> +
> +	return ret;
> +}
> +
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	long ret = 0;
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_TDREPORT:
> +		ret = tdx_get_tdreport(argp);
> +		break;
> +	default:
> +		pr_err("cmd %d not supported\n", cmd);

pr_debug()?  At least error should be returned to user space.

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct file_operations tdx_attest_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= tdx_attest_ioctl,
> +	.llseek		= no_llseek,
> +};
> +
> +static int tdx_attest_probe(struct platform_device *attest_pdev)
> +{
> +	struct device *dev = &attest_pdev->dev;
> +	long ret = 0;
> +
> +	/* Only single device is allowed */
> +	if (pdev)
> +		return -EBUSY;
> +
> +	pdev = attest_pdev;
> +
> +	miscdev.name = DRIVER_NAME;
> +	miscdev.minor = MISC_DYNAMIC_MINOR;
> +	miscdev.fops = &tdx_attest_fops;
> +	miscdev.parent = dev;
> +
> +	ret = misc_register(&miscdev);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		goto failed;
> +	}
> +
> +	pr_debug("module initialization success\n");
> +
> +	return 0;
> +
> +failed:
> +	misc_deregister(&miscdev);
> +
> +	pr_debug("module initialization failed\n");
> +
> +	return ret;
> +}
> +
> +static int tdx_attest_remove(struct platform_device *attest_pdev)
> +{
> +	misc_deregister(&miscdev);
> +	pr_debug("module is successfully removed\n");
> +	return 0;
> +}
> +
> +static struct platform_driver tdx_attest_driver = {
> +	.probe		= tdx_attest_probe,
> +	.remove		= tdx_attest_remove,
> +	.driver		= {
> +		.name	= DRIVER_NAME,
> +	},
> +};
> +
> +static int __init tdx_attest_init(void)
> +{
> +	int ret;
> +
> +	/* Make sure we are in a valid TDX platform */
> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EIO;
> +
> +	ret = platform_driver_register(&tdx_attest_driver);
> +	if (ret) {
> +		pr_err("failed to register driver, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		ret = PTR_ERR(pdev);
> +		pr_err("failed to allocate device, err=%d\n", ret);
> +		platform_driver_unregister(&tdx_attest_driver);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit tdx_attest_exit(void)
> +{
> +	platform_device_unregister(pdev);
> +	platform_driver_unregister(&tdx_attest_driver);
> +}
> +
> +module_init(tdx_attest_init);
> +module_exit(tdx_attest_exit);
> +
> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
> +MODULE_DESCRIPTION("TDX attestation driver");
> +MODULE_LICENSE("GPL");
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 03deb4d6920d..2a79ca92a52d 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -11,10 +11,12 @@
>  #include <asm/insn.h>
>  #include <asm/insn-eval.h>
>  #include <asm/pgtable.h>
> +#include <asm/io.h>
>  
>  /* TDX module Call Leaf IDs */
>  #define TDX_GET_INFO			1
>  #define TDX_GET_VEINFO			3
> +#define TDX_GET_REPORT			4
>  #define TDX_ACCEPT_PAGE			6
>  
>  /* TDX hypercall Leaf IDs */
> @@ -34,6 +36,10 @@
>  #define VE_GET_PORT_NUM(e)	((e) >> 16)
>  #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>  
> +/* TDX Module call error codes */
> +#define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
> +#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
> +
>  /*
>   * Wrapper for standard use of __tdx_hypercall with no output aside from
>   * return code.
> @@ -98,6 +104,45 @@ 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_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
> + *
> + * @data        : Address of 1024B aligned data to store
> + *                TDREPORT_STRUCT.
> + * @reportdata  : Address of 64B aligned report data
> + *
> + * return 0 on success or failure error number.

Is "failure error number" TDX status code? not -Evalue?


> + */
> +long tdx_mcall_tdreport(void *data, void *reportdata)
> +{
> +	u64 ret;
> +
> +	/*
> +	 * Check for a valid TDX guest to ensure this API is only
> +	 * used by TDX guest platform. Also make sure "data" and
> +	 * "reportdata" pointers are valid.
> +	 */
> +	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EINVAL;
> +
> +	/*
> +	 * Pass the physical address of user generated report data
> +	 * and the physical address of output buffer to the TDX module
> +	 * to generate the TD report. Generated data contains
> +	 * measurements/configuration data of the TD guest. More info
> +	 * about ABI can be found in TDX 1.0 Module specification, sec
> +	 * titled "TDG.MR.REPORT".
> +	 */
> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
> +				virt_to_phys(reportdata), 0, 0, NULL);
> +
> +	if (ret)
> +		return TDCALL_RETURN_CODE(ret);

Why is status code masked?


> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);

Can this function be put into attest.c. Instead, wecan export __tdx_module_call.


>  static u64 get_cc_mask(void)
>  {
>  	struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 020c81a7c729..a151f69dd6ef 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -67,6 +67,8 @@ void tdx_safe_halt(void);
>  
>  bool tdx_early_handle_ve(struct pt_regs *regs);
>  
> +long tdx_mcall_tdreport(void *data, void *reportdata);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> new file mode 100644
> index 000000000000..c21f9d6fe88b
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -0,0 +1,23 @@
> +/* 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>
> +
> +/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
> +#define TDX_REPORT_DATA_LEN		64
> +
> +/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
> +#define TDX_TDREPORT_LEN		1024
> +
> +/*
> + * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
> + * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
> + * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
> + * TDREPORT data is copied to the user buffer. On failure, TDCALL error
> + * code is copied back to the user buffer.
> + */
> +#define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
> +
> +#endif /* _UAPI_ASM_X86_TDX_H */
> -- 
> 2.25.1
> 

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

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Kai Huang]

On Tue, 2022-04-26 at 22:45 -0700, Isaku Yamahata wrote:
> > + */
> > +long tdx_mcall_tdreport(void *data, void *reportdata)
> > +{
> > +	u64 ret;
> > +
> > +	/*
> > +	 * Check for a valid TDX guest to ensure this API is only
> > +	 * used by TDX guest platform. Also make sure "data" and
> > +	 * "reportdata" pointers are valid.
> > +	 */
> > +	if (!data || !reportdata ||
> > !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Pass the physical address of user generated report data
> > +	 * and the physical address of output buffer to the TDX module
> > +	 * to generate the TD report. Generated data contains
> > +	 * measurements/configuration data of the TD guest. More info
> > +	 * about ABI can be found in TDX 1.0 Module specification, sec
> > +	 * titled "TDG.MR.REPORT".
> > +	 */
> > +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
> > +				virt_to_phys(reportdata), 0, 0, NULL);
> > +
> > +	if (ret)
> > +		return TDCALL_RETURN_CODE(ret);
> 
> Why is status code masked?
> 
> 
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
> 
> Can this function be put into attest.c. Instead, wecan export
> __tdx_module_call.

As we discussed, there's no need to export this anymore.

Btw, I even think we can move this function to attest.c so it can be static. 
Theoretically there should be no other caller of this except attest.c.  I
understand in this way we need to define TDX_GET_REPORT macro in attest.c but
this looks fine to me.  This function can even be removed, and we can call
__tdx_module_call() directly in attest.c.  This function literally doesn't do
anything meaningful more than __tdx_module_call(), instead, calling
__tdx_module_call() directly gives us more context, i.e., we can easily see the
buffer is allocated by kernel and passed to TDX module, etc.

-- 
Thanks,
-Kai

Re: [PATCH v4 3/3] x86/tdx: Add Quote generation support

[Isaku Yamahata]

On Fri, Apr 22, 2022 at 04:34:18PM -0700,
Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:

> In TDX guest, the second stage in attestation process is quote
> generation and signing. GetQuote hypercall can be used by the TD guest
> to request VMM facilitate the quote generation via a Quoting Enclave
> (QE). 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 quote is generated. So VMM uses callback interrupt vector
> configured by SetupEventNotifyInterrupt hypercall to notify the guest
> about quote generation completion or failure. Upon receiving the
> completion notification, status can be found in the Quote data header.
> 
> Add tdx_hcall_get_quote() helper function to implement the GetQuote
> hypercall and add TDX_CMD_GEN_QUOTE IOCTL support to allow the user
> agent request for quote generation.
> 
> When a user agent requests for quote generation, it is expected that
> the user agent knows about the Quoting Enclave response time, and sets
> a valid timeout value for the quote generation completion. Timeout
> support is added to make sure the kernel does not wait for the
> quote completion indefinitely.
> 
> Although GHCI specification does not restrict parallel GetQuote
> requests, since quote generation is not in performance critical path
> and the frequency of attestation requests are expected to be low, only
> support serialized quote generation requests. Serialization support is
> added via a mutex lock (attest_lock). Parallel quote request support
> can be added once demand arises.
> 
> 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      | 118 +++++++++++++++++++++++++++++++-
>  arch/x86/coco/tdx/tdx.c         |  37 ++++++++++
>  arch/x86/include/asm/tdx.h      |   2 +
>  arch/x86/include/uapi/asm/tdx.h |  36 ++++++++++
>  4 files changed, 191 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
> index b776e81f6c20..d485163d3222 100644
> --- a/arch/x86/coco/tdx/attest.c
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -32,6 +32,11 @@
>  static struct platform_device *pdev;
>  static struct miscdevice miscdev;
>  
> +/* Completion object to track GetQuote completion status */
> +static DECLARE_COMPLETION(req_compl);
> +/* Mutex to serialize GetQuote requests */
> +static DEFINE_MUTEX(quote_lock);
> +
>  static long tdx_get_tdreport(void __user *argp)
>  {
>  	void *report_buf = NULL, *tdreport_buf = NULL;
> @@ -79,6 +84,95 @@ static long tdx_get_tdreport(void __user *argp)
>  	return ret;
>  }
>  
> +static long tdx_get_tdquote(void __user *argp)
> +{
> +	struct tdx_quote_hdr *quote_hdr;
> +	struct tdx_quote_req quote_req;
> +	void *quote_buf = NULL;
> +	dma_addr_t handle;
> +	long ret = 0, err;
> +	u64 quote_buf_len;
> +
> +	mutex_lock(&quote_lock);
> +
> +	reinit_completion(&req_compl);
> +
> +	/* Copy Quote request struct from user buffer */
> +	if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
> +		return -EFAULT;
> +
> +	/* Make sure the length & timeout is valid */
> +	if (quote_req.len <= 0 || quote_req.timeout <= 0)
> +		return -EINVAL;
> +
> +	/* Align with page size to meet 4K alignment */
> +	quote_buf_len = PAGE_ALIGN(quote_req.len);
> +
> +	/*
> +	 * Allocate DMA buffer to get TDQUOTE data from the VMM.
> +	 * dma_alloc_coherent() API internally marks allocated
> +	 * memory as shared with VMM. So explicit shared mapping is
> +	 * not required.
> +	 */
> +	quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
> +					GFP_KERNEL | __GFP_ZERO);
> +	if (!quote_buf) {
> +		ret = -ENOMEM;
> +		goto quote_failed;
> +	}
> +
> +	/* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
> +	if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
> +		ret = -EFAULT;
> +		goto quote_failed;
> +	}
> +
> +	/* Submit GetQuote Request */
> +	err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
> +	if (err) {
> +		/* if failed, copy hypercall error code to user buffer */
> +		ret = put_user(err, (long __user *)argp);

ret is ignored.  Do you want to return TDX status code to user?
Does just -EIO suffice?
(If you really want, the TDX status code should be defined as uapi.)


> +		ret = -EIO;
> +		goto quote_failed;
> +	}
> +
> +	/* Wait for attestation completion */
> +	ret = wait_for_completion_interruptible_timeout(
> +			&req_compl,
> +			msecs_to_jiffies(quote_req.timeout));

If you want to support timeout, you need to handle in-flight case below.


> +	if (ret <= 0) {
> +		ret = -EIO;
> +		goto quote_failed;
> +	}
> +
> +	/* Copy generated Quote data back to user buffer */
> +	if (copy_to_user((void __user *)quote_req.buf, quote_buf, quote_buf_len)) {
> +		ret = -EFAULT;
> +		goto quote_failed;
> +	}
> +
> +	quote_hdr = (struct tdx_quote_hdr *)quote_buf;
> +
> +	/* Make sure quote generation is successful */
> +	if (!quote_hdr->status)

status check should be before copy_to_user() above.


> +		ret = 0;
> +	else
> +		ret = -EIO;
> +
> +quote_failed:
> +	if (quote_buf)
> +		dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);

quote_buf can be still owned by VMM because timeout is used above.
Even if interrupt is arrived,  quote_hdr->status can still be in-flight.


> +
> +	mutex_unlock(&quote_lock);
> +
> +	return ret;
> +}
> +
> +static void attestation_callback_handler(void)
> +{
> +	complete(&req_compl);
> +}
> +
>  static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>  			     unsigned long arg)
>  {
> @@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>  	case TDX_CMD_GET_TDREPORT:
>  		ret = tdx_get_tdreport(argp);
>  		break;
> +	case TDX_CMD_GEN_QUOTE:
> +		ret = tdx_get_tdquote(argp);
> +		break;
>  	default:
>  		pr_err("cmd %d not supported\n", cmd);
>  		break;
> @@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
>  	.llseek		= no_llseek,
>  };
>  
> +/* Helper function to cleanup attestation related allocations */
> +static void _tdx_attest_remove(void)
> +{
> +	misc_deregister(&miscdev);
> +
> +	tdx_remove_ev_notify_handler();
> +}
> +
>  static int tdx_attest_probe(struct platform_device *attest_pdev)
>  {
>  	struct device *dev = &attest_pdev->dev;
> @@ -114,6 +219,15 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>  
>  	pdev = attest_pdev;
>  
> +	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> +	if (ret) {
> +		pr_err("dma set coherent mask failed\n");
> +		goto failed;
> +	}
> +
> +	/* 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;
> @@ -130,7 +244,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>  	return 0;
>  
>  failed:
> -	misc_deregister(&miscdev);
> +	_tdx_attest_remove();
>  
>  	pr_debug("module initialization failed\n");
>  
> @@ -139,7 +253,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>  
>  static int tdx_attest_remove(struct platform_device *attest_pdev)
>  {
> -	misc_deregister(&miscdev);
> +	_tdx_attest_remove();
>  	pr_debug("module is successfully removed\n");
>  	return 0;
>  }
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index d0c62b94a1f6..cba22a8d4084 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -25,6 +25,7 @@
>  
>  /* TDX hypercall Leaf IDs */
>  #define TDVMCALL_MAP_GPA		0x10001
> +#define TDVMCALL_GET_QUOTE		0x10002
>  #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
>  
>  /* MMIO direction */
> @@ -214,6 +215,42 @@ static long tdx_hcall_set_notify_intr(u8 vector)
>  	return 0;
>  }
>  
> +/*
> + * tdx_hcall_get_quote() - Request for TDQUOTE using TDREPORT.
> + *
> + * @data        : Address of 4KB aligned GPA memory which contains
> + *                TDREPORT_STRUCT.
> + * @len		: Length of the GPA in bytes.
> + *
> + * return 0 on success or failure error number.
> + */
> +long tdx_hcall_get_quote(void *data, u64 len)
> +{
> +	u64 ret;
> +
> +	/*
> +	 * Use confidential guest TDX check to ensure this API is only
> +	 * used by TDX guest platforms.
> +	 */
> +	if (!data || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EINVAL;

Isn't X86_FEATURE_TDX_GUEST checked on module loading time?

> +
> +	/*
> +	 * Pass the physical address of tdreport data to the VMM
> +	 * and trigger the TDQUOTE 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>".
> +	 */
> +	ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
> +			     len, 0, 0);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static u64 get_cc_mask(void)
>  {
>  	struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 89ed09809c13..90c2a5f6c40c 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));
>  
>  void tdx_remove_ev_notify_handler(void);
>  
> +long tdx_hcall_get_quote(void *data, u64 len);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> index c21f9d6fe88b..69259b7841a9 100644
> --- a/arch/x86/include/uapi/asm/tdx.h
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -20,4 +20,40 @@
>   */
>  #define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
>  
> +/*
> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
> + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
> + * buffer of quote size. Once IOCTL is successful quote data is copied back to
> + * the user buffer. On failure, TDCALL error code is copied back to the user
> + * buffer.
> + */
> +#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
> +
> +struct tdx_quote_req {
> +	/* Buffer address to store Quote data */
> +	__u64 buf;
> +	/* Length of the Quote buffer */
> +	__u64 len;
> +	/* Quote generation timeout value in ms */
> +	__u32 timeout;

What's the point of timeout?


> +};
> +
> +/*
> + * 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;

If you export version and status, also define related constants for user space.


> +	/* Length of TDREPORT, filled by TD */
> +	__u32 in_len;
> +	/* Length of Quote, filled by VMM */
> +	__u32 out_len;
> +	/* Actual Quote data */
> +	__u64 data;
> +};
> +
>  #endif /* _UAPI_ASM_X86_TDX_H */
> -- 
> 2.25.1
> 

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

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/26/22 9:28 PM, Kai Huang wrote:
> 
>>
>> How about the following summary? It includes important notes mentioned
>> by you and some more driver info.
> 
> Yes fine to me, except minor comments below:
> 
>>
>> x86/tdx: Add TDX Guest attestation interface driver
>>
>> 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 key server, 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".
>>
>> After getting the TDREPORT, the second step of the attestation process
>> is to send the TDREPORT to Quoting Enclave (QE) or Quote Generation
>> Service (QGS) to generate the Quote. However, the method of sending the
>> TDREPORT to QE/QGS, communication channel used and data format used is
>> specific to the implementation of QE/QGS.
>>
>> A typical implementation is, TD userspace attestation software gets the
>> TDREPORT from TD kernel, sends it to QE/QGS, and QE/QGS returns the
>> Quote. TD attestation software can use any available communication
>> channel to talk to QE/QGS, such as using vsock and tcp/ip.
>>
>> To support the case that those communication channels are not directly
>> available to the TD, TDX also defines TDVMCALL
>> (TDG.VP.VMCALL<GetQuote>) to allow TD to ask VMM to help with sending
>> the TDREPORT and receiving the Quote. This support is documented in the
>> GHCI spec section titled "5.4 TD attestation".
> 
> I intentionally omitted to mention TDG.VP.VMCALL<GetQuote> as I personally
> believe there are still couple issues around GetQuote that we haven't discussed
> thoroughly (timeout, etc).  I am still considering whether we should change GHCI
> to use TDG.VP.VMCALL<Service> defined in GHCI 1.5 for attestation.  And the name
> of TDVMCALL doesn't actually matter here, so I think we don't need to mention
> GetQuote here but just say we have TDVMCALLs for that.

Ok.

> 
>>
>> 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. Add a wrapper function
>> (tdx_mcall_tdreport()) to get the TDREPORT from the TDX Module. This
>> API will be used by the interface driver to request for TDREPORT.
> 
> I don't think you need to mention tdx_mcall_tdreport().

Ok. Will remove it.

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

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi,

On 4/26/22 10:15 PM, Kai Huang wrote:
> On Tue, 2022-04-26 at 12:07 -0700, Sathyanarayanan Kuppuswamy wrote:
>>> Is there any particular reason to use platform driver and support it as a
>>> module?
>>>
>>> SGX driver uses misc_register() to register /dev/sgx_enclave during boot.
>>> Looks
>>> it would be simpler.
>>
>> Main reason is to use a proper device in dma_alloc* APIs.
>>
>> My initial version only used misc device as you have mentioned. But
>> Hans raised a concern about using proper struct device in dma_alloc*
>> APIs and suggested modifying the driver to use platform device
>> model. You can find relevant discussion here.
>>
>> https://lore.kernel.org/all/47d06f45-c1b5-2c8f-d937-3abacbf10321@redhat.com/
> 
> Thanks for the info.  Sorry I didn't dig review comments for previous version.
> 
> However, after digging more, I am wondering why do you need to use DMA API at
> the first place.
> 
> Firstly, for this basic driver to report TDREPORT to userspace, there's no need
> to use any DMA API, nor we need to use shared memory, as we just get the report
> into some buffer (doesn't need to be shared) and copy the report back to
> userspace.  So it doesn't make a lot sense to use platform device here.

Yes. For this patch itself, since  we don't need to use DMA API,
platform driver model is not required. But I have made this patch use
platform driver format in consideration of its need in the next patch.
Making it misc driver in this patch and changing it to platform driver
in next patch does not make sense. Since they are all in the same patch
set we can add some changes in consideration of the next patch.

> 
> Secondly, in terms of GetQuote support, it seems Dave/Andi suggested you can use
> vmalloc()/vmap() and just use set_memory_decrypted() to convert it to shared:
> 
> https://lore.kernel.org/all/ce0feeec-a949-35f8-3010-b0d69acbbc2e@linux.intel.com/
> 
> I don't see why it cannot work?  Then wouldn't this approach be simpler than
> using DMA API?  Any reason to choose platform device?

Yes, it will work. But I am not sure whether it is simpler than adding
platform driver specific buffer code. I have used DMA APIs because it
will handle allocation and decryption setting internally. I thought is
simpler than we handling it ourselves.

But if platform device driver model is not preferred, I can change it.


> 
> Btw, a side topic:
> 
> Andy suggested we don't do memory allocation and private-shared conversion at
> IOCTL time as the conversion is expensive:
> 
> https://lore.kernel.org/all/06c85c19-e16c-3121-ed47-075cfa779b67@kernel.org/
> 
> This is reasonable (and sorry I didn't see this when I commented in v3).
> 
> To avoid IOCTL time private-shared conversion, and yet support dynamic Quote
> length, can we do following:
> 
> - Allocate a *default* size buffer at driver loading time (i.e. 4 pages), and
> convert to shared.  This default size should cover 99% cases as Intel QGS
> currently generates Quote smaller than 8K, and Intel attestation agent hard-code
> a 4 pages buffer for Quote.
> 
> - In GetQuote IOCTL, when the len is larger than default size, we discard the
> original one and allocate a larger buffer.
> 
> How does this sound?

It sounds fine. Your suggestion can indeed decrease the IOCTL time.

But, IMO, since attestation will not be used that frequently,
we don't need to consider optimization at this point of time. Also, I
think the memory allocation time is negligible compared to time it takes
for the TDQUOTE generation.

Even if we have to do it, we can add it in future as a separate
patch. We don't need to add it part of this basic driver support
patch.

> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Isaku,

On 4/26/22 10:45 PM, Isaku Yamahata wrote:
> On Fri, Apr 22, 2022 at 04:34:16PM -0700,
> Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 
>> In TDX guest, attestation is used to verify the trustworthiness of a TD
>> to other entities before making any secure communication.
>>
>> One usage example is, when a TD guest uses encrypted drive and the
>> decryption keys required to access the drive are stored in a secure
>> 3rd party keyserver, TD guest can use the quote generated via the
>> attestation process to prove its trustworthiness with keyserver and
>> get access to the storage keys.
>>
>> General steps involved in attestation process are,
>>
>>    1. TD guest generates the TDREPORT that contains version information
>>       about the Intel TDX module, measurement of the TD, along with a
>>       TD-specified nonce.
>>    2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
>>       which is used by the host to generate a quote via quoting
>>       enclave (QE).
>>    3. Quote generation completion notification is sent to TD OS via
>>       callback interrupt vector configured by TD using
>>       SetupEventNotifyInterrupt hypercall.
>>    4. After receiving the generated TDQUOTE, a remote verifier can be
>>       used to verify the quote and confirm the trustworthiness of the
>>       TD.
>>       
>> More details on above mentioned steps can be found in TDX Guest-Host
>> Communication Interface (GHCI) for Intel TDX 1.0, section titled
>> "TD attestation".
>>
>> To allow the attestation agent (user application) to implement this
>> feature, add an IOCTL interface to get TDREPORT and TDQUOTE from the
>> user space. Since attestation agent can also use methods like vosck or
>> TCP/IP to get the TDQUOTE, adding an IOCTL interface for it is an
>> optional feature. So to simplify the driver, first add support for
>> TDX_CMD_GET_TDREPORT IOCTL. Support for TDQUOTE IOCTL will be added by
>> follow-on patches.
>>
>> TDREPORT can be generated by sending a TDCALL with leaf ID as 0x04.
>> More details about the TDREPORT TDCALL can be found in Intel Trust
>> Domain Extensions (Intel TDX) Module specification, section titled
>> "TDG.MR.REPORT Leaf". Add a wrapper function (tdx_mcall_tdreport())
>> to get the TDREPORT from the TDX Module. This API will be used by the
>> interface driver to request for TDREPORT.
>>
>> 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.
>>
>> 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      | 191 ++++++++++++++++++++++++++++++++
>>   arch/x86/coco/tdx/tdx.c         |  45 ++++++++
>>   arch/x86/include/asm/tdx.h      |   2 +
>>   arch/x86/include/uapi/asm/tdx.h |  23 ++++
>>   5 files changed, 262 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..b776e81f6c20
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * attest.c - TDX guest attestation interface driver.
>> + *
>> + * Implements user interface to trigger attestation process and
>> + * read the TD Quote result.
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) "x86/tdx: attest: " fmt
>> +
>> +#include <linux/module.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/fs.h>
>> +#include <linux/mm.h>
>> +#include <linux/slab.h>
>> +#include <linux/set_memory.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/io.h>
>> +#include <asm/apic.h>
>> +#include <asm/tdx.h>
>> +#include <asm/irq_vectors.h>
>> +#include <uapi/asm/tdx.h>
>> +
>> +#define DRIVER_NAME "tdx-attest"
>> +
>> +static struct platform_device *pdev;
>> +static struct miscdevice miscdev;
>> +
>> +static long tdx_get_tdreport(void __user *argp)
>> +{
>> +	void *report_buf = NULL, *tdreport_buf = NULL;
>> +	long ret = 0, err;
>> +
>> +	/* Allocate space for report data */
>> +	report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
>> +	if (!report_buf)
>> +		return -ENOMEM;
>> +
>> +	/*
>> +	 * Allocate space for TDREPORT buffer (1024-byte aligned).
>> +	 * Full page alignment is more than enough.
>> +	 */
>> +	tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
>> +	if (!tdreport_buf) {
>> +		ret = -ENOMEM;
>> +		goto tdreport_failed;
>> +	}
>> +
>> +	/* Copy report data to kernel buffer */
>> +	if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) {
>> +		ret = -EFAULT;
>> +		goto tdreport_failed;
>> +	}
>> +
>> +	/* Generate TDREPORT using report data in report_buf */
>> +	err = tdx_mcall_tdreport(tdreport_buf, report_buf);
>> +	if (err) {
>> +		/* If failed, pass TDCALL error code back to user */
>> +		ret = put_user(err, (long __user *)argp);
> 
> ret is ignored.  ret should be checked or ignore it explicitly by (void).

Since we are already in error path and we are going to return -EIO, I
did not  check for it. I will ignore it explictly in next version.


> 
> 
>> +		ret = -EIO;
>> +		goto tdreport_failed;
>> +	}
>> +
>> +	/* Copy TDREPORT data back to user buffer */
>> +	if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN))
>> +		ret = -EFAULT;
>> +
>> +tdreport_failed:
>> +	kfree(report_buf);
>> +	if (tdreport_buf)
>> +		free_pages((unsigned long)tdreport_buf, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>> +			     unsigned long arg)
>> +{
>> +	void __user *argp = (void __user *)arg;
>> +	long ret = 0;
>> +
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_TDREPORT:
>> +		ret = tdx_get_tdreport(argp);
>> +		break;
>> +	default:
>> +		pr_err("cmd %d not supported\n", cmd);
> 
> pr_debug()?  At least error should be returned to user space.

Ok. I will change it to pr_debug(). I have already addressed the
issue of not setting error value in default case.

> 
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations tdx_attest_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.unlocked_ioctl	= tdx_attest_ioctl,
>> +	.llseek		= no_llseek,
>> +};
>> +
>> +static int tdx_attest_probe(struct platform_device *attest_pdev)
>> +{
>> +	struct device *dev = &attest_pdev->dev;
>> +	long ret = 0;
>> +
>> +	/* Only single device is allowed */
>> +	if (pdev)
>> +		return -EBUSY;
>> +
>> +	pdev = attest_pdev;
>> +
>> +	miscdev.name = DRIVER_NAME;
>> +	miscdev.minor = MISC_DYNAMIC_MINOR;
>> +	miscdev.fops = &tdx_attest_fops;
>> +	miscdev.parent = dev;
>> +
>> +	ret = misc_register(&miscdev);
>> +	if (ret) {
>> +		pr_err("misc device registration failed\n");
>> +		goto failed;
>> +	}
>> +
>> +	pr_debug("module initialization success\n");
>> +
>> +	return 0;
>> +
>> +failed:
>> +	misc_deregister(&miscdev);
>> +
>> +	pr_debug("module initialization failed\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int tdx_attest_remove(struct platform_device *attest_pdev)
>> +{
>> +	misc_deregister(&miscdev);
>> +	pr_debug("module is successfully removed\n");
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver tdx_attest_driver = {
>> +	.probe		= tdx_attest_probe,
>> +	.remove		= tdx_attest_remove,
>> +	.driver		= {
>> +		.name	= DRIVER_NAME,
>> +	},
>> +};
>> +
>> +static int __init tdx_attest_init(void)
>> +{
>> +	int ret;
>> +
>> +	/* Make sure we are in a valid TDX platform */
>> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EIO;
>> +
>> +	ret = platform_driver_register(&tdx_attest_driver);
>> +	if (ret) {
>> +		pr_err("failed to register driver, err=%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
>> +	if (IS_ERR(pdev)) {
>> +		ret = PTR_ERR(pdev);
>> +		pr_err("failed to allocate device, err=%d\n", ret);
>> +		platform_driver_unregister(&tdx_attest_driver);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit tdx_attest_exit(void)
>> +{
>> +	platform_device_unregister(pdev);
>> +	platform_driver_unregister(&tdx_attest_driver);
>> +}
>> +
>> +module_init(tdx_attest_init);
>> +module_exit(tdx_attest_exit);
>> +
>> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>");
>> +MODULE_DESCRIPTION("TDX attestation driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 03deb4d6920d..2a79ca92a52d 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -11,10 +11,12 @@
>>   #include <asm/insn.h>
>>   #include <asm/insn-eval.h>
>>   #include <asm/pgtable.h>
>> +#include <asm/io.h>
>>   
>>   /* TDX module Call Leaf IDs */
>>   #define TDX_GET_INFO			1
>>   #define TDX_GET_VEINFO			3
>> +#define TDX_GET_REPORT			4
>>   #define TDX_ACCEPT_PAGE			6
>>   
>>   /* TDX hypercall Leaf IDs */
>> @@ -34,6 +36,10 @@
>>   #define VE_GET_PORT_NUM(e)	((e) >> 16)
>>   #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>>   
>> +/* TDX Module call error codes */
>> +#define TDCALL_RETURN_CODE_MASK		0xffffffff00000000
>> +#define TDCALL_RETURN_CODE(a)		((a) & TDCALL_RETURN_CODE_MASK)
>> +
>>   /*
>>    * Wrapper for standard use of __tdx_hypercall with no output aside from
>>    * return code.
>> @@ -98,6 +104,45 @@ 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_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
>> + *
>> + * @data        : Address of 1024B aligned data to store
>> + *                TDREPORT_STRUCT.
>> + * @reportdata  : Address of 64B aligned report data
>> + *
>> + * return 0 on success or failure error number.
> 
> Is "failure error number" TDX status code? not -Evalue?

It is not -Evalue. I can use the term "TDX error code"

> 
> 
>> + */
>> +long tdx_mcall_tdreport(void *data, void *reportdata)
>> +{
>> +	u64 ret;
>> +
>> +	/*
>> +	 * Check for a valid TDX guest to ensure this API is only
>> +	 * used by TDX guest platform. Also make sure "data" and
>> +	 * "reportdata" pointers are valid.
>> +	 */
>> +	if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Pass the physical address of user generated report data
>> +	 * and the physical address of output buffer to the TDX module
>> +	 * to generate the TD report. Generated data contains
>> +	 * measurements/configuration data of the TD guest. More info
>> +	 * about ABI can be found in TDX 1.0 Module specification, sec
>> +	 * titled "TDG.MR.REPORT".
>> +	 */
>> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
>> +				virt_to_phys(reportdata), 0, 0, NULL);
>> +
>> +	if (ret)
>> +		return TDCALL_RETURN_CODE(ret);
> 
> Why is status code masked?

Only upper 32 bit has error code. Check "Function Completion Status
Codes" in TDX module spec.

> 
> 
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
> 
> Can this function be put into attest.c. Instead, wecan export __tdx_module_call.

We don't need to export it anymore. I will remove it in next version.

Regarding moving it to attest.c, currently all TDCALL/TDVMCALL wrappers
are left in one place (in tdx.c). For example check tdx_kvm_hypercall()
implementation in tdx.c. I don't see any specific need to break this
uniformity for our use case. Grouping them together will make
it easier if we are fixing some core issues in core functions like 
__tdx_hypercall() in future.


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Kai Huang]

On Wed, 2022-04-27 at 14:45 -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi,
> 
> On 4/26/22 10:15 PM, Kai Huang wrote:
> > On Tue, 2022-04-26 at 12:07 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > > Is there any particular reason to use platform driver and support it as a
> > > > module?
> > > > 
> > > > SGX driver uses misc_register() to register /dev/sgx_enclave during boot.
> > > > Looks
> > > > it would be simpler.
> > > 
> > > Main reason is to use a proper device in dma_alloc* APIs.
> > > 
> > > My initial version only used misc device as you have mentioned. But
> > > Hans raised a concern about using proper struct device in dma_alloc*
> > > APIs and suggested modifying the driver to use platform device
> > > model. You can find relevant discussion here.
> > > 
> > > https://lore.kernel.org/all/47d06f45-c1b5-2c8f-d937-3abacbf10321@redhat.com/
> > 
> > Thanks for the info.  Sorry I didn't dig review comments for previous version.
> > 
> > However, after digging more, I am wondering why do you need to use DMA API at
> > the first place.
> > 
> > Firstly, for this basic driver to report TDREPORT to userspace, there's no need
> > to use any DMA API, nor we need to use shared memory, as we just get the report
> > into some buffer (doesn't need to be shared) and copy the report back to
> > userspace.  So it doesn't make a lot sense to use platform device here.
> 
> Yes. For this patch itself, since  we don't need to use DMA API,
> platform driver model is not required. But I have made this patch use
> platform driver format in consideration of its need in the next patch.
> Making it misc driver in this patch and changing it to platform driver
> in next patch does not make sense. Since they are all in the same patch
> set we can add some changes in consideration of the next patch.
> 
> > 
> > Secondly, in terms of GetQuote support, it seems Dave/Andi suggested you can use
> > vmalloc()/vmap() and just use set_memory_decrypted() to convert it to shared:
> > 
> > https://lore.kernel.org/all/ce0feeec-a949-35f8-3010-b0d69acbbc2e@linux.intel.com/
> > 
> > I don't see why it cannot work?  Then wouldn't this approach be simpler than
> > using DMA API?  Any reason to choose platform device?
> 
> Yes, it will work. But I am not sure whether it is simpler than adding
> platform driver specific buffer code. I have used DMA APIs because it
> will handle allocation and decryption setting internally. I thought is
> simpler than we handling it ourselves.
> 
> But if platform device driver model is not preferred, I can change it.

I don't think ignoring Dave/Andi's comments w/o providing feedback is good.

Also I personally don't see how using DMA API is better than using
vmalloc()/vmap().  In order to use DMA API, you have to add more code to use
platform_device, which isn't necessary.

I'll leave this to Dave/Andi.

> 
> 
> > 
> > Btw, a side topic:
> > 
> > Andy suggested we don't do memory allocation and private-shared conversion at
> > IOCTL time as the conversion is expensive:
> > 
> > https://lore.kernel.org/all/06c85c19-e16c-3121-ed47-075cfa779b67@kernel.org/
> > 
> > This is reasonable (and sorry I didn't see this when I commented in v3).
> > 
> > To avoid IOCTL time private-shared conversion, and yet support dynamic Quote
> > length, can we do following:
> > 
> > - Allocate a *default* size buffer at driver loading time (i.e. 4 pages), and
> > convert to shared.  This default size should cover 99% cases as Intel QGS
> > currently generates Quote smaller than 8K, and Intel attestation agent hard-code
> > a 4 pages buffer for Quote.
> > 
> > - In GetQuote IOCTL, when the len is larger than default size, we discard the
> > original one and allocate a larger buffer.
> > 
> > How does this sound?
> 
> It sounds fine. Your suggestion can indeed decrease the IOCTL time.
> 
> But, IMO, since attestation will not be used that frequently,
> we don't need to consider optimization at this point of time. Also, I
> think the memory allocation time is negligible compared to time it takes
> for the TDQUOTE generation.
> 
> Even if we have to do it, we can add it in future as a separate
> patch. We don't need to add it part of this basic driver support
> patch.
> 
> 

I am just pointing out Andy made such suggestion before, and it's not something
we cannot support.

Anyway will let you decide.


-- 
Thanks,
-Kai

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Kai,

On 4/27/22 4:40 PM, Kai Huang wrote:
> On Wed, 2022-04-27 at 14:45 -0700, Sathyanarayanan Kuppuswamy wrote:
>> Hi,
>>
>> On 4/26/22 10:15 PM, Kai Huang wrote:
>>> On Tue, 2022-04-26 at 12:07 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>>> Is there any particular reason to use platform driver and support it as a
>>>>> module?
>>>>>
>>>>> SGX driver uses misc_register() to register /dev/sgx_enclave during boot.
>>>>> Looks
>>>>> it would be simpler.
>>>>
>>>> Main reason is to use a proper device in dma_alloc* APIs.
>>>>
>>>> My initial version only used misc device as you have mentioned. But
>>>> Hans raised a concern about using proper struct device in dma_alloc*
>>>> APIs and suggested modifying the driver to use platform device
>>>> model. You can find relevant discussion here.
>>>>
>>>> https://lore.kernel.org/all/47d06f45-c1b5-2c8f-d937-3abacbf10321@redhat.com/
>>>
>>> Thanks for the info.  Sorry I didn't dig review comments for previous version.
>>>
>>> However, after digging more, I am wondering why do you need to use DMA API at
>>> the first place.
>>>
>>> Firstly, for this basic driver to report TDREPORT to userspace, there's no need
>>> to use any DMA API, nor we need to use shared memory, as we just get the report
>>> into some buffer (doesn't need to be shared) and copy the report back to
>>> userspace.  So it doesn't make a lot sense to use platform device here.
>>
>> Yes. For this patch itself, since  we don't need to use DMA API,
>> platform driver model is not required. But I have made this patch use
>> platform driver format in consideration of its need in the next patch.
>> Making it misc driver in this patch and changing it to platform driver
>> in next patch does not make sense. Since they are all in the same patch
>> set we can add some changes in consideration of the next patch.
>>
>>>
>>> Secondly, in terms of GetQuote support, it seems Dave/Andi suggested you can use
>>> vmalloc()/vmap() and just use set_memory_decrypted() to convert it to shared:
>>>
>>> https://lore.kernel.org/all/ce0feeec-a949-35f8-3010-b0d69acbbc2e@linux.intel.com/
>>>
>>> I don't see why it cannot work?  Then wouldn't this approach be simpler than
>>> using DMA API?  Any reason to choose platform device?
>>
>> Yes, it will work. But I am not sure whether it is simpler than adding
>> platform driver specific buffer code. I have used DMA APIs because it
>> will handle allocation and decryption setting internally. I thought is
>> simpler than we handling it ourselves.
>>
>> But if platform device driver model is not preferred, I can change it.
> 
> I don't think ignoring Dave/Andi's comments w/o providing feedback is good.

It is not intentional. I think it is missed during my vacation due to
some mail access issues. Sorry about it.

> 
> Also I personally don't see how using DMA API is better than using
> vmalloc()/vmap().  In order to use DMA API, you have to add more code to use
> platform_device, which isn't necessary.
> 
> I'll leave this to Dave/Andi.

As I said, I am fine with the change (if it is preferred).

> 
>>
>>
>>>
>>> Btw, a side topic:
>>>
>>> Andy suggested we don't do memory allocation and private-shared conversion at
>>> IOCTL time as the conversion is expensive:
>>>
>>> https://lore.kernel.org/all/06c85c19-e16c-3121-ed47-075cfa779b67@kernel.org/
>>>
>>> This is reasonable (and sorry I didn't see this when I commented in v3).
>>>
>>> To avoid IOCTL time private-shared conversion, and yet support dynamic Quote
>>> length, can we do following:
>>>
>>> - Allocate a *default* size buffer at driver loading time (i.e. 4 pages), and
>>> convert to shared.  This default size should cover 99% cases as Intel QGS
>>> currently generates Quote smaller than 8K, and Intel attestation agent hard-code
>>> a 4 pages buffer for Quote.
>>>
>>> - In GetQuote IOCTL, when the len is larger than default size, we discard the
>>> original one and allocate a larger buffer.
>>>
>>> How does this sound?
>>
>> It sounds fine. Your suggestion can indeed decrease the IOCTL time.
>>
>> But, IMO, since attestation will not be used that frequently,
>> we don't need to consider optimization at this point of time. Also, I
>> think the memory allocation time is negligible compared to time it takes
>> for the TDQUOTE generation.
>>
>> Even if we have to do it, we can add it in future as a separate
>> patch. We don't need to add it part of this basic driver support
>> patch.
>>
>>
> 
> I am just pointing out Andy made such suggestion before, and it's not something
> we cannot support.
> 
> Anyway will let you decide.

Ok.

> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Wander Lairson Costa]

On Fri, Apr 22, 2022 at 04:34:16PM -0700, Kuppuswamy Sathyanarayanan wrote:

[snip]

> +static long tdx_get_tdreport(void __user *argp)
> +{
> +	void *report_buf = NULL, *tdreport_buf = NULL;
> +	long ret = 0, err;
> +
> +	/* Allocate space for report data */
> +	report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
> +	if (!report_buf)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Allocate space for TDREPORT buffer (1024-byte aligned).
> +	 * Full page alignment is more than enough.
> +	 */
> +	tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);

Maybe we should add BUILD_BUG_ON(TDX_TDREPORT_LEN > PAGE_SIZE)

> +	if (!tdreport_buf) {
> +		ret = -ENOMEM;
> +		goto tdreport_failed;
> +	}
> +
> +	/* Copy report data to kernel buffer */
> +	if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) {
> +		ret = -EFAULT;
> +		goto tdreport_failed;
> +	}
> +
> +	/* Generate TDREPORT using report data in report_buf */
> +	err = tdx_mcall_tdreport(tdreport_buf, report_buf);
> +	if (err) {
> +		/* If failed, pass TDCALL error code back to user */
> +		ret = put_user(err, (long __user *)argp);

The assigment to ret is useless here

> +		ret = -EIO;
> +		goto tdreport_failed;
> +	}
> +
> +	/* Copy TDREPORT data back to user buffer */
> +	if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN))
> +		ret = -EFAULT;
> +
> +tdreport_failed:
> +	kfree(report_buf);
> +	if (tdreport_buf)
> +		free_pages((unsigned long)tdreport_buf, 0);
> +
> +	return ret;
> +}
> +
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> +			     unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	long ret = 0;
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_TDREPORT:
> +		ret = tdx_get_tdreport(argp);
> +		break;
> +	default:
> +		pr_err("cmd %d not supported\n", cmd);

Shouldn't we add "ret = -EINVAL" here?

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct file_operations tdx_attest_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= tdx_attest_ioctl,
> +	.llseek		= no_llseek,
> +};
> +
> +static int tdx_attest_probe(struct platform_device *attest_pdev)
> +{
> +	struct device *dev = &attest_pdev->dev;
> +	long ret = 0;
> +
> +	/* Only single device is allowed */
> +	if (pdev)
> +		return -EBUSY;
> +
> +	pdev = attest_pdev;
> +
> +	miscdev.name = DRIVER_NAME;
> +	miscdev.minor = MISC_DYNAMIC_MINOR;
> +	miscdev.fops = &tdx_attest_fops;
> +	miscdev.parent = dev;
> +
> +	ret = misc_register(&miscdev);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		goto failed;

Why just not return error here? There is nothing to cleanup

> +	}
> +
> +	pr_debug("module initialization success\n");
> +
> +	return 0;
> +
> +failed:
> +	misc_deregister(&miscdev);

The only way to get here is if misc_register fails, so we don't need
this call here.

> +
> +	pr_debug("module initialization failed\n");
> +
> +	return ret;
> +}
> +
> +static int tdx_attest_remove(struct platform_device *attest_pdev)
> +{
> +	misc_deregister(&miscdev);
> +	pr_debug("module is successfully removed\n");
> +	return 0;
> +}
> +
> +static struct platform_driver tdx_attest_driver = {
> +	.probe		= tdx_attest_probe,
> +	.remove		= tdx_attest_remove,
> +	.driver		= {
> +		.name	= DRIVER_NAME,
> +	},
> +};
> +
> +static int __init tdx_attest_init(void)
> +{
> +	int ret;
> +
> +	/* Make sure we are in a valid TDX platform */
> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EIO;
> +
> +	ret = platform_driver_register(&tdx_attest_driver);
> +	if (ret) {
> +		pr_err("failed to register driver, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);

pdev is assigned here and in the probe function. Is it correct?

> +	if (IS_ERR(pdev)) {
> +		ret = PTR_ERR(pdev);
> +		pr_err("failed to allocate device, err=%d\n", ret);
> +		platform_driver_unregister(&tdx_attest_driver);
> +		return ret;
> +	}
> +

Re: [PATCH v4 2/3] x86/tdx: Add TDX Guest event notify interrupt support

[Wander Lairson Costa]

On Fri, Apr 22, 2022 at 04:34:17PM -0700, Kuppuswamy Sathyanarayanan wrote:

[snip]

>  
> +/*
> + * 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)
> +{
> +	u64 ret;
> +
> +	/* 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>".
> +	 */
> +	ret = _tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, vector, 0, 0, 0);
> +
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Just "return _tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, vector, 0, 0, 0);" does the job, doesn't it?
And we can remove the declaration of ret.

[snip]

>  
> +#ifdef CONFIG_INTEL_TDX_GUEST

Just for consistency, can we change to "#if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)"?

> +DECLARE_IDTENTRY_SYSVEC(TDX_GUEST_EVENT_NOTIFY_VECTOR,	sysvec_tdx_event_notify);
> +#endif
> +

[snip]

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi,

On 4/28/22 10:45 AM, Wander Lairson Costa wrote:
> On Fri, Apr 22, 2022 at 04:34:16PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> [snip]
> 
>> +static long tdx_get_tdreport(void __user *argp)
>> +{
>> +	void *report_buf = NULL, *tdreport_buf = NULL;
>> +	long ret = 0, err;
>> +
>> +	/* Allocate space for report data */
>> +	report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
>> +	if (!report_buf)
>> +		return -ENOMEM;
>> +
>> +	/*
>> +	 * Allocate space for TDREPORT buffer (1024-byte aligned).
>> +	 * Full page alignment is more than enough.
>> +	 */
>> +	tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
> 
> Maybe we should add BUILD_BUG_ON(TDX_TDREPORT_LEN > PAGE_SIZE)

Currently, it is a constant value < PAGE_SIZE. But I can add the
BUILD_BUG_ON check for it.

> 
>> +	if (!tdreport_buf) {
>> +		ret = -ENOMEM;
>> +		goto tdreport_failed;
>> +	}
>> +
>> +	/* Copy report data to kernel buffer */
>> +	if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) {
>> +		ret = -EFAULT;
>> +		goto tdreport_failed;
>> +	}
>> +
>> +	/* Generate TDREPORT using report data in report_buf */
>> +	err = tdx_mcall_tdreport(tdreport_buf, report_buf);
>> +	if (err) {
>> +		/* If failed, pass TDCALL error code back to user */
>> +		ret = put_user(err, (long __user *)argp);
> 
> The assigment to ret is useless here

Yes, noted it already. I will remove it in next version.

> 
>> +		ret = -EIO;
>> +		goto tdreport_failed;
>> +	}
>> +
>> +	/* Copy TDREPORT data back to user buffer */
>> +	if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN))
>> +		ret = -EFAULT;
>> +
>> +tdreport_failed:
>> +	kfree(report_buf);
>> +	if (tdreport_buf)
>> +		free_pages((unsigned long)tdreport_buf, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>> +			     unsigned long arg)
>> +{
>> +	void __user *argp = (void __user *)arg;
>> +	long ret = 0;
>> +
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_TDREPORT:
>> +		ret = tdx_get_tdreport(argp);
>> +		break;
>> +	default:
>> +		pr_err("cmd %d not supported\n", cmd);
> 
> Shouldn't we add "ret = -EINVAL" here?

Yes. I have noted it already, I will fix this in next version.

> 
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations tdx_attest_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.unlocked_ioctl	= tdx_attest_ioctl,
>> +	.llseek		= no_llseek,
>> +};
>> +
>> +static int tdx_attest_probe(struct platform_device *attest_pdev)
>> +{
>> +	struct device *dev = &attest_pdev->dev;
>> +	long ret = 0;
>> +
>> +	/* Only single device is allowed */
>> +	if (pdev)
>> +		return -EBUSY;
>> +
>> +	pdev = attest_pdev;
>> +
>> +	miscdev.name = DRIVER_NAME;
>> +	miscdev.minor = MISC_DYNAMIC_MINOR;
>> +	miscdev.fops = &tdx_attest_fops;
>> +	miscdev.parent = dev;
>> +
>> +	ret = misc_register(&miscdev);
>> +	if (ret) {
>> +		pr_err("misc device registration failed\n");
>> +		goto failed;
> 
> Why just not return error here? There is nothing to cleanup

Agree. It came along with patch split I did. I will remove it
in next version.

> 
>> +	}
>> +
>> +	pr_debug("module initialization success\n");
>> +
>> +	return 0;
>> +
>> +failed:
>> +	misc_deregister(&miscdev);
> 
> The only way to get here is if misc_register fails, so we don't need
> this call here.

Yes. It is not required. I will remove it.

> 
>> +
>> +	pr_debug("module initialization failed\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int tdx_attest_remove(struct platform_device *attest_pdev)
>> +{
>> +	misc_deregister(&miscdev);
>> +	pr_debug("module is successfully removed\n");
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver tdx_attest_driver = {
>> +	.probe		= tdx_attest_probe,
>> +	.remove		= tdx_attest_remove,
>> +	.driver		= {
>> +		.name	= DRIVER_NAME,
>> +	},
>> +};
>> +
>> +static int __init tdx_attest_init(void)
>> +{
>> +	int ret;
>> +
>> +	/* Make sure we are in a valid TDX platform */
>> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EIO;
>> +
>> +	ret = platform_driver_register(&tdx_attest_driver);
>> +	if (ret) {
>> +		pr_err("failed to register driver, err=%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> 
> pdev is assigned here and in the probe function. Is it correct?

platform_device_register_simple() seem to trigger probe before it
returns. So assigning it in probe is correct. Here it is redundant (
but not harmful)

Anyway this change will go way in next version when I change the driver
to be a pure "misc driver" and remove the "platform driver" support.

> 
>> +	if (IS_ERR(pdev)) {
>> +		ret = PTR_ERR(pdev);
>> +		pr_err("failed to allocate device, err=%d\n", ret);
>> +		platform_driver_unregister(&tdx_attest_driver);
>> +		return ret;
>> +	}
>> +
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 2/3] x86/tdx: Add TDX Guest event notify interrupt support

[Sathyanarayanan Kuppuswamy]

On 4/28/22 10:50 AM, Wander Lairson Costa wrote:
> On Fri, Apr 22, 2022 at 04:34:17PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> [snip]
> 
>>   
>> +/*
>> + * 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)
>> +{
>> +	u64 ret;
>> +
>> +	/* 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>".
>> +	 */
>> +	ret = _tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, vector, 0, 0, 0);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> Just "return _tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, vector, 0, 0, 0);" does the job, doesn't it?
> And we can remove the declaration of ret.

Makes sense. I will remove it.

> 
> [snip]
> 
>>   
>> +#ifdef CONFIG_INTEL_TDX_GUEST
> 
> Just for consistency, can we change to "#if IS_ENABLED(CONFIG_INTEL_TDX_GUEST)"?

Ok. I will change it in next version.

> 
>> +DECLARE_IDTENTRY_SYSVEC(TDX_GUEST_EVENT_NOTIFY_VECTOR,	sysvec_tdx_event_notify);
>> +#endif
>> +
> 
> [snip]
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 3/3] x86/tdx: Add Quote generation support

[Wander Lairson Costa]

On Fri, Apr 22, 2022 at 04:34:18PM -0700, Kuppuswamy Sathyanarayanan wrote:

[snip]

> +static long tdx_get_tdquote(void __user *argp)
> +{
> +	struct tdx_quote_hdr *quote_hdr;
> +	struct tdx_quote_req quote_req;
> +	void *quote_buf = NULL;
> +	dma_addr_t handle;
> +	long ret = 0, err;
> +	u64 quote_buf_len;
> +
> +	mutex_lock(&quote_lock);
> +
> +	reinit_completion(&req_compl);
> +
> +	/* Copy Quote request struct from user buffer */
> +	if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
> +		return -EFAULT;
> +
> +	/* Make sure the length & timeout is valid */
> +	if (quote_req.len <= 0 || quote_req.timeout <= 0)

len and timeout are unsigned values, so they will never be negative.

> +		return -EINVAL;
> +
> +	/* Align with page size to meet 4K alignment */
> +	quote_buf_len = PAGE_ALIGN(quote_req.len);
> +
> +	/*
> +	 * Allocate DMA buffer to get TDQUOTE data from the VMM.
> +	 * dma_alloc_coherent() API internally marks allocated
> +	 * memory as shared with VMM. So explicit shared mapping is
> +	 * not required.
> +	 */
> +	quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
> +					GFP_KERNEL | __GFP_ZERO);
> +	if (!quote_buf) {
> +		ret = -ENOMEM;
> +		goto quote_failed;
> +	}
> +
> +	/* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
> +	if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
> +		ret = -EFAULT;
> +		goto quote_failed;
> +	}
> +
> +	/* Submit GetQuote Request */
> +	err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
> +	if (err) {
> +		/* if failed, copy hypercall error code to user buffer */
> +		ret = put_user(err, (long __user *)argp);

The assigment to ret is unused.

> +		ret = -EIO;
> +		goto quote_failed;
> +	}
> +
> +	/* Wait for attestation completion */
> +	ret = wait_for_completion_interruptible_timeout(
> +			&req_compl,
> +			msecs_to_jiffies(quote_req.timeout));
> +	if (ret <= 0) {
> +		ret = -EIO;
> +		goto quote_failed;
> +	}
> +
> +	/* Copy generated Quote data back to user buffer */
> +	if (copy_to_user((void __user *)quote_req.buf, quote_buf, quote_buf_len)) {

Shouldn't we use quote_req.len instead of quote_buf_len here?

> +		ret = -EFAULT;
> +		goto quote_failed;
> +	}
> +
> +	quote_hdr = (struct tdx_quote_hdr *)quote_buf;
> +
> +	/* Make sure quote generation is successful */
> +	if (!quote_hdr->status)
> +		ret = 0;
> +	else
> +		ret = -EIO;
> +

Shouldn't copy_to_user be called after checking the status?

> +quote_failed:
> +	if (quote_buf)
> +		dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);
> +
> +	mutex_unlock(&quote_lock);
> +
> +	return ret;
> +}
> +
> +static void attestation_callback_handler(void)
> +{
> +	complete(&req_compl);
> +}
> +
>  static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>  			     unsigned long arg)
>  {
> @@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>  	case TDX_CMD_GET_TDREPORT:
>  		ret = tdx_get_tdreport(argp);
>  		break;
> +	case TDX_CMD_GEN_QUOTE:
> +		ret = tdx_get_tdquote(argp);
> +		break;
>  	default:
>  		pr_err("cmd %d not supported\n", cmd);
>  		break;
> @@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
>  	.llseek		= no_llseek,
>  };
>  
> +/* Helper function to cleanup attestation related allocations */
> +static void _tdx_attest_remove(void)
> +{
> +	misc_deregister(&miscdev);

Won't misc_deregister be called even if misc_register fails?

> +
> +	tdx_remove_ev_notify_handler();
> +}
> +

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Dave Hansen]

On 4/28/22 10:56, Sathyanarayanan Kuppuswamy wrote:
> On 4/28/22 10:45 AM, Wander Lairson Costa wrote:
>> On Fri, Apr 22, 2022 at 04:34:16PM -0700, Kuppuswamy Sathyanarayanan
>> wrote:
>>> +static long tdx_get_tdreport(void __user *argp)
>>> +{
>>> +    void *report_buf = NULL, *tdreport_buf = NULL;
>>> +    long ret = 0, err;
>>> +
>>> +    /* Allocate space for report data */
>>> +    report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
>>> +    if (!report_buf)
>>> +        return -ENOMEM;
>>> +
>>> +    /*
>>> +     * Allocate space for TDREPORT buffer (1024-byte aligned).
>>> +     * Full page alignment is more than enough.
>>> +     */
>>> +    tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
>>
>> Maybe we should add BUILD_BUG_ON(TDX_TDREPORT_LEN > PAGE_SIZE)
> 
> Currently, it is a constant value < PAGE_SIZE. But I can add the
> BUILD_BUG_ON check for it.

That's kinda silly.  If it might ever be bigger than a page, you just do:

	tdreport_buf = alloc_pages();

But, seriously, TDX_TDREPORT_LEN is part of the ABI and can't change.
kmalloc() would work too since TDX_TDREPORT_LEN is a power of 2.

Re: [PATCH v4 3/3] x86/tdx: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

On 4/28/22 10:58 AM, Wander Lairson Costa wrote:
> On Fri, Apr 22, 2022 at 04:34:18PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> [snip]
> 
>> +static long tdx_get_tdquote(void __user *argp)
>> +{
>> +	struct tdx_quote_hdr *quote_hdr;
>> +	struct tdx_quote_req quote_req;
>> +	void *quote_buf = NULL;
>> +	dma_addr_t handle;
>> +	long ret = 0, err;
>> +	u64 quote_buf_len;
>> +
>> +	mutex_lock(&quote_lock);
>> +
>> +	reinit_completion(&req_compl);
>> +
>> +	/* Copy Quote request struct from user buffer */
>> +	if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
>> +		return -EFAULT;
>> +
>> +	/* Make sure the length & timeout is valid */
>> +	if (quote_req.len <= 0 || quote_req.timeout <= 0)
> 
> len and timeout are unsigned values, so they will never be negative.

Yes. I will change it to non-zero check.

> 
>> +		return -EINVAL;
>> +
>> +	/* Align with page size to meet 4K alignment */
>> +	quote_buf_len = PAGE_ALIGN(quote_req.len);
>> +
>> +	/*
>> +	 * Allocate DMA buffer to get TDQUOTE data from the VMM.
>> +	 * dma_alloc_coherent() API internally marks allocated
>> +	 * memory as shared with VMM. So explicit shared mapping is
>> +	 * not required.
>> +	 */
>> +	quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
>> +					GFP_KERNEL | __GFP_ZERO);
>> +	if (!quote_buf) {
>> +		ret = -ENOMEM;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
>> +	if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
>> +		ret = -EFAULT;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Submit GetQuote Request */
>> +	err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
>> +	if (err) {
>> +		/* if failed, copy hypercall error code to user buffer */
>> +		ret = put_user(err, (long __user *)argp);
> 
> The assigment to ret is unused.

Since we are already in error path and setting ret to -EIO I did not
handle the pur_user() error case.

> 
>> +		ret = -EIO;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Wait for attestation completion */
>> +	ret = wait_for_completion_interruptible_timeout(
>> +			&req_compl,
>> +			msecs_to_jiffies(quote_req.timeout));
>> +	if (ret <= 0) {
>> +		ret = -EIO;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Copy generated Quote data back to user buffer */
>> +	if (copy_to_user((void __user *)quote_req.buf, quote_buf, quote_buf_len)) {
> 
> Shouldn't we use quote_req.len instead of quote_buf_len here?

Good catch. I will fix it in next version.

> 
>> +		ret = -EFAULT;
>> +		goto quote_failed;
>> +	}
>> +
>> +	quote_hdr = (struct tdx_quote_hdr *)quote_buf;
>> +
>> +	/* Make sure quote generation is successful */
>> +	if (!quote_hdr->status)
>> +		ret = 0;
>> +	else
>> +		ret = -EIO;
>> +
> 
> Shouldn't copy_to_user be called after checking the status?

IMO, since GetQuote TDVMCALL is successful, we can copy the buffer
back to the user. My idea is to let the attestation agent handle the
error in GetQuote header (like IN_FLIGHT scenario).

Maybe I should remove quote_hdr->status check in kernel and let user
agent handle it.

Isaku/Kai, any comments?

> 
>> +quote_failed:
>> +	if (quote_buf)
>> +		dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);
>> +
>> +	mutex_unlock(&quote_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static void attestation_callback_handler(void)
>> +{
>> +	complete(&req_compl);
>> +}
>> +
>>   static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>>   			     unsigned long arg)
>>   {
>> @@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>>   	case TDX_CMD_GET_TDREPORT:
>>   		ret = tdx_get_tdreport(argp);
>>   		break;
>> +	case TDX_CMD_GEN_QUOTE:
>> +		ret = tdx_get_tdquote(argp);
>> +		break;
>>   	default:
>>   		pr_err("cmd %d not supported\n", cmd);
>>   		break;
>> @@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
>>   	.llseek		= no_llseek,
>>   };
>>   
>> +/* Helper function to cleanup attestation related allocations */
>> +static void _tdx_attest_remove(void)
>> +{
>> +	misc_deregister(&miscdev);
> 
> Won't misc_deregister be called even if misc_register fails?

Yes. I will fix this in next version.

> 
>> +
>> +	tdx_remove_ev_notify_handler();
>> +}
>> +
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 4/28/22 11:04 AM, Dave Hansen wrote:
> On 4/28/22 10:56, Sathyanarayanan Kuppuswamy wrote:
>> On 4/28/22 10:45 AM, Wander Lairson Costa wrote:
>>> On Fri, Apr 22, 2022 at 04:34:16PM -0700, Kuppuswamy Sathyanarayanan
>>> wrote:
>>>> +static long tdx_get_tdreport(void __user *argp)
>>>> +{
>>>> +    void *report_buf = NULL, *tdreport_buf = NULL;
>>>> +    long ret = 0, err;
>>>> +
>>>> +    /* Allocate space for report data */
>>>> +    report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
>>>> +    if (!report_buf)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    /*
>>>> +     * Allocate space for TDREPORT buffer (1024-byte aligned).
>>>> +     * Full page alignment is more than enough.
>>>> +     */
>>>> +    tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
>>>
>>> Maybe we should add BUILD_BUG_ON(TDX_TDREPORT_LEN > PAGE_SIZE)
>>
>> Currently, it is a constant value < PAGE_SIZE. But I can add the
>> BUILD_BUG_ON check for it.
> 
> That's kinda silly.  If it might ever be bigger than a page, you just do:
> 
> 	tdreport_buf = alloc_pages();
> 
> But, seriously, TDX_TDREPORT_LEN is part of the ABI and can't change.
> kmalloc() would work too since TDX_TDREPORT_LEN is a power of 2.


Agree. For our current requirement, kmalloc will just work fine. If the
length changes in the future, I can start using alloc_pages.

> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 3/3] x86/tdx: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

Hi Kai,

On 4/26/22 2:47 AM, Kai Huang wrote:
> On Fri, 2022-04-22 at 16:34 -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, the second stage in attestation process is quote
>> generation and signing. GetQuote hypercall can be used by the TD guest
>> to request VMM facilitate the quote generation via a Quoting Enclave
>> (QE). 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 quote is generated. So VMM uses callback interrupt vector
>> configured by SetupEventNotifyInterrupt hypercall to notify the guest
>> about quote generation completion or failure. Upon receiving the
>> completion notification, status can be found in the Quote data header.
>>
>> Add tdx_hcall_get_quote() helper function to implement the GetQuote
>> hypercall and add TDX_CMD_GEN_QUOTE IOCTL support to allow the user
>> agent request for quote generation.
>>
>> When a user agent requests for quote generation, it is expected that
>> the user agent knows about the Quoting Enclave response time, and sets
>> a valid timeout value for the quote generation completion. Timeout
>> support is added to make sure the kernel does not wait for the
>> quote completion indefinitely.
>>
>> Although GHCI specification does not restrict parallel GetQuote
>> requests, since quote generation is not in performance critical path
>> and the frequency of attestation requests are expected to be low, only
>> support serialized quote generation requests. Serialization support is
>> added via a mutex lock (attest_lock). Parallel quote request support
>> can be added once demand arises.
>>
>> 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      | 118 +++++++++++++++++++++++++++++++-
>>   arch/x86/coco/tdx/tdx.c         |  37 ++++++++++
>>   arch/x86/include/asm/tdx.h      |   2 +
>>   arch/x86/include/uapi/asm/tdx.h |  36 ++++++++++
>>   4 files changed, 191 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
>> index b776e81f6c20..d485163d3222 100644
>> --- a/arch/x86/coco/tdx/attest.c
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -32,6 +32,11 @@
>>   static struct platform_device *pdev;
>>   static struct miscdevice miscdev;
>>   
>> +/* Completion object to track GetQuote completion status */
>> +static DECLARE_COMPLETION(req_compl);
>> +/* Mutex to serialize GetQuote requests */
>> +static DEFINE_MUTEX(quote_lock);
>> +
>>   static long tdx_get_tdreport(void __user *argp)
>>   {
>>   	void *report_buf = NULL, *tdreport_buf = NULL;
>> @@ -79,6 +84,95 @@ static long tdx_get_tdreport(void __user *argp)
>>   	return ret;
>>   }
>>   
>> +static long tdx_get_tdquote(void __user *argp)
>> +{
>> +	struct tdx_quote_hdr *quote_hdr;
>> +	struct tdx_quote_req quote_req;
>> +	void *quote_buf = NULL;
>> +	dma_addr_t handle;
>> +	long ret = 0, err;
>> +	u64 quote_buf_len;
>> +
>> +	mutex_lock(&quote_lock);
>> +
>> +	reinit_completion(&req_compl);
>> +
>> +	/* Copy Quote request struct from user buffer */
>> +	if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
>> +		return -EFAULT;
>> +
>> +	/* Make sure the length & timeout is valid */
>> +	if (quote_req.len <= 0 || quote_req.timeout <= 0)
>> +		return -EINVAL;
>> +
>> +	/* Align with page size to meet 4K alignment */
>> +	quote_buf_len = PAGE_ALIGN(quote_req.len);
>> +
>> +	/*
>> +	 * Allocate DMA buffer to get TDQUOTE data from the VMM.
>> +	 * dma_alloc_coherent() API internally marks allocated
>> +	 * memory as shared with VMM. So explicit shared mapping is
>> +	 * not required.
>> +	 */
>> +	quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
>> +					GFP_KERNEL | __GFP_ZERO);
>> +	if (!quote_buf) {
>> +		ret = -ENOMEM;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
>> +	if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
>> +		ret = -EFAULT;
>> +		goto quote_failed;
>> +	}
> 
> So if I read correctly, you are depending on userspace to prepare the
> tdx_quote_hdr, right?

Yes.

> 
> If so, should the driver check the correctness of the hdr? For instance, whether
> hdr.version == 1, etc?

If there are incorrect values in quote header, GetQuote hypercall will
fail automatically. So I don't think we need to check for it explicitly
here and return error.

> 
>> +
>> +	/* Submit GetQuote Request */
>> +	err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
>> +	if (err) {
>> +		/* if failed, copy hypercall error code to user buffer */
>> +		ret = put_user(err, (long __user *)argp);
>> +		ret = -EIO;
>> +		goto quote_failed;
>> +	}
> 
> Similar to getting TDREPORT, is there any particular case that needs to pass
> TDVMCALL error code back to userspace?

I have mainly returned this error code for debug purpose. It does not
have any other use case. As I have mentioned for TDREPORT use case, I
plan to remove it in next version.

> 
>> +
>> +	/* Wait for attestation completion */
>> +	ret = wait_for_completion_interruptible_timeout(
>> +			&req_compl,
>> +			msecs_to_jiffies(quote_req.timeout));
>> +	if (ret <= 0) {
>> +		ret = -EIO;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Copy generated Quote data back to user buffer */
>> +	if (copy_to_user((void __user *)quote_req.buf, quote_buf, quote_buf_len)) {
>> +		ret = -EFAULT;
>> +		goto quote_failed;
>> +	}
>> +
>> +	quote_hdr = (struct tdx_quote_hdr *)quote_buf;
>> +
>> +	/* Make sure quote generation is successful */
>> +	if (!quote_hdr->status)
>> +		ret = 0;
>> +	else
>> +		ret = -EIO;
>> +
>> +quote_failed:
>> +	if (quote_buf)
>> +		dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);
> 
> Will dma_free_coherent() convert the shared buffer back to private (using
> MapGPA)?
> 
> If so, since it's possible to timeout, if the buffer still have IN_FLIGHT flag
> set (VMM is still using it), can we do it?

We depend on user agent to set appropriate timeout specific to QE/QGS
involved. So once it timeout, we assume VMM/QE/QGS will not update it
anymore.

> 
> Isaku, what will happen if guest uses MapGPA to convert a buffer back to private
> while it still has IN_FLIGHT set?

This is not defined in spec. Since we allow user agent select the time
timeout, we assume QE/QGS will not update it after the timeout value.

> 
>> +
>> +	mutex_unlock(&quote_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static void attestation_callback_handler(void)
>> +{
>> +	complete(&req_compl);
>> +}
>> +
>>   static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>>   			     unsigned long arg)
>>   {
>> @@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>>   	case TDX_CMD_GET_TDREPORT:
>>   		ret = tdx_get_tdreport(argp);
>>   		break;
>> +	case TDX_CMD_GEN_QUOTE:
>> +		ret = tdx_get_tdquote(argp);
>> +		break;
>>   	default:
>>   		pr_err("cmd %d not supported\n", cmd);
>>   		break;
>> @@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
>>   	.llseek		= no_llseek,
>>   };
>>   
>> +/* Helper function to cleanup attestation related allocations */
>> +static void _tdx_attest_remove(void)
>> +{
>> +	misc_deregister(&miscdev);
>> +
>> +	tdx_remove_ev_notify_handler();
>> +}
>> +
>>   static int tdx_attest_probe(struct platform_device *attest_pdev)
>>   {
>>   	struct device *dev = &attest_pdev->dev;
>> @@ -114,6 +219,15 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>>   
>>   	pdev = attest_pdev;
>>   
>> +	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>> +	if (ret) {
>> +		pr_err("dma set coherent mask failed\n");
>> +		goto failed;
>> +	}
>> +
>> +	/* 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;
>> @@ -130,7 +244,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>>   	return 0;
>>   
>>   failed:
>> -	misc_deregister(&miscdev);
>> +	_tdx_attest_remove();
>>   
>>   	pr_debug("module initialization failed\n");
>>   
>> @@ -139,7 +253,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>>   
>>   static int tdx_attest_remove(struct platform_device *attest_pdev)
>>   {
>> -	misc_deregister(&miscdev);
>> +	_tdx_attest_remove();
>>   	pr_debug("module is successfully removed\n");
>>   	return 0;
>>   }
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index d0c62b94a1f6..cba22a8d4084 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -25,6 +25,7 @@
>>   
>>   /* TDX hypercall Leaf IDs */
>>   #define TDVMCALL_MAP_GPA		0x10001
>> +#define TDVMCALL_GET_QUOTE		0x10002
>>   #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
>>   
>>   /* MMIO direction */
>> @@ -214,6 +215,42 @@ static long tdx_hcall_set_notify_intr(u8 vector)
>>   	return 0;
>>   }
>>   
>> +/*
>> + * tdx_hcall_get_quote() - Request for TDQUOTE using TDREPORT.
>> + *
>> + * @data        : Address of 4KB aligned GPA memory which contains
>> + *                TDREPORT_STRUCT.
>> + * @len		: Length of the GPA in bytes.
>> + *
>> + * return 0 on success or failure error number.
>> + */
>> +long tdx_hcall_get_quote(void *data, u64 len)
> 
> Rename data/len to something meaningful, i.e. quote_buf, quote_len ?

I plan to move the hypercall implementation to attest.c.

As for names, I am fine with quote_buf and quote_len

> 
>> +{
>> +	u64 ret;
>> +
>> +	/*
>> +	 * Use confidential guest TDX check to ensure this API is only
>> +	 * used by TDX guest platforms.
>> +	 */
>> +	if (!data || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EINVAL;
> 
> The same comment to tdx_mcall_get_tdreport().  You can have a single check of
> X86_FEATURE_TDX_GUEST during driver initialization and refuse to initialize the
> driver if it's not.

Yes. Moved it to initcall of attestation driver.

> 
>> +
>> +	/*
>> +	 * Pass the physical address of tdreport data to the VMM
>> +	 * and trigger the TDQUOTE generation. It is not a blocking
> 
> I see there is inconsistency regarding to how to spell TD Quote.  I have seen
> TDQUOTE, TD QUOTE, quote, and Quote.  I guess we can have a unified way for
> this.  How about: Quote, or TD Quote (when you want to highlight TD)?

Ok.

> 
>> +	 * 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>".
>> +	 */
>> +	ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
>> +			     len, 0, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>>   static u64 get_cc_mask(void)
>>   {
>>   	struct tdx_module_output out;
>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>> index 89ed09809c13..90c2a5f6c40c 100644
>> --- a/arch/x86/include/asm/tdx.h
>> +++ b/arch/x86/include/asm/tdx.h
>> @@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));
>>   
>>   void tdx_remove_ev_notify_handler(void);
>>   
>> +long tdx_hcall_get_quote(void *data, u64 len);
>> +
>>   #else
>>   
>>   static inline void tdx_early_init(void) { };
>> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
>> index c21f9d6fe88b..69259b7841a9 100644
>> --- a/arch/x86/include/uapi/asm/tdx.h
>> +++ b/arch/x86/include/uapi/asm/tdx.h
>> @@ -20,4 +20,40 @@
>>    */
>>   #define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
>>   
>> +/*
>> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
> 
> Replace "TD QUOTE" to some consistent name.

Ok.

> 
>> + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
>> + * buffer of quote size.
>>
> 
> This is not correct.  The data userspace put into the buffer is transparent to
> driver.  It's between userspace attestation agent and QE/QGS.  In fact, Intel's
> implementation has an additional header besides TDREPORT, and the whole data is
> encoded in proto2 format.

Noted. I will fix the description in next version.

> 
>> Once IOCTL is successful quote data is copied back to
>> + * the user buffer. On failure, TDCALL error code is copied back to the user
>> + * buffer.
> 
> The output data may be more than the Quote, the same as above.
> 
>> + */
>> +#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
>> +
>> +struct tdx_quote_req {
>> +	/* Buffer address to store Quote data */
>> +	__u64 buf;
>> +	/* Length of the Quote buffer */
>> +	__u64 len;
>> +	/* Quote generation timeout value in ms */
>> +	__u32 timeout;
>> +};
>> +
>> +/*
>> + * 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;
>> +};
> 
> Needs to be '__u64 data[0]'.  The first 8B data isn't header.

Ok.

> 
> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v4 3/3] x86/tdx: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

Hi,

On 4/26/22 11:14 PM, Isaku Yamahata wrote:
> On Fri, Apr 22, 2022 at 04:34:18PM -0700,
> Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> 
>> In TDX guest, the second stage in attestation process is quote
>> generation and signing. GetQuote hypercall can be used by the TD guest
>> to request VMM facilitate the quote generation via a Quoting Enclave
>> (QE). 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 quote is generated. So VMM uses callback interrupt vector
>> configured by SetupEventNotifyInterrupt hypercall to notify the guest
>> about quote generation completion or failure. Upon receiving the
>> completion notification, status can be found in the Quote data header.
>>
>> Add tdx_hcall_get_quote() helper function to implement the GetQuote
>> hypercall and add TDX_CMD_GEN_QUOTE IOCTL support to allow the user
>> agent request for quote generation.
>>
>> When a user agent requests for quote generation, it is expected that
>> the user agent knows about the Quoting Enclave response time, and sets
>> a valid timeout value for the quote generation completion. Timeout
>> support is added to make sure the kernel does not wait for the
>> quote completion indefinitely.
>>
>> Although GHCI specification does not restrict parallel GetQuote
>> requests, since quote generation is not in performance critical path
>> and the frequency of attestation requests are expected to be low, only
>> support serialized quote generation requests. Serialization support is
>> added via a mutex lock (attest_lock). Parallel quote request support
>> can be added once demand arises.
>>
>> 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      | 118 +++++++++++++++++++++++++++++++-
>>   arch/x86/coco/tdx/tdx.c         |  37 ++++++++++
>>   arch/x86/include/asm/tdx.h      |   2 +
>>   arch/x86/include/uapi/asm/tdx.h |  36 ++++++++++
>>   4 files changed, 191 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
>> index b776e81f6c20..d485163d3222 100644
>> --- a/arch/x86/coco/tdx/attest.c
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -32,6 +32,11 @@
>>   static struct platform_device *pdev;
>>   static struct miscdevice miscdev;
>>   
>> +/* Completion object to track GetQuote completion status */
>> +static DECLARE_COMPLETION(req_compl);
>> +/* Mutex to serialize GetQuote requests */
>> +static DEFINE_MUTEX(quote_lock);
>> +
>>   static long tdx_get_tdreport(void __user *argp)
>>   {
>>   	void *report_buf = NULL, *tdreport_buf = NULL;
>> @@ -79,6 +84,95 @@ static long tdx_get_tdreport(void __user *argp)
>>   	return ret;
>>   }
>>   
>> +static long tdx_get_tdquote(void __user *argp)
>> +{
>> +	struct tdx_quote_hdr *quote_hdr;
>> +	struct tdx_quote_req quote_req;
>> +	void *quote_buf = NULL;
>> +	dma_addr_t handle;
>> +	long ret = 0, err;
>> +	u64 quote_buf_len;
>> +
>> +	mutex_lock(&quote_lock);
>> +
>> +	reinit_completion(&req_compl);
>> +
>> +	/* Copy Quote request struct from user buffer */
>> +	if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
>> +		return -EFAULT;
>> +
>> +	/* Make sure the length & timeout is valid */
>> +	if (quote_req.len <= 0 || quote_req.timeout <= 0)
>> +		return -EINVAL;
>> +
>> +	/* Align with page size to meet 4K alignment */
>> +	quote_buf_len = PAGE_ALIGN(quote_req.len);
>> +
>> +	/*
>> +	 * Allocate DMA buffer to get TDQUOTE data from the VMM.
>> +	 * dma_alloc_coherent() API internally marks allocated
>> +	 * memory as shared with VMM. So explicit shared mapping is
>> +	 * not required.
>> +	 */
>> +	quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
>> +					GFP_KERNEL | __GFP_ZERO);
>> +	if (!quote_buf) {
>> +		ret = -ENOMEM;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
>> +	if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
>> +		ret = -EFAULT;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Submit GetQuote Request */
>> +	err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
>> +	if (err) {
>> +		/* if failed, copy hypercall error code to user buffer */
>> +		ret = put_user(err, (long __user *)argp);
> 
> ret is ignored.  Do you want to return TDX status code to user?
> Does just -EIO suffice?
> (If you really want, the TDX status code should be defined as uapi.)

I will remove it in next version.

> 
> 
>> +		ret = -EIO;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Wait for attestation completion */
>> +	ret = wait_for_completion_interruptible_timeout(
>> +			&req_compl,
>> +			msecs_to_jiffies(quote_req.timeout));
> 
> If you want to support timeout, you need to handle in-flight case below.

Regarding IN_FLIGHT case, I will let user agent handle it. I am going to
change this code to just copy the quote data once hypercall is
successful.


> 
> 
>> +		ret = 0;
>> +	else
>> +		ret = -EIO;
>> +
>> +quote_failed:
>> +	if (quote_buf)
>> +		dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);
> 
> quote_buf can be still owned by VMM because timeout is used above.
> Even if interrupt is arrived,  quote_hdr->status can still be in-flight.

Since timeout behavior is not clearly defined in the spec, I let the
user agent configure the appropriate timeout value. Once it timesout,
I assume QE/QGS/VMM will no longer respond or use the buffer. I have
planned to add the following help in struct tdx_quote_hdr.timeout.

@timeout: Time to wait for VMM to respond back to GetQuote request.
This value is dependent on response time of the Quoting Enclave
(QE) or Quote generation service (QGS) involved. It is expected
the user agent is aware of it and sets the appropriate value.

Agree ?

> 
> 
>> +
>> +	mutex_unlock(&quote_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static void attestation_callback_handler(void)
>> +{
>> +	complete(&req_compl);
>> +}
>> +
>>   static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>>   			     unsigned long arg)
>>   {
>> @@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>>   	case TDX_CMD_GET_TDREPORT:
>>   		ret = tdx_get_tdreport(argp);
>>   		break;
>> +	case TDX_CMD_GEN_QUOTE:
>> +		ret = tdx_get_tdquote(argp);
>> +		break;
>>   	default:
>>   		pr_err("cmd %d not supported\n", cmd);
>>   		break;
>> @@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
>>   	.llseek		= no_llseek,
>>   };
>>   
>> +/* Helper function to cleanup attestation related allocations */
>> +static void _tdx_attest_remove(void)
>> +{
>> +	misc_deregister(&miscdev);
>> +
>> +	tdx_remove_ev_notify_handler();
>> +}
>> +
>>   static int tdx_attest_probe(struct platform_device *attest_pdev)
>>   {
>>   	struct device *dev = &attest_pdev->dev;
>> @@ -114,6 +219,15 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>>   
>>   	pdev = attest_pdev;
>>   
>> +	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>> +	if (ret) {
>> +		pr_err("dma set coherent mask failed\n");
>> +		goto failed;
>> +	}
>> +
>> +	/* 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;
>> @@ -130,7 +244,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>>   	return 0;
>>   
>>   failed:
>> -	misc_deregister(&miscdev);
>> +	_tdx_attest_remove();
>>   
>>   	pr_debug("module initialization failed\n");
>>   
>> @@ -139,7 +253,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
>>   
>>   static int tdx_attest_remove(struct platform_device *attest_pdev)
>>   {
>> -	misc_deregister(&miscdev);
>> +	_tdx_attest_remove();
>>   	pr_debug("module is successfully removed\n");
>>   	return 0;
>>   }
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index d0c62b94a1f6..cba22a8d4084 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -25,6 +25,7 @@
>>   
>>   /* TDX hypercall Leaf IDs */
>>   #define TDVMCALL_MAP_GPA		0x10001
>> +#define TDVMCALL_GET_QUOTE		0x10002
>>   #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
>>   
>>   /* MMIO direction */
>> @@ -214,6 +215,42 @@ static long tdx_hcall_set_notify_intr(u8 vector)
>>   	return 0;
>>   }
>>   
>> +/*
>> + * tdx_hcall_get_quote() - Request for TDQUOTE using TDREPORT.
>> + *
>> + * @data        : Address of 4KB aligned GPA memory which contains
>> + *                TDREPORT_STRUCT.
>> + * @len		: Length of the GPA in bytes.
>> + *
>> + * return 0 on success or failure error number.
>> + */
>> +long tdx_hcall_get_quote(void *data, u64 len)
>> +{
>> +	u64 ret;
>> +
>> +	/*
>> +	 * Use confidential guest TDX check to ensure this API is only
>> +	 * used by TDX guest platforms.
>> +	 */
>> +	if (!data || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EINVAL;
> 
> Isn't X86_FEATURE_TDX_GUEST checked on module loading time?
> 
>> +
>> +	/*
>> +	 * Pass the physical address of tdreport data to the VMM
>> +	 * and trigger the TDQUOTE 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>".
>> +	 */
>> +	ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
>> +			     len, 0, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>>   static u64 get_cc_mask(void)
>>   {
>>   	struct tdx_module_output out;
>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>> index 89ed09809c13..90c2a5f6c40c 100644
>> --- a/arch/x86/include/asm/tdx.h
>> +++ b/arch/x86/include/asm/tdx.h
>> @@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));
>>   
>>   void tdx_remove_ev_notify_handler(void);
>>   
>> +long tdx_hcall_get_quote(void *data, u64 len);
>> +
>>   #else
>>   
>>   static inline void tdx_early_init(void) { };
>> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
>> index c21f9d6fe88b..69259b7841a9 100644
>> --- a/arch/x86/include/uapi/asm/tdx.h
>> +++ b/arch/x86/include/uapi/asm/tdx.h
>> @@ -20,4 +20,40 @@
>>    */
>>   #define TDX_CMD_GET_TDREPORT		_IOWR('T', 0x01, __u64)
>>   
>> +/*
>> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
>> + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
>> + * buffer of quote size. Once IOCTL is successful quote data is copied back to
>> + * the user buffer. On failure, TDCALL error code is copied back to the user
>> + * buffer.
>> + */
>> +#define TDX_CMD_GEN_QUOTE		_IOR('T', 0x02, __u64)
>> +
>> +struct tdx_quote_req {
>> +	/* Buffer address to store Quote data */
>> +	__u64 buf;
>> +	/* Length of the Quote buffer */
>> +	__u64 len;
>> +	/* Quote generation timeout value in ms */
>> +	__u32 timeout;
> 
> What's the point of timeout?

Explained above.

> 
> 
>> +};
>> +
>> +/*
>> + * 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;
> 
> If you export version and status, also define related constants for user space.

Ok.

> 
> 
>> +	/* Length of TDREPORT, filled by TD */
>> +	__u32 in_len;
>> +	/* Length of Quote, filled by VMM */
>> +	__u32 out_len;
>> +	/* Actual Quote data */
>> +	__u64 data;
>> +};
>> +
>>   #endif /* _UAPI_ASM_X86_TDX_H */
>> -- 
>> 2.25.1
>>
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer