linux-coco.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
@ 2023-09-07  2:54 Kuppuswamy Sathyanarayanan
  2023-09-08  0:03 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-07  2:54 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, Dan Williams
  Cc: Kirill A . Shutemov, H . Peter Anvin, Tony Luck,
	Wander Lairson Costa, Erdem Aktas, Dionna Amalie Glaze,
	Qinkun Bao, Guorui Yu, linux-coco, x86, linux-kernel

In TDX guest, the attestation process is used to verify the TDX guest
trustworthiness to other entities before provisioning secrets to the
guest. The First step in the attestation process is TDREPORT
generation, which involves getting the guest measurement data in the
format of TDREPORT, which is further used to validate the authenticity
of the TDX guest. TDREPORT by design is integrity-protected and can
only be verified on the local machine.

To support remote verification of the TDREPORT (in a SGX-based
attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
(QE) to convert it to a remote verifiable Quote. SGX QE by design can
only run outside of the TDX guest (i.e. in a host process or in a
normal VM) and guest can use communication channels like vsock or
TCP/IP to send the TDREPORT to the QE. But for security concerns, the
TDX guest may not support these communication channels. To handle such
cases, TDX defines a GetQuote hypercall which can be used by the guest
to request the host VMM to communicate with the SGX 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>".

Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
Computing Guest platforms to get the measurement data via ConfigFS.
Extend the TSM framework and add support to allow an attestation agent
to get the TDX Quote data (included usage example below).

  report=/sys/kernel/config/tsm/report/report0
  mkdir $report
  dd if=/dev/urandom bs=64 count=1 > $report/inblob
  hexdump -C $report/outblob
  rmdir $report

GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
with TDREPORT data as input, which is further used by the VMM to copy
the TD Quote result after successful Quote generation. To create the
shared buffer, allocate a large enough memory and mark it shared using
set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
for GetQuote requests in the TDX TSM handler.

Although this method reserves a fixed chunk of memory for GetQuote
requests, such one time allocation can help avoid memory fragmentation
related allocation failures later in the uptime of the guest.

Since the Quote generation process is not time-critical or frequently
used, the current version uses a polling model for Quote requests and
it also does not support parallel GetQuote requests.

Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

Hi All,

The previous version of this patch series [1] added support for TDX
Guest Quote generation via an IOCTL interface. Since we have multiple
vendors implementing such interface, to avoid ABI proliferation, Dan
proposed using a common ABI for it and submitted the Trusted Secure
module (TSM) report ABI support [2]. This patchset extends the
TSM REPORTS to implement the TDX Quote generation support. Since there
is a change in interface type, I have dropped the previous Acks.

[1] https://lore.kernel.org/lkml/3c57deb0-a311-2aad-c06b-4938e33491b5@linux.intel.com/
[2] https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/

Changes since previous version:
* Used ConfigFS interface instead of IOCTL interface.
* Used polling model for Quote generation and dropped the event notification IRQ support.

 arch/x86/coco/tdx/tdx.c                 |  21 +++
 arch/x86/include/asm/shared/tdx.h       |   1 +
 arch/x86/include/asm/tdx.h              |   2 +
 drivers/virt/coco/tdx-guest/Kconfig     |   1 +
 drivers/virt/coco/tdx-guest/tdx-guest.c | 205 +++++++++++++++++++++++-
 5 files changed, 229 insertions(+), 1 deletion(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 1d6b863c42b0..20414ed82fc5 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
 }
 EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
 
+/**
+ * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
+ *                         hypercall.
+ * @buf: Address of the directly mapped shared kernel buffer which
+ *	 contains TDREPORT data. The same buffer will be used by
+ *	 VMM to store the generated TD Quote output.
+ * @size: size of the tdquote buffer (4KB-aligned).
+ *
+ * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
+ * v1.0 specification for more information on GetQuote hypercall.
+ * It is used in the TDX guest driver module to get the TD Quote.
+ *
+ * Return 0 on success or error code on failure.
+ */
+u64 tdx_hcall_get_quote(u8 *buf, size_t size)
+{
+	/* Since buf is a shared memory, set the shared (decrypted) bits */
+	return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
+}
+EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
+
 static void __noreturn tdx_panic(const char *msg)
 {
 	struct tdx_hypercall_args args = {
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 7513b3bb69b7..9eab19950f39 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -22,6 +22,7 @@
 
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
+#define TDVMCALL_GET_QUOTE		0x10002
 #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
 
 #ifndef __ASSEMBLY__
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 603e6d1e9d4a..ebd1cda4875f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
 
 int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
 
+u64 tdx_hcall_get_quote(u8 *buf, size_t size);
+
 #else
 
 static inline void tdx_early_init(void) { };
diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
index 14246fc2fb02..22dd59e19431 100644
--- a/drivers/virt/coco/tdx-guest/Kconfig
+++ b/drivers/virt/coco/tdx-guest/Kconfig
@@ -1,6 +1,7 @@
 config TDX_GUEST_DRIVER
 	tristate "TDX Guest driver"
 	depends on INTEL_TDX_GUEST
+	select TSM_REPORTS
 	help
 	  The driver provides userspace interface to communicate with
 	  the TDX module to request the TDX guest details like attestation
diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
index 5e44a0fa69bd..135d89a7e418 100644
--- a/drivers/virt/coco/tdx-guest/tdx-guest.c
+++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
@@ -12,12 +12,59 @@
 #include <linux/mod_devicetable.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
+#include <linux/set_memory.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/tsm.h>
 
 #include <uapi/linux/tdx-guest.h>
 
 #include <asm/cpu_device_id.h>
 #include <asm/tdx.h>
 
+/*
+ * Intel's SGX QE implementation generally uses Quote size less
+ * than 8K (2K Quote data + ~5K of ceritificate blob).
+ */
+#define GET_QUOTE_BUF_SIZE		SZ_8K
+
+#define GET_QUOTE_CMD_VER		1
+
+/* TDX GetQuote status codes */
+#define GET_QUOTE_SUCCESS		0
+#define GET_QUOTE_IN_FLIGHT		0xffffffffffffffff
+
+/* struct tdx_quote_buf: Format of Quote request buffer.
+ * @version: Quote format version, filled by TD.
+ * @status: Status code of Quote request, filled by VMM.
+ * @in_len: Length of TDREPORT, filled by TD.
+ * @out_len: Length of Quote data, filled by VMM.
+ * @data: Quote data on output or TDREPORT on input.
+ *
+ * More details of Quote request buffer can be found in TDX
+ * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
+ * section titled "TDG.VP.VMCALL<GetQuote>"
+ */
+struct tdx_quote_buf {
+	u64 version;
+	u64 status;
+	u32 in_len;
+	u32 out_len;
+	u8 data[];
+};
+
+/* Quote data buffer */
+static void *quote_data;
+
+/* Lock to streamline quote requests */
+static DEFINE_MUTEX(quote_lock);
+
+/*
+ * GetQuote request timeout in seconds. Expect that 30 seconds
+ * is enough time for QE to respond to any Quote requests.
+ */
+static u32 getquote_timeout = 30;
+
 static long tdx_get_report0(struct tdx_report_req __user *req)
 {
 	u8 *reportdata, *tdreport;
@@ -53,6 +100,131 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
 	return ret;
 }
 
+static void free_quote_buf(void *buf)
+{
+	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
+	unsigned int count = len >> PAGE_SHIFT;
+
+	set_memory_encrypted((unsigned long)buf, count);
+
+	free_pages_exact(buf, len);
+}
+
+static void *alloc_quote_buf(void)
+{
+	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
+	unsigned int count = len >> PAGE_SHIFT;
+	void *addr;
+	int ret;
+
+	addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
+	if (!addr)
+		return NULL;
+
+	ret = set_memory_decrypted((unsigned long)addr, count);
+	if (ret) {
+		free_pages_exact(addr, len);
+		return NULL;
+	}
+
+	return addr;
+}
+
+/*
+ * wait_for_quote_completion() - Wait for Quote request completion
+ * @quote_buf: Address of Quote buffer.
+ * @timeout: Timeout in seconds to wait for the Quote generation.
+ *
+ * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
+ * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
+ * while VMM processes the GetQuote request, and will change it to success
+ * or error code after processing is complete. So wait till the status
+ * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
+ */
+static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
+{
+	int i = 0;
+
+	/*
+	 * Quote requests usually take a few seconds to complete, so waking up
+	 * once per second to recheck the status is fine for this use case.
+	 */
+	while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
+		ssleep(1);
+
+	return (i == timeout) ? -ETIMEDOUT : 0;
+}
+
+static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
+{
+	struct tdx_quote_buf *quote_buf = quote_data;
+	int ret;
+	u8 *buf;
+	u64 err;
+
+	guard(mutex)(&quote_lock);
+
+	/*
+	 * If the previous request is timedout and Quote buf status is
+	 * still in GET_QUOTE_IN_FLIGHT (owned by VMM), don't permit any
+	 * new request.
+	 */
+	if (quote_buf->status == GET_QUOTE_IN_FLIGHT)
+		return ERR_PTR(-EBUSY);
+
+	if (desc->inblob_len != TDX_REPORTDATA_LEN)
+		return ERR_PTR(-EINVAL);
+
+	/* TDX attestation does not support multiple formats */
+	if (desc->outblob_format != TSM_FORMAT_DEFAULT)
+		return ERR_PTR(-EINVAL);
+
+	u8 *reportdata __free(kfree) = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
+	if (!reportdata)
+		return ERR_PTR(-ENOMEM);
+
+	u8 *tdreport __free(kfree) = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
+	if (!tdreport)
+		return ERR_PTR(-ENOMEM);
+
+	memcpy(reportdata, desc->inblob, desc->inblob_len);
+
+	/* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
+	ret = tdx_mcall_get_report0(reportdata, tdreport);
+	if (ret) {
+		pr_err("GetReport call failed\n");
+		return ERR_PTR(ret);
+	}
+
+	memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
+
+	/* Update Quote buffer header */
+	quote_buf->version = GET_QUOTE_CMD_VER;
+	quote_buf->in_len = TDX_REPORT_LEN;
+
+	memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
+
+	err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
+	if (err) {
+		pr_err("GetQuote hypercall failed, status:%llx\n", err);
+		return ERR_PTR(-EIO);
+	}
+
+	ret = wait_for_quote_completion(quote_buf, getquote_timeout);
+	if (ret) {
+		pr_err("GetQuote request timedout\n");
+		return ERR_PTR(ret);
+	}
+
+	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
+	if (!buf)
+		return ERR_PTR(-ENOMEM);
+
+	*outblob_len = quote_buf->out_len;
+
+	return buf;
+}
+
 static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
 {
@@ -82,17 +254,48 @@ static const struct x86_cpu_id tdx_guest_ids[] = {
 };
 MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
 
+static const struct tsm_ops tdx_tsm_ops = {
+	.name = KBUILD_MODNAME,
+	.report_new = tdx_report_new,
+};
+
 static int __init tdx_guest_init(void)
 {
+	int ret;
+
 	if (!x86_match_cpu(tdx_guest_ids))
 		return -ENODEV;
 
-	return misc_register(&tdx_misc_dev);
+	ret = misc_register(&tdx_misc_dev);
+	if (ret)
+		return ret;
+
+	quote_data = alloc_quote_buf();
+	if (!quote_data) {
+		pr_err("Failed to allocate Quote buffer\n");
+		ret = -ENOMEM;
+		goto free_misc;
+	}
+
+	ret = register_tsm(&tdx_tsm_ops, NULL, NULL);
+	if (ret)
+		goto free_quote;
+
+	return 0;
+
+free_quote:
+	free_quote_buf(quote_data);
+free_misc:
+	misc_deregister(&tdx_misc_dev);
+
+	return ret;
 }
 module_init(tdx_guest_init);
 
 static void __exit tdx_guest_exit(void)
 {
+	unregister_tsm(&tdx_tsm_ops);
+	free_quote_buf(quote_data);
 	misc_deregister(&tdx_misc_dev);
 }
 module_exit(tdx_guest_exit);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-07  2:54 [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Kuppuswamy Sathyanarayanan
@ 2023-09-08  0:03 ` kernel test robot
  2023-09-08  1:30 ` Erdem Aktas
  2023-09-08  4:50 ` Huang, Kai
  2 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2023-09-08  0:03 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, Dan Williams
  Cc: oe-kbuild-all, Kirill A . Shutemov, H . Peter Anvin, Tony Luck,
	Wander Lairson Costa, Erdem Aktas, Dionna Amalie Glaze,
	Qinkun Bao, Guorui Yu, linux-coco, x86, linux-kernel

Hi Kuppuswamy,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/x86/core]
[also build test ERROR on linus/master v6.5 next-20230907]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuppuswamy-Sathyanarayanan/virt-tdx-guest-Add-Quote-generation-support-using-TSM_REPORTS/20230907-105752
base:   tip/x86/core
patch link:    https://lore.kernel.org/r/20230907025405.2310931-1-sathyanarayanan.kuppuswamy%40linux.intel.com
patch subject: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230908/202309080741.ua24EU6S-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230908/202309080741.ua24EU6S-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309080741.ua24EU6S-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/virt/coco/tdx-guest/tdx-guest.c:18:10: fatal error: linux/tsm.h: No such file or directory
      18 | #include <linux/tsm.h>
         |          ^~~~~~~~~~~~~
   compilation terminated.


vim +18 drivers/virt/coco/tdx-guest/tdx-guest.c

  > 18	#include <linux/tsm.h>
    19	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-07  2:54 [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Kuppuswamy Sathyanarayanan
  2023-09-08  0:03 ` kernel test robot
@ 2023-09-08  1:30 ` Erdem Aktas
  2023-09-08  2:29   ` Kuppuswamy Sathyanarayanan
  2023-09-08  4:50 ` Huang, Kai
  2 siblings, 1 reply; 12+ messages in thread
From: Erdem Aktas @ 2023-09-08  1:30 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
	Dan Williams, Kirill A . Shutemov, H . Peter Anvin, Tony Luck,
	Wander Lairson Costa, Dionna Amalie Glaze, Qinkun Bao, Guorui Yu,
	linux-coco, x86, linux-kernel

On Wed, Sep 6, 2023 at 7:54 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> In TDX guest, the attestation process is used to verify the TDX guest
> trustworthiness to other entities before provisioning secrets to the
> guest. The First step in the attestation process is TDREPORT
> generation, which involves getting the guest measurement data in the
> format of TDREPORT, which is further used to validate the authenticity
> of the TDX guest. TDREPORT by design is integrity-protected and can
> only be verified on the local machine.
>
> To support remote verification of the TDREPORT (in a SGX-based
> attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
> (QE) to convert it to a remote verifiable Quote. SGX QE by design can
s/remote/remotely ?
> only run outside of the TDX guest (i.e. in a host process or in a
> normal VM) and guest can use communication channels like vsock or
> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> TDX guest may not support these communication channels. To handle such
> cases, TDX defines a GetQuote hypercall which can be used by the guest
> to request the host VMM to communicate with the SGX 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>".
>
> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> Computing Guest platforms to get the measurement data via ConfigFS.
> Extend the TSM framework and add support to allow an attestation agent
> to get the TDX Quote data (included usage example below).
>
>   report=/sys/kernel/config/tsm/report/report0
>   mkdir $report
>   dd if=/dev/urandom bs=64 count=1 > $report/inblob
>   hexdump -C $report/outblob
>   rmdir $report
>
> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> with TDREPORT data as input, which is further used by the VMM to copy
> the TD Quote result after successful Quote generation. To create the
> shared buffer, allocate a large enough memory and mark it shared using
> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> for GetQuote requests in the TDX TSM handler.
>
> Although this method reserves a fixed chunk of memory for GetQuote
> requests, such one time allocation can help avoid memory fragmentation
> related allocation failures later in the uptime of the guest.
>
> Since the Quote generation process is not time-critical or frequently
> used, the current version uses a polling model for Quote requests and
> it also does not support parallel GetQuote requests.
>
> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>
> Hi All,
>
> The previous version of this patch series [1] added support for TDX
> Guest Quote generation via an IOCTL interface. Since we have multiple
> vendors implementing such interface, to avoid ABI proliferation, Dan
> proposed using a common ABI for it and submitted the Trusted Secure
> module (TSM) report ABI support [2]. This patchset extends the
> TSM REPORTS to implement the TDX Quote generation support. Since there
> is a change in interface type, I have dropped the previous Acks.
>
> [1] https://lore.kernel.org/lkml/3c57deb0-a311-2aad-c06b-4938e33491b5@linux.intel.com/
> [2] https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/
>
> Changes since previous version:
> * Used ConfigFS interface instead of IOCTL interface.
> * Used polling model for Quote generation and dropped the event notification IRQ support.
>
>  arch/x86/coco/tdx/tdx.c                 |  21 +++
>  arch/x86/include/asm/shared/tdx.h       |   1 +
>  arch/x86/include/asm/tdx.h              |   2 +
>  drivers/virt/coco/tdx-guest/Kconfig     |   1 +
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 205 +++++++++++++++++++++++-
>  5 files changed, 229 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 1d6b863c42b0..20414ed82fc5 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>  }
>  EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
>
> +/**
> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
> + *                         hypercall.
> + * @buf: Address of the directly mapped shared kernel buffer which
> + *      contains TDREPORT data. The same buffer will be used by
> + *      VMM to store the generated TD Quote output.
> + * @size: size of the tdquote buffer (4KB-aligned).
> + *
> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
> + * v1.0 specification for more information on GetQuote hypercall.
> + * It is used in the TDX guest driver module to get the TD Quote.
> + *
> + * Return 0 on success or error code on failure.
> + */
> +u64 tdx_hcall_get_quote(u8 *buf, size_t size)
> +{
> +       /* Since buf is a shared memory, set the shared (decrypted) bits */
> +       return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
> +}
> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
> +
>  static void __noreturn tdx_panic(const char *msg)
>  {
>         struct tdx_hypercall_args args = {
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 7513b3bb69b7..9eab19950f39 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -22,6 +22,7 @@
>
>  /* TDX hypercall Leaf IDs */
>  #define TDVMCALL_MAP_GPA               0x10001
> +#define TDVMCALL_GET_QUOTE             0x10002
>  #define TDVMCALL_REPORT_FATAL_ERROR    0x10003
>
>  #ifndef __ASSEMBLY__
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 603e6d1e9d4a..ebd1cda4875f 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
>
>  int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
>
> +u64 tdx_hcall_get_quote(u8 *buf, size_t size);
> +
>  #else
>
>  static inline void tdx_early_init(void) { };
> diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
> index 14246fc2fb02..22dd59e19431 100644
> --- a/drivers/virt/coco/tdx-guest/Kconfig
> +++ b/drivers/virt/coco/tdx-guest/Kconfig
> @@ -1,6 +1,7 @@
>  config TDX_GUEST_DRIVER
>         tristate "TDX Guest driver"
>         depends on INTEL_TDX_GUEST
> +       select TSM_REPORTS
>         help
>           The driver provides userspace interface to communicate with
>           the TDX module to request the TDX guest details like attestation
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 5e44a0fa69bd..135d89a7e418 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -12,12 +12,59 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
> +#include <linux/set_memory.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/tsm.h>
>
>  #include <uapi/linux/tdx-guest.h>
>
>  #include <asm/cpu_device_id.h>
>  #include <asm/tdx.h>
>
> +/*
> + * Intel's SGX QE implementation generally uses Quote size less
> + * than 8K (2K Quote data + ~5K of ceritificate blob).
s/ceritificate/certificate
> + */
> +#define GET_QUOTE_BUF_SIZE             SZ_8K
> +
> +#define GET_QUOTE_CMD_VER              1
> +
> +/* TDX GetQuote status codes */
> +#define GET_QUOTE_SUCCESS              0
> +#define GET_QUOTE_IN_FLIGHT            0xffffffffffffffff
> +
> +/* struct tdx_quote_buf: Format of Quote request buffer.
> + * @version: Quote format version, filled by TD.
> + * @status: Status code of Quote request, filled by VMM.
> + * @in_len: Length of TDREPORT, filled by TD.
> + * @out_len: Length of Quote data, filled by VMM.
> + * @data: Quote data on output or TDREPORT on input.
> + *
> + * More details of Quote request buffer can be found in TDX
> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> + * section titled "TDG.VP.VMCALL<GetQuote>"
> + */
> +struct tdx_quote_buf {
> +       u64 version;
> +       u64 status;
> +       u32 in_len;
> +       u32 out_len;
> +       u8 data[];
> +};
> +
> +/* Quote data buffer */
> +static void *quote_data;
> +
> +/* Lock to streamline quote requests */
> +static DEFINE_MUTEX(quote_lock);
> +
> +/*
> + * GetQuote request timeout in seconds. Expect that 30 seconds
> + * is enough time for QE to respond to any Quote requests.
> + */
> +static u32 getquote_timeout = 30;
> +
>  static long tdx_get_report0(struct tdx_report_req __user *req)
>  {
>         u8 *reportdata, *tdreport;
> @@ -53,6 +100,131 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>         return ret;
>  }
>
> +static void free_quote_buf(void *buf)
> +{
> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> +       unsigned int count = len >> PAGE_SHIFT;
> +
> +       set_memory_encrypted((unsigned long)buf, count);
Why not check the return error? if conversion fails (even though
unlikely), we should at least print an error message.
> +
> +       free_pages_exact(buf, len);
> +}
> +
> +static void *alloc_quote_buf(void)
> +{
> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> +       unsigned int count = len >> PAGE_SHIFT;
> +       void *addr;
> +       int ret;
> +
> +       addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
> +       if (!addr)
> +               return NULL;
> +
> +       ret = set_memory_decrypted((unsigned long)addr, count);
> +       if (ret) {
> +               free_pages_exact(addr, len);
> +               return NULL;
> +       }
> +
> +       return addr;
> +}
> +
> +/*
> + * wait_for_quote_completion() - Wait for Quote request completion
> + * @quote_buf: Address of Quote buffer.
> + * @timeout: Timeout in seconds to wait for the Quote generation.
> + *
> + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
> + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
> + * while VMM processes the GetQuote request, and will change it to success
> + * or error code after processing is complete. So wait till the status
> + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
s/timedout/being timed out?
> + */
> +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
> +{
> +       int i = 0;
> +
> +       /*
> +        * Quote requests usually take a few seconds to complete, so waking up
> +        * once per second to recheck the status is fine for this use case.
> +        */
> +       while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
> +               ssleep(1);
Would not this loop cause soft lock (or even panic) if getquote waits
for 30s? Should we not yield?
> +
> +       return (i == timeout) ? -ETIMEDOUT : 0;
> +}
> +
> +static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
> +{
> +       struct tdx_quote_buf *quote_buf = quote_data;
> +       int ret;
> +       u8 *buf;
> +       u64 err;
> +
> +       guard(mutex)(&quote_lock);
I understand that this does not support parallel getQuote requests but
if the user space for some reason makes multiple requests, each
request will finish until the previous ones are completed in kernel
scope which might cause soft lockups. Should now we return  EBUSY if
the lock is already taken?
> +
> +       /*
> +        * If the previous request is timedout and Quote buf status is
> +        * still in GET_QUOTE_IN_FLIGHT (owned by VMM), don't permit any
> +        * new request.
> +        */
> +       if (quote_buf->status == GET_QUOTE_IN_FLIGHT)
> +               return ERR_PTR(-EBUSY);
> +
> +       if (desc->inblob_len != TDX_REPORTDATA_LEN)
> +               return ERR_PTR(-EINVAL);
> +
> +       /* TDX attestation does not support multiple formats */
> +       if (desc->outblob_format != TSM_FORMAT_DEFAULT)
> +               return ERR_PTR(-EINVAL);
> +
> +       u8 *reportdata __free(kfree) = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
> +       if (!reportdata)
> +               return ERR_PTR(-ENOMEM);
> +
> +       u8 *tdreport __free(kfree) = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
> +       if (!tdreport)
> +               return ERR_PTR(-ENOMEM);
> +
> +       memcpy(reportdata, desc->inblob, desc->inblob_len);
> +
> +       /* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
> +       ret = tdx_mcall_get_report0(reportdata, tdreport);
> +       if (ret) {
> +               pr_err("GetReport call failed\n");
> +               return ERR_PTR(ret);
> +       }
> +
> +       memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
> +
> +       /* Update Quote buffer header */
> +       quote_buf->version = GET_QUOTE_CMD_VER;
> +       quote_buf->in_len = TDX_REPORT_LEN;
> +
> +       memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
> +
> +       err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
> +       if (err) {
> +               pr_err("GetQuote hypercall failed, status:%llx\n", err);
> +               return ERR_PTR(-EIO);
> +       }
> +
Should not we set the quote_bud->status = GET_QUOTE_IN_FLIGHT in somewhere here?
> +       ret = wait_for_quote_completion(quote_buf, getquote_timeout);
> +       if (ret) {
> +               pr_err("GetQuote request timedout\n");
> +               return ERR_PTR(ret);
> +       }
> +
> +       buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
> +       if (!buf)
> +               return ERR_PTR(-ENOMEM);
> +
> +       *outblob_len = quote_buf->out_len;
> +
> +       return buf;
> +}
> +
>  static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>                             unsigned long arg)
>  {
> @@ -82,17 +254,48 @@ static const struct x86_cpu_id tdx_guest_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>
> +static const struct tsm_ops tdx_tsm_ops = {
> +       .name = KBUILD_MODNAME,
> +       .report_new = tdx_report_new,
> +};
> +
>  static int __init tdx_guest_init(void)
>  {
> +       int ret;
> +
>         if (!x86_match_cpu(tdx_guest_ids))
>                 return -ENODEV;
>
> -       return misc_register(&tdx_misc_dev);
> +       ret = misc_register(&tdx_misc_dev);
> +       if (ret)
> +               return ret;
> +
> +       quote_data = alloc_quote_buf();
> +       if (!quote_data) {
> +               pr_err("Failed to allocate Quote buffer\n");
> +               ret = -ENOMEM;
> +               goto free_misc;
> +       }
> +
> +       ret = register_tsm(&tdx_tsm_ops, NULL, NULL);
> +       if (ret)
> +               goto free_quote;
> +
> +       return 0;
> +
> +free_quote:
> +       free_quote_buf(quote_data);
> +free_misc:
> +       misc_deregister(&tdx_misc_dev);
> +
> +       return ret;
>  }
>  module_init(tdx_guest_init);
>
>  static void __exit tdx_guest_exit(void)
>  {
> +       unregister_tsm(&tdx_tsm_ops);
> +       free_quote_buf(quote_data);
>         misc_deregister(&tdx_misc_dev);
>  }
>  module_exit(tdx_guest_exit);
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-08  1:30 ` Erdem Aktas
@ 2023-09-08  2:29   ` Kuppuswamy Sathyanarayanan
  2023-09-08  4:10     ` Dan Williams
  0 siblings, 1 reply; 12+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-08  2:29 UTC (permalink / raw)
  To: Erdem Aktas
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
	Dan Williams, Kirill A . Shutemov, H . Peter Anvin, Tony Luck,
	Wander Lairson Costa, Dionna Amalie Glaze, Qinkun Bao, Guorui Yu,
	linux-coco, x86, linux-kernel

Hi Erdem,

Thanks for the review.

On 9/7/2023 6:30 PM, Erdem Aktas wrote:
> On Wed, Sep 6, 2023 at 7:54 PM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> In TDX guest, the attestation process is used to verify the TDX guest
>> trustworthiness to other entities before provisioning secrets to the
>> guest. The First step in the attestation process is TDREPORT
>> generation, which involves getting the guest measurement data in the
>> format of TDREPORT, which is further used to validate the authenticity
>> of the TDX guest. TDREPORT by design is integrity-protected and can
>> only be verified on the local machine.
>>
>> To support remote verification of the TDREPORT (in a SGX-based
>> attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
>> (QE) to convert it to a remote verifiable Quote. SGX QE by design can
> s/remote/remotely ?
>> only run outside of the TDX guest (i.e. in a host process or in a
>> normal VM) and guest can use communication channels like vsock or
>> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
>> TDX guest may not support these communication channels. To handle such
>> cases, TDX defines a GetQuote hypercall which can be used by the guest
>> to request the host VMM to communicate with the SGX 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>".
>>
>> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
>> Computing Guest platforms to get the measurement data via ConfigFS.
>> Extend the TSM framework and add support to allow an attestation agent
>> to get the TDX Quote data (included usage example below).
>>
>>   report=/sys/kernel/config/tsm/report/report0
>>   mkdir $report
>>   dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>   hexdump -C $report/outblob
>>   rmdir $report
>>
>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
>> with TDREPORT data as input, which is further used by the VMM to copy
>> the TD Quote result after successful Quote generation. To create the
>> shared buffer, allocate a large enough memory and mark it shared using
>> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
>> for GetQuote requests in the TDX TSM handler.
>>
>> Although this method reserves a fixed chunk of memory for GetQuote
>> requests, such one time allocation can help avoid memory fragmentation
>> related allocation failures later in the uptime of the guest.
>>
>> Since the Quote generation process is not time-critical or frequently
>> used, the current version uses a polling model for Quote requests and
>> it also does not support parallel GetQuote requests.
>>
>> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>
>> Hi All,
>>
>> The previous version of this patch series [1] added support for TDX
>> Guest Quote generation via an IOCTL interface. Since we have multiple
>> vendors implementing such interface, to avoid ABI proliferation, Dan
>> proposed using a common ABI for it and submitted the Trusted Secure
>> module (TSM) report ABI support [2]. This patchset extends the
>> TSM REPORTS to implement the TDX Quote generation support. Since there
>> is a change in interface type, I have dropped the previous Acks.
>>
>> [1] https://lore.kernel.org/lkml/3c57deb0-a311-2aad-c06b-4938e33491b5@linux.intel.com/
>> [2] https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/
>>
>> Changes since previous version:
>> * Used ConfigFS interface instead of IOCTL interface.
>> * Used polling model for Quote generation and dropped the event notification IRQ support.
>>
>>  arch/x86/coco/tdx/tdx.c                 |  21 +++
>>  arch/x86/include/asm/shared/tdx.h       |   1 +
>>  arch/x86/include/asm/tdx.h              |   2 +
>>  drivers/virt/coco/tdx-guest/Kconfig     |   1 +
>>  drivers/virt/coco/tdx-guest/tdx-guest.c | 205 +++++++++++++++++++++++-
>>  5 files changed, 229 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 1d6b863c42b0..20414ed82fc5 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>>  }
>>  EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
>>
>> +/**
>> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
>> + *                         hypercall.
>> + * @buf: Address of the directly mapped shared kernel buffer which
>> + *      contains TDREPORT data. The same buffer will be used by
>> + *      VMM to store the generated TD Quote output.
>> + * @size: size of the tdquote buffer (4KB-aligned).
>> + *
>> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
>> + * v1.0 specification for more information on GetQuote hypercall.
>> + * It is used in the TDX guest driver module to get the TD Quote.
>> + *
>> + * Return 0 on success or error code on failure.
>> + */
>> +u64 tdx_hcall_get_quote(u8 *buf, size_t size)
>> +{
>> +       /* Since buf is a shared memory, set the shared (decrypted) bits */
>> +       return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
>> +
>>  static void __noreturn tdx_panic(const char *msg)
>>  {
>>         struct tdx_hypercall_args args = {
>> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
>> index 7513b3bb69b7..9eab19950f39 100644
>> --- a/arch/x86/include/asm/shared/tdx.h
>> +++ b/arch/x86/include/asm/shared/tdx.h
>> @@ -22,6 +22,7 @@
>>
>>  /* TDX hypercall Leaf IDs */
>>  #define TDVMCALL_MAP_GPA               0x10001
>> +#define TDVMCALL_GET_QUOTE             0x10002
>>  #define TDVMCALL_REPORT_FATAL_ERROR    0x10003
>>
>>  #ifndef __ASSEMBLY__
>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>> index 603e6d1e9d4a..ebd1cda4875f 100644
>> --- a/arch/x86/include/asm/tdx.h
>> +++ b/arch/x86/include/asm/tdx.h
>> @@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
>>
>>  int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
>>
>> +u64 tdx_hcall_get_quote(u8 *buf, size_t size);
>> +
>>  #else
>>
>>  static inline void tdx_early_init(void) { };
>> diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
>> index 14246fc2fb02..22dd59e19431 100644
>> --- a/drivers/virt/coco/tdx-guest/Kconfig
>> +++ b/drivers/virt/coco/tdx-guest/Kconfig
>> @@ -1,6 +1,7 @@
>>  config TDX_GUEST_DRIVER
>>         tristate "TDX Guest driver"
>>         depends on INTEL_TDX_GUEST
>> +       select TSM_REPORTS
>>         help
>>           The driver provides userspace interface to communicate with
>>           the TDX module to request the TDX guest details like attestation
>> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
>> index 5e44a0fa69bd..135d89a7e418 100644
>> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
>> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
>> @@ -12,12 +12,59 @@
>>  #include <linux/mod_devicetable.h>
>>  #include <linux/string.h>
>>  #include <linux/uaccess.h>
>> +#include <linux/set_memory.h>
>> +#include <linux/io.h>
>> +#include <linux/delay.h>
>> +#include <linux/tsm.h>
>>
>>  #include <uapi/linux/tdx-guest.h>
>>
>>  #include <asm/cpu_device_id.h>
>>  #include <asm/tdx.h>
>>
>> +/*
>> + * Intel's SGX QE implementation generally uses Quote size less
>> + * than 8K (2K Quote data + ~5K of ceritificate blob).
> s/ceritificate/certificate
>> + */
>> +#define GET_QUOTE_BUF_SIZE             SZ_8K
>> +
>> +#define GET_QUOTE_CMD_VER              1
>> +
>> +/* TDX GetQuote status codes */
>> +#define GET_QUOTE_SUCCESS              0
>> +#define GET_QUOTE_IN_FLIGHT            0xffffffffffffffff
>> +
>> +/* struct tdx_quote_buf: Format of Quote request buffer.
>> + * @version: Quote format version, filled by TD.
>> + * @status: Status code of Quote request, filled by VMM.
>> + * @in_len: Length of TDREPORT, filled by TD.
>> + * @out_len: Length of Quote data, filled by VMM.
>> + * @data: Quote data on output or TDREPORT on input.
>> + *
>> + * More details of Quote request buffer can be found in TDX
>> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
>> + * section titled "TDG.VP.VMCALL<GetQuote>"
>> + */
>> +struct tdx_quote_buf {
>> +       u64 version;
>> +       u64 status;
>> +       u32 in_len;
>> +       u32 out_len;
>> +       u8 data[];
>> +};
>> +
>> +/* Quote data buffer */
>> +static void *quote_data;
>> +
>> +/* Lock to streamline quote requests */
>> +static DEFINE_MUTEX(quote_lock);
>> +
>> +/*
>> + * GetQuote request timeout in seconds. Expect that 30 seconds
>> + * is enough time for QE to respond to any Quote requests.
>> + */
>> +static u32 getquote_timeout = 30;
>> +
>>  static long tdx_get_report0(struct tdx_report_req __user *req)
>>  {
>>         u8 *reportdata, *tdreport;
>> @@ -53,6 +100,131 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>>         return ret;
>>  }
>>
>> +static void free_quote_buf(void *buf)
>> +{
>> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
>> +       unsigned int count = len >> PAGE_SHIFT;
>> +
>> +       set_memory_encrypted((unsigned long)buf, count);
> Why not check the return error? if conversion fails (even though
> unlikely), we should at least print an error message.

Ok. Since it is unlikely to fail, we can use WARN_ON().

WARN_ON(set_memory_encrypted((unsigned long)buf, count))

>> +
>> +       free_pages_exact(buf, len);
>> +}
>> +
>> +static void *alloc_quote_buf(void)
>> +{
>> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
>> +       unsigned int count = len >> PAGE_SHIFT;
>> +       void *addr;
>> +       int ret;
>> +
>> +       addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
>> +       if (!addr)
>> +               return NULL;
>> +
>> +       ret = set_memory_decrypted((unsigned long)addr, count);
>> +       if (ret) {
>> +               free_pages_exact(addr, len);
>> +               return NULL;
>> +       }
>> +
>> +       return addr;
>> +}
>> +
>> +/*
>> + * wait_for_quote_completion() - Wait for Quote request completion
>> + * @quote_buf: Address of Quote buffer.
>> + * @timeout: Timeout in seconds to wait for the Quote generation.
>> + *
>> + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
>> + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
>> + * while VMM processes the GetQuote request, and will change it to success
>> + * or error code after processing is complete. So wait till the status
>> + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
> s/timedout/being timed out?
>> + */
>> +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
>> +{
>> +       int i = 0;
>> +
>> +       /*
>> +        * Quote requests usually take a few seconds to complete, so waking up
>> +        * once per second to recheck the status is fine for this use case.
>> +        */
>> +       while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
>> +               ssleep(1);
> Would not this loop cause soft lock (or even panic) if getquote waits
> for 30s? Should we not yield?

Since we are sleeping for a second in each cycle (which will relinquish the CPU
to other threads), it should not create a soft lockup.

>> +
>> +       return (i == timeout) ? -ETIMEDOUT : 0;
>> +}
>> +
>> +static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
>> +{
>> +       struct tdx_quote_buf *quote_buf = quote_data;
>> +       int ret;
>> +       u8 *buf;
>> +       u64 err;
>> +
>> +       guard(mutex)(&quote_lock);
> I understand that this does not support parallel getQuote requests but
> if the user space for some reason makes multiple requests, each
> request will finish until the previous ones are completed in kernel

Softlockup will only happen if a thread hogs the CPU for a long time without
relinquishing it. In our case, user processes waiting for the lock will be in
sleep state. So it should not create a lockup issue.

> scope which might cause soft lockups. Should now we return  EBUSY if
> the lock is already taken?

For this use case, in my opinion, it is not required. I think it's only
necessary if the user app is concerned about waiting, which I don't think
is the case in this situation. 

>> +
>> +       /*
>> +        * If the previous request is timedout and Quote buf status is
>> +        * still in GET_QUOTE_IN_FLIGHT (owned by VMM), don't permit any
>> +        * new request.
>> +        */
>> +       if (quote_buf->status == GET_QUOTE_IN_FLIGHT)
>> +               return ERR_PTR(-EBUSY);
>> +
>> +       if (desc->inblob_len != TDX_REPORTDATA_LEN)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       /* TDX attestation does not support multiple formats */
>> +       if (desc->outblob_format != TSM_FORMAT_DEFAULT)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       u8 *reportdata __free(kfree) = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
>> +       if (!reportdata)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       u8 *tdreport __free(kfree) = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
>> +       if (!tdreport)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       memcpy(reportdata, desc->inblob, desc->inblob_len);
>> +
>> +       /* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
>> +       ret = tdx_mcall_get_report0(reportdata, tdreport);
>> +       if (ret) {
>> +               pr_err("GetReport call failed\n");
>> +               return ERR_PTR(ret);
>> +       }
>> +
>> +       memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
>> +
>> +       /* Update Quote buffer header */
>> +       quote_buf->version = GET_QUOTE_CMD_VER;
>> +       quote_buf->in_len = TDX_REPORT_LEN;
>> +
>> +       memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
>> +
>> +       err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
>> +       if (err) {
>> +               pr_err("GetQuote hypercall failed, status:%llx\n", err);
>> +               return ERR_PTR(-EIO);
>> +       }
>> +
> Should not we set the quote_bud->status = GET_QUOTE_IN_FLIGHT in somewhere here?

After the hypercall request, VMM will set it. Guest is not allowed to
modify the status.

>> +       ret = wait_for_quote_completion(quote_buf, getquote_timeout);
>> +       if (ret) {
>> +               pr_err("GetQuote request timedout\n");
>> +               return ERR_PTR(ret);
>> +       }
>> +
>> +       buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
>> +       if (!buf)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       *outblob_len = quote_buf->out_len;
>> +
>> +       return buf;
>> +}
>> +
>>  static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>>                             unsigned long arg)
>>  {
>> @@ -82,17 +254,48 @@ static const struct x86_cpu_id tdx_guest_ids[] = {
>>  };
>>  MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>>
>> +static const struct tsm_ops tdx_tsm_ops = {
>> +       .name = KBUILD_MODNAME,
>> +       .report_new = tdx_report_new,
>> +};
>> +
>>  static int __init tdx_guest_init(void)
>>  {
>> +       int ret;
>> +
>>         if (!x86_match_cpu(tdx_guest_ids))
>>                 return -ENODEV;
>>
>> -       return misc_register(&tdx_misc_dev);
>> +       ret = misc_register(&tdx_misc_dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       quote_data = alloc_quote_buf();
>> +       if (!quote_data) {
>> +               pr_err("Failed to allocate Quote buffer\n");
>> +               ret = -ENOMEM;
>> +               goto free_misc;
>> +       }
>> +
>> +       ret = register_tsm(&tdx_tsm_ops, NULL, NULL);
>> +       if (ret)
>> +               goto free_quote;
>> +
>> +       return 0;
>> +
>> +free_quote:
>> +       free_quote_buf(quote_data);
>> +free_misc:
>> +       misc_deregister(&tdx_misc_dev);
>> +
>> +       return ret;
>>  }
>>  module_init(tdx_guest_init);
>>
>>  static void __exit tdx_guest_exit(void)
>>  {
>> +       unregister_tsm(&tdx_tsm_ops);
>> +       free_quote_buf(quote_data);
>>         misc_deregister(&tdx_misc_dev);
>>  }
>>  module_exit(tdx_guest_exit);
>> --
>> 2.25.1
>>
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-08  2:29   ` Kuppuswamy Sathyanarayanan
@ 2023-09-08  4:10     ` Dan Williams
  2023-09-09  5:24       ` Kuppuswamy Sathyanarayanan
  2023-09-13  3:51       ` Kuppuswamy Sathyanarayanan
  0 siblings, 2 replies; 12+ messages in thread
From: Dan Williams @ 2023-09-08  4:10 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan, Erdem Aktas
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
	Dan Williams, Kirill A . Shutemov, H . Peter Anvin, Tony Luck,
	Wander Lairson Costa, Dionna Amalie Glaze, Qinkun Bao, Guorui Yu,
	linux-coco, x86, linux-kernel

Kuppuswamy Sathyanarayanan wrote:
> Hi Erdem,
> 
> Thanks for the review.
> 
> On 9/7/2023 6:30 PM, Erdem Aktas wrote:
> > On Wed, Sep 6, 2023 at 7:54 PM Kuppuswamy Sathyanarayanan
> > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >>
> >> In TDX guest, the attestation process is used to verify the TDX guest
> >> trustworthiness to other entities before provisioning secrets to the
> >> guest. The First step in the attestation process is TDREPORT
> >> generation, which involves getting the guest measurement data in the
> >> format of TDREPORT, which is further used to validate the authenticity
> >> of the TDX guest. TDREPORT by design is integrity-protected and can
> >> only be verified on the local machine.
> >>
> >> To support remote verification of the TDREPORT (in a SGX-based
> >> attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
> >> (QE) to convert it to a remote verifiable Quote. SGX QE by design can
> > s/remote/remotely ?
> >> only run outside of the TDX guest (i.e. in a host process or in a
> >> normal VM) and guest can use communication channels like vsock or
> >> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> >> TDX guest may not support these communication channels. To handle such
> >> cases, TDX defines a GetQuote hypercall which can be used by the guest
> >> to request the host VMM to communicate with the SGX 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>".
> >>
> >> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> >> Computing Guest platforms to get the measurement data via ConfigFS.
> >> Extend the TSM framework and add support to allow an attestation agent
> >> to get the TDX Quote data (included usage example below).
> >>
> >>   report=/sys/kernel/config/tsm/report/report0
> >>   mkdir $report
> >>   dd if=/dev/urandom bs=64 count=1 > $report/inblob
> >>   hexdump -C $report/outblob
> >>   rmdir $report
> >>
> >> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> >> with TDREPORT data as input, which is further used by the VMM to copy
> >> the TD Quote result after successful Quote generation. To create the
> >> shared buffer, allocate a large enough memory and mark it shared using
> >> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> >> for GetQuote requests in the TDX TSM handler.
> >>
> >> Although this method reserves a fixed chunk of memory for GetQuote
> >> requests, such one time allocation can help avoid memory fragmentation
> >> related allocation failures later in the uptime of the guest.
> >>
> >> Since the Quote generation process is not time-critical or frequently
> >> used, the current version uses a polling model for Quote requests and
> >> it also does not support parallel GetQuote requests.
> >>
> >> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >> ---
> >>
> >> Hi All,
> >>
> >> The previous version of this patch series [1] added support for TDX
> >> Guest Quote generation via an IOCTL interface. Since we have multiple
> >> vendors implementing such interface, to avoid ABI proliferation, Dan
> >> proposed using a common ABI for it and submitted the Trusted Secure
> >> module (TSM) report ABI support [2]. This patchset extends the
> >> TSM REPORTS to implement the TDX Quote generation support. Since there
> >> is a change in interface type, I have dropped the previous Acks.
> >>
> >> [1] https://lore.kernel.org/lkml/3c57deb0-a311-2aad-c06b-4938e33491b5@linux.intel.com/
> >> [2] https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/
> >>
> >> Changes since previous version:
> >> * Used ConfigFS interface instead of IOCTL interface.
> >> * Used polling model for Quote generation and dropped the event notification IRQ support.
> >>
> >>  arch/x86/coco/tdx/tdx.c                 |  21 +++
> >>  arch/x86/include/asm/shared/tdx.h       |   1 +
> >>  arch/x86/include/asm/tdx.h              |   2 +
> >>  drivers/virt/coco/tdx-guest/Kconfig     |   1 +
> >>  drivers/virt/coco/tdx-guest/tdx-guest.c | 205 +++++++++++++++++++++++-
> >>  5 files changed, 229 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> >> index 1d6b863c42b0..20414ed82fc5 100644
> >> --- a/arch/x86/coco/tdx/tdx.c
> >> +++ b/arch/x86/coco/tdx/tdx.c
> >> @@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
> >>  }
> >>  EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
> >>
> >> +/**
> >> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
> >> + *                         hypercall.
> >> + * @buf: Address of the directly mapped shared kernel buffer which
> >> + *      contains TDREPORT data. The same buffer will be used by
> >> + *      VMM to store the generated TD Quote output.
> >> + * @size: size of the tdquote buffer (4KB-aligned).
> >> + *
> >> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
> >> + * v1.0 specification for more information on GetQuote hypercall.
> >> + * It is used in the TDX guest driver module to get the TD Quote.
> >> + *
> >> + * Return 0 on success or error code on failure.
> >> + */
> >> +u64 tdx_hcall_get_quote(u8 *buf, size_t size)
> >> +{
> >> +       /* Since buf is a shared memory, set the shared (decrypted) bits */
> >> +       return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
> >> +}
> >> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
> >> +
> >>  static void __noreturn tdx_panic(const char *msg)
> >>  {
> >>         struct tdx_hypercall_args args = {
> >> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> >> index 7513b3bb69b7..9eab19950f39 100644
> >> --- a/arch/x86/include/asm/shared/tdx.h
> >> +++ b/arch/x86/include/asm/shared/tdx.h
> >> @@ -22,6 +22,7 @@
> >>
> >>  /* TDX hypercall Leaf IDs */
> >>  #define TDVMCALL_MAP_GPA               0x10001
> >> +#define TDVMCALL_GET_QUOTE             0x10002
> >>  #define TDVMCALL_REPORT_FATAL_ERROR    0x10003
> >>
> >>  #ifndef __ASSEMBLY__
> >> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> >> index 603e6d1e9d4a..ebd1cda4875f 100644
> >> --- a/arch/x86/include/asm/tdx.h
> >> +++ b/arch/x86/include/asm/tdx.h
> >> @@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
> >>
> >>  int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
> >>
> >> +u64 tdx_hcall_get_quote(u8 *buf, size_t size);
> >> +
> >>  #else
> >>
> >>  static inline void tdx_early_init(void) { };
> >> diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
> >> index 14246fc2fb02..22dd59e19431 100644
> >> --- a/drivers/virt/coco/tdx-guest/Kconfig
> >> +++ b/drivers/virt/coco/tdx-guest/Kconfig
> >> @@ -1,6 +1,7 @@
> >>  config TDX_GUEST_DRIVER
> >>         tristate "TDX Guest driver"
> >>         depends on INTEL_TDX_GUEST
> >> +       select TSM_REPORTS
> >>         help
> >>           The driver provides userspace interface to communicate with
> >>           the TDX module to request the TDX guest details like attestation
> >> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> >> index 5e44a0fa69bd..135d89a7e418 100644
> >> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> >> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> >> @@ -12,12 +12,59 @@
> >>  #include <linux/mod_devicetable.h>
> >>  #include <linux/string.h>
> >>  #include <linux/uaccess.h>
> >> +#include <linux/set_memory.h>
> >> +#include <linux/io.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/tsm.h>
> >>
> >>  #include <uapi/linux/tdx-guest.h>
> >>
> >>  #include <asm/cpu_device_id.h>
> >>  #include <asm/tdx.h>
> >>
> >> +/*
> >> + * Intel's SGX QE implementation generally uses Quote size less
> >> + * than 8K (2K Quote data + ~5K of ceritificate blob).
> > s/ceritificate/certificate
> >> + */
> >> +#define GET_QUOTE_BUF_SIZE             SZ_8K
> >> +
> >> +#define GET_QUOTE_CMD_VER              1
> >> +
> >> +/* TDX GetQuote status codes */
> >> +#define GET_QUOTE_SUCCESS              0
> >> +#define GET_QUOTE_IN_FLIGHT            0xffffffffffffffff
> >> +
> >> +/* struct tdx_quote_buf: Format of Quote request buffer.
> >> + * @version: Quote format version, filled by TD.
> >> + * @status: Status code of Quote request, filled by VMM.
> >> + * @in_len: Length of TDREPORT, filled by TD.
> >> + * @out_len: Length of Quote data, filled by VMM.
> >> + * @data: Quote data on output or TDREPORT on input.
> >> + *
> >> + * More details of Quote request buffer can be found in TDX
> >> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> >> + * section titled "TDG.VP.VMCALL<GetQuote>"
> >> + */
> >> +struct tdx_quote_buf {
> >> +       u64 version;
> >> +       u64 status;
> >> +       u32 in_len;
> >> +       u32 out_len;
> >> +       u8 data[];
> >> +};
> >> +
> >> +/* Quote data buffer */
> >> +static void *quote_data;
> >> +
> >> +/* Lock to streamline quote requests */
> >> +static DEFINE_MUTEX(quote_lock);
> >> +
> >> +/*
> >> + * GetQuote request timeout in seconds. Expect that 30 seconds
> >> + * is enough time for QE to respond to any Quote requests.
> >> + */
> >> +static u32 getquote_timeout = 30;
> >> +
> >>  static long tdx_get_report0(struct tdx_report_req __user *req)
> >>  {
> >>         u8 *reportdata, *tdreport;
> >> @@ -53,6 +100,131 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
> >>         return ret;
> >>  }
> >>
> >> +static void free_quote_buf(void *buf)
> >> +{
> >> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> >> +       unsigned int count = len >> PAGE_SHIFT;
> >> +
> >> +       set_memory_encrypted((unsigned long)buf, count);
> > Why not check the return error? if conversion fails (even though
> > unlikely), we should at least print an error message.
> 
> Ok. Since it is unlikely to fail, we can use WARN_ON().
> 
> WARN_ON(set_memory_encrypted((unsigned long)buf, count))

No, panic_on_warn turns recoverable errors into fatal errors. Just
pr_err(). I also assume you don't want memory that failed to be set back
to encrypted into the free page pool, so it seems safer to leak the
memory at this point.

> 
> >> +
> >> +       free_pages_exact(buf, len);
> >> +}
> >> +
> >> +static void *alloc_quote_buf(void)
> >> +{
> >> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> >> +       unsigned int count = len >> PAGE_SHIFT;
> >> +       void *addr;
> >> +       int ret;
> >> +
> >> +       addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
> >> +       if (!addr)
> >> +               return NULL;
> >> +
> >> +       ret = set_memory_decrypted((unsigned long)addr, count);
> >> +       if (ret) {
> >> +               free_pages_exact(addr, len);
> >> +               return NULL;
> >> +       }
> >> +
> >> +       return addr;
> >> +}
> >> +
> >> +/*
> >> + * wait_for_quote_completion() - Wait for Quote request completion
> >> + * @quote_buf: Address of Quote buffer.
> >> + * @timeout: Timeout in seconds to wait for the Quote generation.
> >> + *
> >> + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
> >> + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
> >> + * while VMM processes the GetQuote request, and will change it to success
> >> + * or error code after processing is complete. So wait till the status
> >> + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
> > s/timedout/being timed out?
> >> + */
> >> +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
> >> +{
> >> +       int i = 0;
> >> +
> >> +       /*
> >> +        * Quote requests usually take a few seconds to complete, so waking up
> >> +        * once per second to recheck the status is fine for this use case.
> >> +        */
> >> +       while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
> >> +               ssleep(1);
> > Would not this loop cause soft lock (or even panic) if getquote waits
> > for 30s? Should we not yield?
> 
> Since we are sleeping for a second in each cycle (which will relinquish the CPU
> to other threads), it should not create a soft lockup.

No, this would need to release the lock while sleeping or otherwise make
this interruptible to make that happen. In fact it should release the
lock or use mutex_lock_interruptible() and configfs-tsm should switch to
interruptible synchronization.

> 
> >> +
> >> +       return (i == timeout) ? -ETIMEDOUT : 0;
> >> +}
> >> +
> >> +static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
> >> +{
> >> +       struct tdx_quote_buf *quote_buf = quote_data;
> >> +       int ret;
> >> +       u8 *buf;
> >> +       u64 err;
> >> +
> >> +       guard(mutex)(&quote_lock);
> > I understand that this does not support parallel getQuote requests but
> > if the user space for some reason makes multiple requests, each
> > request will finish until the previous ones are completed in kernel
> 
> Softlockup will only happen if a thread hogs the CPU for a long time without
> relinquishing it. In our case, user processes waiting for the lock will be in
> sleep state. So it should not create a lockup issue.

Stuck in uninterruptible sleep is another failure mode of the lockup
detector. The other threads waiting to acquire the mutex will fire being
stuck behind this thread.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-07  2:54 [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Kuppuswamy Sathyanarayanan
  2023-09-08  0:03 ` kernel test robot
  2023-09-08  1:30 ` Erdem Aktas
@ 2023-09-08  4:50 ` Huang, Kai
  2023-09-08 16:19   ` Dan Williams
  2 siblings, 1 reply; 12+ messages in thread
From: Huang, Kai @ 2023-09-08  4:50 UTC (permalink / raw)
  To: sathyanarayanan.kuppuswamy, tglx, mingo, Williams, Dan J, bp,
	dave.hansen
  Cc: Yu, Guorui, qinkun, wander, hpa, linux-kernel, kirill.shutemov,
	Aktas, Erdem, Luck, Tony, linux-coco, dionnaglaze, x86


> 
> Changes since previous version:
> * Used ConfigFS interface instead of IOCTL interface.
> * Used polling model for Quote generation and dropped the event notification IRQ support.

Can you elaborate why the notification IRQ is dropped?

> 
>  arch/x86/coco/tdx/tdx.c                 |  21 +++
>  arch/x86/include/asm/shared/tdx.h       |   1 +
>  arch/x86/include/asm/tdx.h              |   2 +
>  drivers/virt/coco/tdx-guest/Kconfig     |   1 +
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 205 +++++++++++++++++++++++-
>  5 files changed, 229 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 1d6b863c42b0..20414ed82fc5 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>  }
>  EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
>  
> +/**
> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
> + *                         hypercall.
> + * @buf: Address of the directly mapped shared kernel buffer which
> + *	 contains TDREPORT data. The same buffer will be used by
> + *	 VMM to store the generated TD Quote output.

Nit: 

It seems indent can be improved to make the text vertically aligned.

Also, "TDREPORT data" -> "TDREPORT".
 
> + * @size: size of the tdquote buffer (4KB-aligned).
> + *
> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
> + * v1.0 specification for more information on GetQuote hypercall.
> + * It is used in the TDX guest driver module to get the TD Quote.
> + *
> + * Return 0 on success or error code on failure.
> + */
> +u64 tdx_hcall_get_quote(u8 *buf, size_t size)
> +{
> +	/* Since buf is a shared memory, set the shared (decrypted) bits */
> +	return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
> +}
> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
> +
>  static void __noreturn tdx_panic(const char *msg)
>  {
>  	struct tdx_hypercall_args args = {
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 7513b3bb69b7..9eab19950f39 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -22,6 +22,7 @@
>  
>  /* TDX hypercall Leaf IDs */
>  #define TDVMCALL_MAP_GPA		0x10001
> +#define TDVMCALL_GET_QUOTE		0x10002
>  #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
>  
>  #ifndef __ASSEMBLY__
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 603e6d1e9d4a..ebd1cda4875f 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
>  
>  int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
>  
> +u64 tdx_hcall_get_quote(u8 *buf, size_t size);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };
> diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
> index 14246fc2fb02..22dd59e19431 100644
> --- a/drivers/virt/coco/tdx-guest/Kconfig
> +++ b/drivers/virt/coco/tdx-guest/Kconfig
> @@ -1,6 +1,7 @@
>  config TDX_GUEST_DRIVER
>  	tristate "TDX Guest driver"
>  	depends on INTEL_TDX_GUEST
> +	select TSM_REPORTS
>  	help
>  	  The driver provides userspace interface to communicate with
>  	  the TDX module to request the TDX guest details like attestation
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 5e44a0fa69bd..135d89a7e418 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -12,12 +12,59 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
> +#include <linux/set_memory.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/tsm.h>
>  
>  #include <uapi/linux/tdx-guest.h>
>  
>  #include <asm/cpu_device_id.h>
>  #include <asm/tdx.h>
>  
> +/*
> + * Intel's SGX QE implementation generally uses Quote size less
> + * than 8K (2K Quote data + ~5K of ceritificate blob).
> + */
> +#define GET_QUOTE_BUF_SIZE		SZ_8K

SZ_8K is defined in <linux/sizes.h>.  It seems it's not explicitly included. 
It's better to explicitly include it.

Btw, although the size of the certificate blob shouldn't change dramatically,
the Quote can also include the "QE Authentication Data", which can vary a lot
depending on different QE implementation (e.g., containing geography
information, etc).

I wish eventually there's some /sysfs entry to configure the size of Quote
buffer, but I guess it can be done in the future.

> +
> +#define GET_QUOTE_CMD_VER		1
> +
> +/* TDX GetQuote status codes */
> +#define GET_QUOTE_SUCCESS		0
> +#define GET_QUOTE_IN_FLIGHT		0xffffffffffffffff
> +
> +/* struct tdx_quote_buf: Format of Quote request buffer.
> + * @version: Quote format version, filled by TD.
> + * @status: Status code of Quote request, filled by VMM.
> + * @in_len: Length of TDREPORT, filled by TD.
> + * @out_len: Length of Quote data, filled by VMM.
> + * @data: Quote data on output or TDREPORT on input.
> + *
> + * More details of Quote request buffer can be found in TDX
> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> + * section titled "TDG.VP.VMCALL<GetQuote>"
> + */
> +struct tdx_quote_buf {
> +	u64 version;
> +	u64 status;
> +	u32 in_len;
> +	u32 out_len;
> +	u8 data[];

It seems DECLARE_FLEX_ARRAY() is preferred.

> +};
> +
> +/* Quote data buffer */
> +static void *quote_data;
> +
> +/* Lock to streamline quote requests */
> +static DEFINE_MUTEX(quote_lock);
> +
> +/*
> + * GetQuote request timeout in seconds. Expect that 30 seconds
> + * is enough time for QE to respond to any Quote requests.
> + */
> +static u32 getquote_timeout = 30;
> +
>  static long tdx_get_report0(struct tdx_report_req __user *req)
>  {
>  	u8 *reportdata, *tdreport;
> @@ -53,6 +100,131 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>  	return ret;
>  }
>  
> +static void free_quote_buf(void *buf)
> +{
> +	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> +	unsigned int count = len >> PAGE_SHIFT;
> +
> +	set_memory_encrypted((unsigned long)buf, count);

WARN(), or give an error message on failure, and give up freeing the buffer?

> +
> +	free_pages_exact(buf, len);
> +}
> +
> +static void *alloc_quote_buf(void)
> +{
> +	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> +	unsigned int count = len >> PAGE_SHIFT;
> +	void *addr;
> +	int ret;
> +
> +	addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
> +	if (!addr)
> +		return NULL;
> +
> +	ret = set_memory_decrypted((unsigned long)addr, count);
> +	if (ret) {
> +		free_pages_exact(addr, len);
> +		return NULL;
> +	}

The 'ret' can be avoided if you do:

	if (set_memory_decrypted(...))

> +
> +	return addr;
> +}
> +
> +/*
> + * wait_for_quote_completion() - Wait for Quote request completion
> + * @quote_buf: Address of Quote buffer.
> + * @timeout: Timeout in seconds to wait for the Quote generation.
> + *
> + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
> + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
> + * while VMM processes the GetQuote request, and will change it to success
> + * or error code after processing is complete. So wait till the status
> + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
> + */
> +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
> +{
> +	int i = 0;
> +
> +	/*
> +	 * Quote requests usually take a few seconds to complete, so waking up
> +	 * once per second to recheck the status is fine for this use case.
> +	 */
> +	while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
> +		ssleep(1);
> +
> +	return (i == timeout) ? -ETIMEDOUT : 0;
> +}

Why can't we use wait_for_completion_timeout() provided by the kernel?

Btw, can we use _killable() variant?  Supporting timeout brings extra
complication, and I think supporting timeout isn't mandatory for the first
version.

> +
> +static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
> +{
> +	struct tdx_quote_buf *quote_buf = quote_data;
> +	int ret;
> +	u8 *buf;
> +	u64 err;
> +
> +	guard(mutex)(&quote_lock);
> +
> +	/*
> +	 * If the previous request is timedout and Quote buf status is
> +	 * still in GET_QUOTE_IN_FLIGHT (owned by VMM), don't permit any
> +	 * new request.
> +	 */
> +	if (quote_buf->status == GET_QUOTE_IN_FLIGHT)
> +		return ERR_PTR(-EBUSY);
> +
> +	if (desc->inblob_len != TDX_REPORTDATA_LEN)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* TDX attestation does not support multiple formats */

"only supports default format request" ?

> +	if (desc->outblob_format != TSM_FORMAT_DEFAULT)
> +		return ERR_PTR(-EINVAL);
> +
> +	u8 *reportdata __free(kfree) = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
> +	if (!reportdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	u8 *tdreport __free(kfree) = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
> +	if (!tdreport)
> +		return ERR_PTR(-ENOMEM);
> +
> +	memcpy(reportdata, desc->inblob, desc->inblob_len);
> +
> +	/* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
> +	ret = tdx_mcall_get_report0(reportdata, tdreport);
> +	if (ret) {
> +		pr_err("GetReport call failed\n");
> +		return ERR_PTR(ret);
> +	}
> +
> +	memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
> +
> +	/* Update Quote buffer header */
> +	quote_buf->version = GET_QUOTE_CMD_VER;
> +	quote_buf->in_len = TDX_REPORT_LEN;
> +
> +	memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
> +
> +	err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
> +	if (err) {
> +		pr_err("GetQuote hypercall failed, status:%llx\n", err);
> +		return ERR_PTR(-EIO);
> +	}
> +
> +	ret = wait_for_quote_completion(quote_buf, getquote_timeout);
> +	if (ret) {
> +		pr_err("GetQuote request timedout\n");
> +		return ERR_PTR(ret);
> +	}
> +
> +	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
> +	if (!buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	*outblob_len = quote_buf->out_len;
> +
> +	return buf;
> +}
> +
>  static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>  			    unsigned long arg)
>  {
> @@ -82,17 +254,48 @@ static const struct x86_cpu_id tdx_guest_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>  
> +static const struct tsm_ops tdx_tsm_ops = {
> +	.name = KBUILD_MODNAME,
> +	.report_new = tdx_report_new,
> +};
> +
>  static int __init tdx_guest_init(void)
>  {
> +	int ret;
> +
>  	if (!x86_match_cpu(tdx_guest_ids))
>  		return -ENODEV;
>  
> -	return misc_register(&tdx_misc_dev);
> +	ret = misc_register(&tdx_misc_dev);
> +	if (ret)
> +		return ret;
> +
> +	quote_data = alloc_quote_buf();
> +	if (!quote_data) {
> +		pr_err("Failed to allocate Quote buffer\n");
> +		ret = -ENOMEM;
> +		goto free_misc;
> +	}
> +
> +	ret = register_tsm(&tdx_tsm_ops, NULL, NULL);
> +	if (ret)
> +		goto free_quote;
> +
> +	return 0;
> +
> +free_quote:
> +	free_quote_buf(quote_data);
> +free_misc:
> +	misc_deregister(&tdx_misc_dev);
> +
> +	return ret;
>  }
>  module_init(tdx_guest_init);
>  
>  static void __exit tdx_guest_exit(void)
>  {
> +	unregister_tsm(&tdx_tsm_ops);
> +	free_quote_buf(quote_data);
>  	misc_deregister(&tdx_misc_dev);
>  }
>  module_exit(tdx_guest_exit);


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-08  4:50 ` Huang, Kai
@ 2023-09-08 16:19   ` Dan Williams
  2023-09-11  0:10     ` Kuppuswamy Sathyanarayanan
  2023-09-11 10:00     ` Huang, Kai
  0 siblings, 2 replies; 12+ messages in thread
From: Dan Williams @ 2023-09-08 16:19 UTC (permalink / raw)
  To: Huang, Kai, sathyanarayanan.kuppuswamy, tglx, mingo, Williams,
	Dan J, bp, dave.hansen
  Cc: Yu, Guorui, qinkun, wander, hpa, linux-kernel, kirill.shutemov,
	Aktas, Erdem, Luck, Tony, linux-coco, dionnaglaze, x86

Huang, Kai wrote:
> 
> > 
> > Changes since previous version:
> > * Used ConfigFS interface instead of IOCTL interface.
> > * Used polling model for Quote generation and dropped the event notification IRQ support.
> 
> Can you elaborate why the notification IRQ is dropped?

Because it was a pile of hacks and non-idiomatic complexity. It can come
back when / if driver code can treat it like a typical interrupt.

[..]
> >  
> > +/*
> > + * Intel's SGX QE implementation generally uses Quote size less
> > + * than 8K (2K Quote data + ~5K of ceritificate blob).
> > + */
> > +#define GET_QUOTE_BUF_SIZE		SZ_8K
> 
> SZ_8K is defined in <linux/sizes.h>.  It seems it's not explicitly included. 
> It's better to explicitly include it.
> 
> Btw, although the size of the certificate blob shouldn't change dramatically,
> the Quote can also include the "QE Authentication Data", which can vary a lot
> depending on different QE implementation (e.g., containing geography
> information, etc).
> 
> I wish eventually there's some /sysfs entry to configure the size of Quote
> buffer, but I guess it can be done in the future.

How would userspace have any idea of how big the quote buffer is to be
able to set it? The output format at least needs to standardized within
a given vendor's implementation, and future variation should be
de-emphasized relative to getting to a common report format across
vendors.

[..]
> > +/*
> > + * wait_for_quote_completion() - Wait for Quote request completion
> > + * @quote_buf: Address of Quote buffer.
> > + * @timeout: Timeout in seconds to wait for the Quote generation.
> > + *
> > + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
> > + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
> > + * while VMM processes the GetQuote request, and will change it to success
> > + * or error code after processing is complete. So wait till the status
> > + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
> > + */
> > +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
> > +{
> > +	int i = 0;
> > +
> > +	/*
> > +	 * Quote requests usually take a few seconds to complete, so waking up
> > +	 * once per second to recheck the status is fine for this use case.
> > +	 */
> > +	while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
> > +		ssleep(1);
> > +
> > +	return (i == timeout) ? -ETIMEDOUT : 0;
> > +}
> 
> Why can't we use wait_for_completion_timeout() provided by the kernel?
> 
> Btw, can we use _killable() variant?  Supporting timeout brings extra
> complication, and I think supporting timeout isn't mandatory for the first
> version.

It definitely is required even if interrupts were supported. The kernel
needs to give up on stalled operations in a reasonable amount of time.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-08  4:10     ` Dan Williams
@ 2023-09-09  5:24       ` Kuppuswamy Sathyanarayanan
  2023-09-13  3:51       ` Kuppuswamy Sathyanarayanan
  1 sibling, 0 replies; 12+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-09  5:24 UTC (permalink / raw)
  To: Dan Williams, Erdem Aktas
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
	Kirill A . Shutemov, H . Peter Anvin, Tony Luck,
	Wander Lairson Costa, Dionna Amalie Glaze, Qinkun Bao, Guorui Yu,
	linux-coco, x86, linux-kernel

Hi Dan,

On 9/7/2023 9:10 PM, Dan Williams wrote:
> Kuppuswamy Sathyanarayanan wrote:
>> Hi Erdem,
>>
>> Thanks for the review.
>>
>> On 9/7/2023 6:30 PM, Erdem Aktas wrote:
>>> On Wed, Sep 6, 2023 at 7:54 PM Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>>
>>>> In TDX guest, the attestation process is used to verify the TDX guest
>>>> trustworthiness to other entities before provisioning secrets to the
>>>> guest. The First step in the attestation process is TDREPORT
>>>> generation, which involves getting the guest measurement data in the
>>>> format of TDREPORT, which is further used to validate the authenticity
>>>> of the TDX guest. TDREPORT by design is integrity-protected and can
>>>> only be verified on the local machine.
>>>>
>>>> To support remote verification of the TDREPORT (in a SGX-based
>>>> attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
>>>> (QE) to convert it to a remote verifiable Quote. SGX QE by design can
>>> s/remote/remotely ?
>>>> only run outside of the TDX guest (i.e. in a host process or in a
>>>> normal VM) and guest can use communication channels like vsock or
>>>> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
>>>> TDX guest may not support these communication channels. To handle such
>>>> cases, TDX defines a GetQuote hypercall which can be used by the guest
>>>> to request the host VMM to communicate with the SGX 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>".
>>>>
>>>> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
>>>> Computing Guest platforms to get the measurement data via ConfigFS.
>>>> Extend the TSM framework and add support to allow an attestation agent
>>>> to get the TDX Quote data (included usage example below).
>>>>
>>>>   report=/sys/kernel/config/tsm/report/report0
>>>>   mkdir $report
>>>>   dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>>   hexdump -C $report/outblob
>>>>   rmdir $report
>>>>
>>>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
>>>> with TDREPORT data as input, which is further used by the VMM to copy
>>>> the TD Quote result after successful Quote generation. To create the
>>>> shared buffer, allocate a large enough memory and mark it shared using
>>>> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
>>>> for GetQuote requests in the TDX TSM handler.
>>>>
>>>> Although this method reserves a fixed chunk of memory for GetQuote
>>>> requests, such one time allocation can help avoid memory fragmentation
>>>> related allocation failures later in the uptime of the guest.
>>>>
>>>> Since the Quote generation process is not time-critical or frequently
>>>> used, the current version uses a polling model for Quote requests and
>>>> it also does not support parallel GetQuote requests.
>>>>
>>>> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> ---
>>>>
>>>> Hi All,
>>>>
>>>> The previous version of this patch series [1] added support for TDX
>>>> Guest Quote generation via an IOCTL interface. Since we have multiple
>>>> vendors implementing such interface, to avoid ABI proliferation, Dan
>>>> proposed using a common ABI for it and submitted the Trusted Secure
>>>> module (TSM) report ABI support [2]. This patchset extends the
>>>> TSM REPORTS to implement the TDX Quote generation support. Since there
>>>> is a change in interface type, I have dropped the previous Acks.
>>>>
>>>> [1] https://lore.kernel.org/lkml/3c57deb0-a311-2aad-c06b-4938e33491b5@linux.intel.com/
>>>> [2] https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/
>>>>
>>>> Changes since previous version:
>>>> * Used ConfigFS interface instead of IOCTL interface.
>>>> * Used polling model for Quote generation and dropped the event notification IRQ support.
>>>>
>>>>  arch/x86/coco/tdx/tdx.c                 |  21 +++
>>>>  arch/x86/include/asm/shared/tdx.h       |   1 +
>>>>  arch/x86/include/asm/tdx.h              |   2 +
>>>>  drivers/virt/coco/tdx-guest/Kconfig     |   1 +
>>>>  drivers/virt/coco/tdx-guest/tdx-guest.c | 205 +++++++++++++++++++++++-
>>>>  5 files changed, 229 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>>>> index 1d6b863c42b0..20414ed82fc5 100644
>>>> --- a/arch/x86/coco/tdx/tdx.c
>>>> +++ b/arch/x86/coco/tdx/tdx.c
>>>> @@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
>>>>
>>>> +/**
>>>> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
>>>> + *                         hypercall.
>>>> + * @buf: Address of the directly mapped shared kernel buffer which
>>>> + *      contains TDREPORT data. The same buffer will be used by
>>>> + *      VMM to store the generated TD Quote output.
>>>> + * @size: size of the tdquote buffer (4KB-aligned).
>>>> + *
>>>> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
>>>> + * v1.0 specification for more information on GetQuote hypercall.
>>>> + * It is used in the TDX guest driver module to get the TD Quote.
>>>> + *
>>>> + * Return 0 on success or error code on failure.
>>>> + */
>>>> +u64 tdx_hcall_get_quote(u8 *buf, size_t size)
>>>> +{
>>>> +       /* Since buf is a shared memory, set the shared (decrypted) bits */
>>>> +       return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
>>>> +
>>>>  static void __noreturn tdx_panic(const char *msg)
>>>>  {
>>>>         struct tdx_hypercall_args args = {
>>>> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
>>>> index 7513b3bb69b7..9eab19950f39 100644
>>>> --- a/arch/x86/include/asm/shared/tdx.h
>>>> +++ b/arch/x86/include/asm/shared/tdx.h
>>>> @@ -22,6 +22,7 @@
>>>>
>>>>  /* TDX hypercall Leaf IDs */
>>>>  #define TDVMCALL_MAP_GPA               0x10001
>>>> +#define TDVMCALL_GET_QUOTE             0x10002
>>>>  #define TDVMCALL_REPORT_FATAL_ERROR    0x10003
>>>>
>>>>  #ifndef __ASSEMBLY__
>>>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>>>> index 603e6d1e9d4a..ebd1cda4875f 100644
>>>> --- a/arch/x86/include/asm/tdx.h
>>>> +++ b/arch/x86/include/asm/tdx.h
>>>> @@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
>>>>
>>>>  int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
>>>>
>>>> +u64 tdx_hcall_get_quote(u8 *buf, size_t size);
>>>> +
>>>>  #else
>>>>
>>>>  static inline void tdx_early_init(void) { };
>>>> diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
>>>> index 14246fc2fb02..22dd59e19431 100644
>>>> --- a/drivers/virt/coco/tdx-guest/Kconfig
>>>> +++ b/drivers/virt/coco/tdx-guest/Kconfig
>>>> @@ -1,6 +1,7 @@
>>>>  config TDX_GUEST_DRIVER
>>>>         tristate "TDX Guest driver"
>>>>         depends on INTEL_TDX_GUEST
>>>> +       select TSM_REPORTS
>>>>         help
>>>>           The driver provides userspace interface to communicate with
>>>>           the TDX module to request the TDX guest details like attestation
>>>> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
>>>> index 5e44a0fa69bd..135d89a7e418 100644
>>>> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
>>>> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
>>>> @@ -12,12 +12,59 @@
>>>>  #include <linux/mod_devicetable.h>
>>>>  #include <linux/string.h>
>>>>  #include <linux/uaccess.h>
>>>> +#include <linux/set_memory.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/tsm.h>
>>>>
>>>>  #include <uapi/linux/tdx-guest.h>
>>>>
>>>>  #include <asm/cpu_device_id.h>
>>>>  #include <asm/tdx.h>
>>>>
>>>> +/*
>>>> + * Intel's SGX QE implementation generally uses Quote size less
>>>> + * than 8K (2K Quote data + ~5K of ceritificate blob).
>>> s/ceritificate/certificate
>>>> + */
>>>> +#define GET_QUOTE_BUF_SIZE             SZ_8K
>>>> +
>>>> +#define GET_QUOTE_CMD_VER              1
>>>> +
>>>> +/* TDX GetQuote status codes */
>>>> +#define GET_QUOTE_SUCCESS              0
>>>> +#define GET_QUOTE_IN_FLIGHT            0xffffffffffffffff
>>>> +
>>>> +/* struct tdx_quote_buf: Format of Quote request buffer.
>>>> + * @version: Quote format version, filled by TD.
>>>> + * @status: Status code of Quote request, filled by VMM.
>>>> + * @in_len: Length of TDREPORT, filled by TD.
>>>> + * @out_len: Length of Quote data, filled by VMM.
>>>> + * @data: Quote data on output or TDREPORT on input.
>>>> + *
>>>> + * More details of Quote request buffer can be found in TDX
>>>> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
>>>> + * section titled "TDG.VP.VMCALL<GetQuote>"
>>>> + */
>>>> +struct tdx_quote_buf {
>>>> +       u64 version;
>>>> +       u64 status;
>>>> +       u32 in_len;
>>>> +       u32 out_len;
>>>> +       u8 data[];
>>>> +};
>>>> +
>>>> +/* Quote data buffer */
>>>> +static void *quote_data;
>>>> +
>>>> +/* Lock to streamline quote requests */
>>>> +static DEFINE_MUTEX(quote_lock);
>>>> +
>>>> +/*
>>>> + * GetQuote request timeout in seconds. Expect that 30 seconds
>>>> + * is enough time for QE to respond to any Quote requests.
>>>> + */
>>>> +static u32 getquote_timeout = 30;
>>>> +
>>>>  static long tdx_get_report0(struct tdx_report_req __user *req)
>>>>  {
>>>>         u8 *reportdata, *tdreport;
>>>> @@ -53,6 +100,131 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>>>>         return ret;
>>>>  }
>>>>
>>>> +static void free_quote_buf(void *buf)
>>>> +{
>>>> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
>>>> +       unsigned int count = len >> PAGE_SHIFT;
>>>> +
>>>> +       set_memory_encrypted((unsigned long)buf, count);
>>> Why not check the return error? if conversion fails (even though
>>> unlikely), we should at least print an error message.
>>
>> Ok. Since it is unlikely to fail, we can use WARN_ON().
>>
>> WARN_ON(set_memory_encrypted((unsigned long)buf, count))
> 
> No, panic_on_warn turns recoverable errors into fatal errors. Just
> pr_err(). I also assume you don't want memory that failed to be set back
> to encrypted into the free page pool, so it seems safer to leak the
> memory at this point.

Agree. I will use pr_err().

> 
>>
>>>> +
>>>> +       free_pages_exact(buf, len);
>>>> +}
>>>> +
>>>> +static void *alloc_quote_buf(void)
>>>> +{
>>>> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
>>>> +       unsigned int count = len >> PAGE_SHIFT;
>>>> +       void *addr;
>>>> +       int ret;
>>>> +
>>>> +       addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
>>>> +       if (!addr)
>>>> +               return NULL;
>>>> +
>>>> +       ret = set_memory_decrypted((unsigned long)addr, count);
>>>> +       if (ret) {
>>>> +               free_pages_exact(addr, len);
>>>> +               return NULL;
>>>> +       }
>>>> +
>>>> +       return addr;
>>>> +}
>>>> +
>>>> +/*
>>>> + * wait_for_quote_completion() - Wait for Quote request completion
>>>> + * @quote_buf: Address of Quote buffer.
>>>> + * @timeout: Timeout in seconds to wait for the Quote generation.
>>>> + *
>>>> + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
>>>> + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
>>>> + * while VMM processes the GetQuote request, and will change it to success
>>>> + * or error code after processing is complete. So wait till the status
>>>> + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
>>> s/timedout/being timed out?
>>>> + */
>>>> +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
>>>> +{
>>>> +       int i = 0;
>>>> +
>>>> +       /*
>>>> +        * Quote requests usually take a few seconds to complete, so waking up
>>>> +        * once per second to recheck the status is fine for this use case.
>>>> +        */
>>>> +       while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
>>>> +               ssleep(1);
>>> Would not this loop cause soft lock (or even panic) if getquote waits
>>> for 30s? Should we not yield?
>>
>> Since we are sleeping for a second in each cycle (which will relinquish the CPU
>> to other threads), it should not create a soft lockup.
> 
> No, this would need to release the lock while sleeping or otherwise make
> this interruptible to make that happen. In fact it should release the
> lock or use mutex_lock_interruptible() and configfs-tsm should switch to
> interruptible synchronization.

Ok, makes sense. I will use mutex_lock_interruptible() for quote_lock() and
I will also change ssleep() to msleep_interruptible().

> 
>>
>>>> +
>>>> +       return (i == timeout) ? -ETIMEDOUT : 0;
>>>> +}
>>>> +
>>>> +static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
>>>> +{
>>>> +       struct tdx_quote_buf *quote_buf = quote_data;
>>>> +       int ret;
>>>> +       u8 *buf;
>>>> +       u64 err;
>>>> +
>>>> +       guard(mutex)(&quote_lock);
>>> I understand that this does not support parallel getQuote requests but
>>> if the user space for some reason makes multiple requests, each
>>> request will finish until the previous ones are completed in kernel
>>
>> Softlockup will only happen if a thread hogs the CPU for a long time without
>> relinquishing it. In our case, user processes waiting for the lock will be in
>> sleep state. So it should not create a lockup issue.
> 
> Stuck in uninterruptible sleep is another failure mode of the lockup
> detector. The other threads waiting to acquire the mutex will fire being
> stuck behind this thread.

Ok. I missed that point. I can change it to mutex_lock_interruptible().

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-08 16:19   ` Dan Williams
@ 2023-09-11  0:10     ` Kuppuswamy Sathyanarayanan
  2023-09-11 10:01       ` Huang, Kai
  2023-09-11 10:00     ` Huang, Kai
  1 sibling, 1 reply; 12+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-11  0:10 UTC (permalink / raw)
  To: Dan Williams, Huang, Kai, tglx, mingo, bp, dave.hansen
  Cc: Yu, Guorui, qinkun, wander, hpa, linux-kernel, kirill.shutemov,
	Aktas, Erdem, Luck, Tony, linux-coco, dionnaglaze, x86



On 9/8/2023 9:19 AM, Dan Williams wrote:
>>> Changes since previous version:
>>> * Used ConfigFS interface instead of IOCTL interface.
>>> * Used polling model for Quote generation and dropped the event notification IRQ support.
>> Can you elaborate why the notification IRQ is dropped?
> Because it was a pile of hacks and non-idiomatic complexity. It can come
> back when / if driver code can treat it like a typical interrupt.

Currently, the VMM assumes that the vCPU that executes the TDG.VP.VMCALL
<SetupEventNotifyInterrupt> hypercall as the target vCPU for event
notification IRQ. To satisfy this assumption, the guest driver that uses
this hypercall/IRQ had to include CPU/IRQ affinity related code changes. This
adds unnecessary complication to the guest driver code without any real
ARCH need or benefit. So we want to modify the GHCI ABI to let TD guest pass
the target CPU as an argument. With this change, we can hide the IRQ related
configuration in the IRQ chip code and let the guest driver treat the
event notification IRQ as a regular interrupt.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-08 16:19   ` Dan Williams
  2023-09-11  0:10     ` Kuppuswamy Sathyanarayanan
@ 2023-09-11 10:00     ` Huang, Kai
  1 sibling, 0 replies; 12+ messages in thread
From: Huang, Kai @ 2023-09-11 10:00 UTC (permalink / raw)
  To: Williams, Dan J, sathyanarayanan.kuppuswamy, mingo, tglx, bp,
	dave.hansen
  Cc: Yu, Guorui, qinkun, wander, hpa, linux-kernel, kirill.shutemov,
	Luck, Tony, dionnaglaze, Aktas, Erdem, linux-coco, x86

On Fri, 2023-09-08 at 09:19 -0700, Dan Williams wrote:
> Huang, Kai wrote:
> > 
> > > 
> > > Changes since previous version:
> > > * Used ConfigFS interface instead of IOCTL interface.
> > > * Used polling model for Quote generation and dropped the event notification IRQ support.
> > 
> > Can you elaborate why the notification IRQ is dropped?
> 
> Because it was a pile of hacks and non-idiomatic complexity. It can come
> back when / if driver code can treat it like a typical interrupt.

Thanks.  Agreed.

> 
> [..]
> > >  
> > > +/*
> > > + * Intel's SGX QE implementation generally uses Quote size less
> > > + * than 8K (2K Quote data + ~5K of ceritificate blob).
> > > + */
> > > +#define GET_QUOTE_BUF_SIZE		SZ_8K
> > 
> > SZ_8K is defined in <linux/sizes.h>.  It seems it's not explicitly included. 
> > It's better to explicitly include it.
> > 
> > Btw, although the size of the certificate blob shouldn't change dramatically,
> > the Quote can also include the "QE Authentication Data", which can vary a lot
> > depending on different QE implementation (e.g., containing geography
> > information, etc).
> > 
> > I wish eventually there's some /sysfs entry to configure the size of Quote
> > buffer, but I guess it can be done in the future.
> 
> How would userspace have any idea of how big the quote buffer is to be
> able to set it? The output format at least needs to standardized within
> a given vendor's implementation, and future variation should be
> de-emphasized relative to getting to a common report format across
> vendors.

The Quoting enclave implements what to be included into the Quote.  The admin
who deploys the Quoting enclave knows the Quote size.

> 
> [..]
> > > +/*
> > > + * wait_for_quote_completion() - Wait for Quote request completion
> > > + * @quote_buf: Address of Quote buffer.
> > > + * @timeout: Timeout in seconds to wait for the Quote generation.
> > > + *
> > > + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
> > > + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
> > > + * while VMM processes the GetQuote request, and will change it to success
> > > + * or error code after processing is complete. So wait till the status
> > > + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
> > > + */
> > > +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
> > > +{
> > > +	int i = 0;
> > > +
> > > +	/*
> > > +	 * Quote requests usually take a few seconds to complete, so waking up
> > > +	 * once per second to recheck the status is fine for this use case.
> > > +	 */
> > > +	while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
> > > +		ssleep(1);
> > > +
> > > +	return (i == timeout) ? -ETIMEDOUT : 0;
> > > +}
> > 
> > Why can't we use wait_for_completion_timeout() provided by the kernel?
> > 
> > Btw, can we use _killable() variant?  Supporting timeout brings extra
> > complication, and I think supporting timeout isn't mandatory for the first
> > version.
> 
> It definitely is required even if interrupts were supported. The kernel
> needs to give up on stalled operations in a reasonable amount of time.

Thanks for explaining.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-11  0:10     ` Kuppuswamy Sathyanarayanan
@ 2023-09-11 10:01       ` Huang, Kai
  0 siblings, 0 replies; 12+ messages in thread
From: Huang, Kai @ 2023-09-11 10:01 UTC (permalink / raw)
  To: sathyanarayanan.kuppuswamy, Williams, Dan J, mingo, tglx, bp,
	dave.hansen
  Cc: Yu, Guorui, qinkun, wander, hpa, linux-kernel, kirill.shutemov,
	Luck, Tony, dionnaglaze, Aktas, Erdem, linux-coco, x86

On Sun, 2023-09-10 at 17:10 -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> On 9/8/2023 9:19 AM, Dan Williams wrote:
> > > > Changes since previous version:
> > > > * Used ConfigFS interface instead of IOCTL interface.
> > > > * Used polling model for Quote generation and dropped the event notification IRQ support.
> > > Can you elaborate why the notification IRQ is dropped?
> > Because it was a pile of hacks and non-idiomatic complexity. It can come
> > back when / if driver code can treat it like a typical interrupt.
> 
> Currently, the VMM assumes that the vCPU that executes the TDG.VP.VMCALL
> <SetupEventNotifyInterrupt> hypercall as the target vCPU for event
> notification IRQ. To satisfy this assumption, the guest driver that uses
> this hypercall/IRQ had to include CPU/IRQ affinity related code changes. This
> adds unnecessary complication to the guest driver code without any real
> ARCH need or benefit. So we want to modify the GHCI ABI to let TD guest pass
> the target CPU as an argument. With this change, we can hide the IRQ related
> configuration in the IRQ chip code and let the guest driver treat the
> event notification IRQ as a regular interrupt.
> 

Sorry I missed some internal discussion.  Please feel free to ignore my
comments. 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
  2023-09-08  4:10     ` Dan Williams
  2023-09-09  5:24       ` Kuppuswamy Sathyanarayanan
@ 2023-09-13  3:51       ` Kuppuswamy Sathyanarayanan
  1 sibling, 0 replies; 12+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-13  3:51 UTC (permalink / raw)
  To: Dan Williams, Erdem Aktas
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
	Kirill A . Shutemov, H . Peter Anvin, Tony Luck,
	Wander Lairson Costa, Dionna Amalie Glaze, Qinkun Bao, Guorui Yu,
	linux-coco, x86, linux-kernel



On 9/7/2023 9:10 PM, Dan Williams wrote:
> Kuppuswamy Sathyanarayanan wrote:
>> Hi Erdem,
>>
>> Thanks for the review.
>>
>> On 9/7/2023 6:30 PM, Erdem Aktas wrote:
>>> On Wed, Sep 6, 2023 at 7:54 PM Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>>
>>>> In TDX guest, the attestation process is used to verify the TDX guest
>>>> trustworthiness to other entities before provisioning secrets to the
>>>> guest. The First step in the attestation process is TDREPORT
>>>> generation, which involves getting the guest measurement data in the
>>>> format of TDREPORT, which is further used to validate the authenticity
>>>> of the TDX guest. TDREPORT by design is integrity-protected and can
>>>> only be verified on the local machine.
>>>>
>>>> To support remote verification of the TDREPORT (in a SGX-based
>>>> attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
>>>> (QE) to convert it to a remote verifiable Quote. SGX QE by design can
>>> s/remote/remotely ?
>>>> only run outside of the TDX guest (i.e. in a host process or in a
>>>> normal VM) and guest can use communication channels like vsock or
>>>> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
>>>> TDX guest may not support these communication channels. To handle such
>>>> cases, TDX defines a GetQuote hypercall which can be used by the guest
>>>> to request the host VMM to communicate with the SGX 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>".
>>>>
>>>> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
>>>> Computing Guest platforms to get the measurement data via ConfigFS.
>>>> Extend the TSM framework and add support to allow an attestation agent
>>>> to get the TDX Quote data (included usage example below).
>>>>
>>>>   report=/sys/kernel/config/tsm/report/report0
>>>>   mkdir $report
>>>>   dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>>   hexdump -C $report/outblob
>>>>   rmdir $report
>>>>
>>>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
>>>> with TDREPORT data as input, which is further used by the VMM to copy
>>>> the TD Quote result after successful Quote generation. To create the
>>>> shared buffer, allocate a large enough memory and mark it shared using
>>>> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
>>>> for GetQuote requests in the TDX TSM handler.
>>>>
>>>> Although this method reserves a fixed chunk of memory for GetQuote
>>>> requests, such one time allocation can help avoid memory fragmentation
>>>> related allocation failures later in the uptime of the guest.
>>>>
>>>> Since the Quote generation process is not time-critical or frequently
>>>> used, the current version uses a polling model for Quote requests and
>>>> it also does not support parallel GetQuote requests.
>>>>
>>>> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> ---
>>>>
>>>> Hi All,
>>>>
>>>> The previous version of this patch series [1] added support for TDX
>>>> Guest Quote generation via an IOCTL interface. Since we have multiple
>>>> vendors implementing such interface, to avoid ABI proliferation, Dan
>>>> proposed using a common ABI for it and submitted the Trusted Secure
>>>> module (TSM) report ABI support [2]. This patchset extends the
>>>> TSM REPORTS to implement the TDX Quote generation support. Since there
>>>> is a change in interface type, I have dropped the previous Acks.
>>>>
>>>> [1] https://lore.kernel.org/lkml/3c57deb0-a311-2aad-c06b-4938e33491b5@linux.intel.com/
>>>> [2] https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/
>>>>
>>>> Changes since previous version:
>>>> * Used ConfigFS interface instead of IOCTL interface.
>>>> * Used polling model for Quote generation and dropped the event notification IRQ support.
>>>>
>>>>  arch/x86/coco/tdx/tdx.c                 |  21 +++
>>>>  arch/x86/include/asm/shared/tdx.h       |   1 +
>>>>  arch/x86/include/asm/tdx.h              |   2 +
>>>>  drivers/virt/coco/tdx-guest/Kconfig     |   1 +
>>>>  drivers/virt/coco/tdx-guest/tdx-guest.c | 205 +++++++++++++++++++++++-
>>>>  5 files changed, 229 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>>>> index 1d6b863c42b0..20414ed82fc5 100644
>>>> --- a/arch/x86/coco/tdx/tdx.c
>>>> +++ b/arch/x86/coco/tdx/tdx.c
>>>> @@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
>>>>
>>>> +/**
>>>> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
>>>> + *                         hypercall.
>>>> + * @buf: Address of the directly mapped shared kernel buffer which
>>>> + *      contains TDREPORT data. The same buffer will be used by
>>>> + *      VMM to store the generated TD Quote output.
>>>> + * @size: size of the tdquote buffer (4KB-aligned).
>>>> + *
>>>> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
>>>> + * v1.0 specification for more information on GetQuote hypercall.
>>>> + * It is used in the TDX guest driver module to get the TD Quote.
>>>> + *
>>>> + * Return 0 on success or error code on failure.
>>>> + */
>>>> +u64 tdx_hcall_get_quote(u8 *buf, size_t size)
>>>> +{
>>>> +       /* Since buf is a shared memory, set the shared (decrypted) bits */
>>>> +       return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
>>>> +
>>>>  static void __noreturn tdx_panic(const char *msg)
>>>>  {
>>>>         struct tdx_hypercall_args args = {
>>>> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
>>>> index 7513b3bb69b7..9eab19950f39 100644
>>>> --- a/arch/x86/include/asm/shared/tdx.h
>>>> +++ b/arch/x86/include/asm/shared/tdx.h
>>>> @@ -22,6 +22,7 @@
>>>>
>>>>  /* TDX hypercall Leaf IDs */
>>>>  #define TDVMCALL_MAP_GPA               0x10001
>>>> +#define TDVMCALL_GET_QUOTE             0x10002
>>>>  #define TDVMCALL_REPORT_FATAL_ERROR    0x10003
>>>>
>>>>  #ifndef __ASSEMBLY__
>>>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>>>> index 603e6d1e9d4a..ebd1cda4875f 100644
>>>> --- a/arch/x86/include/asm/tdx.h
>>>> +++ b/arch/x86/include/asm/tdx.h
>>>> @@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
>>>>
>>>>  int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
>>>>
>>>> +u64 tdx_hcall_get_quote(u8 *buf, size_t size);
>>>> +
>>>>  #else
>>>>
>>>>  static inline void tdx_early_init(void) { };
>>>> diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
>>>> index 14246fc2fb02..22dd59e19431 100644
>>>> --- a/drivers/virt/coco/tdx-guest/Kconfig
>>>> +++ b/drivers/virt/coco/tdx-guest/Kconfig
>>>> @@ -1,6 +1,7 @@
>>>>  config TDX_GUEST_DRIVER
>>>>         tristate "TDX Guest driver"
>>>>         depends on INTEL_TDX_GUEST
>>>> +       select TSM_REPORTS
>>>>         help
>>>>           The driver provides userspace interface to communicate with
>>>>           the TDX module to request the TDX guest details like attestation
>>>> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
>>>> index 5e44a0fa69bd..135d89a7e418 100644
>>>> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
>>>> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
>>>> @@ -12,12 +12,59 @@
>>>>  #include <linux/mod_devicetable.h>
>>>>  #include <linux/string.h>
>>>>  #include <linux/uaccess.h>
>>>> +#include <linux/set_memory.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/tsm.h>
>>>>
>>>>  #include <uapi/linux/tdx-guest.h>
>>>>
>>>>  #include <asm/cpu_device_id.h>
>>>>  #include <asm/tdx.h>
>>>>
>>>> +/*
>>>> + * Intel's SGX QE implementation generally uses Quote size less
>>>> + * than 8K (2K Quote data + ~5K of ceritificate blob).
>>> s/ceritificate/certificate
>>>> + */
>>>> +#define GET_QUOTE_BUF_SIZE             SZ_8K
>>>> +
>>>> +#define GET_QUOTE_CMD_VER              1
>>>> +
>>>> +/* TDX GetQuote status codes */
>>>> +#define GET_QUOTE_SUCCESS              0
>>>> +#define GET_QUOTE_IN_FLIGHT            0xffffffffffffffff
>>>> +
>>>> +/* struct tdx_quote_buf: Format of Quote request buffer.
>>>> + * @version: Quote format version, filled by TD.
>>>> + * @status: Status code of Quote request, filled by VMM.
>>>> + * @in_len: Length of TDREPORT, filled by TD.
>>>> + * @out_len: Length of Quote data, filled by VMM.
>>>> + * @data: Quote data on output or TDREPORT on input.
>>>> + *
>>>> + * More details of Quote request buffer can be found in TDX
>>>> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
>>>> + * section titled "TDG.VP.VMCALL<GetQuote>"
>>>> + */
>>>> +struct tdx_quote_buf {
>>>> +       u64 version;
>>>> +       u64 status;
>>>> +       u32 in_len;
>>>> +       u32 out_len;
>>>> +       u8 data[];
>>>> +};
>>>> +
>>>> +/* Quote data buffer */
>>>> +static void *quote_data;
>>>> +
>>>> +/* Lock to streamline quote requests */
>>>> +static DEFINE_MUTEX(quote_lock);
>>>> +
>>>> +/*
>>>> + * GetQuote request timeout in seconds. Expect that 30 seconds
>>>> + * is enough time for QE to respond to any Quote requests.
>>>> + */
>>>> +static u32 getquote_timeout = 30;
>>>> +
>>>>  static long tdx_get_report0(struct tdx_report_req __user *req)
>>>>  {
>>>>         u8 *reportdata, *tdreport;
>>>> @@ -53,6 +100,131 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>>>>         return ret;
>>>>  }
>>>>
>>>> +static void free_quote_buf(void *buf)
>>>> +{
>>>> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
>>>> +       unsigned int count = len >> PAGE_SHIFT;
>>>> +
>>>> +       set_memory_encrypted((unsigned long)buf, count);
>>> Why not check the return error? if conversion fails (even though
>>> unlikely), we should at least print an error message.
>>
>> Ok. Since it is unlikely to fail, we can use WARN_ON().
>>
>> WARN_ON(set_memory_encrypted((unsigned long)buf, count))
> 
> No, panic_on_warn turns recoverable errors into fatal errors. Just
> pr_err(). I also assume you don't want memory that failed to be set back
> to encrypted into the free page pool, so it seems safer to leak the
> memory at this point.
> 
>>
>>>> +
>>>> +       free_pages_exact(buf, len);
>>>> +}
>>>> +
>>>> +static void *alloc_quote_buf(void)
>>>> +{
>>>> +       size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
>>>> +       unsigned int count = len >> PAGE_SHIFT;
>>>> +       void *addr;
>>>> +       int ret;
>>>> +
>>>> +       addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
>>>> +       if (!addr)
>>>> +               return NULL;
>>>> +
>>>> +       ret = set_memory_decrypted((unsigned long)addr, count);
>>>> +       if (ret) {
>>>> +               free_pages_exact(addr, len);
>>>> +               return NULL;
>>>> +       }
>>>> +
>>>> +       return addr;
>>>> +}
>>>> +
>>>> +/*
>>>> + * wait_for_quote_completion() - Wait for Quote request completion
>>>> + * @quote_buf: Address of Quote buffer.
>>>> + * @timeout: Timeout in seconds to wait for the Quote generation.
>>>> + *
>>>> + * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
>>>> + * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
>>>> + * while VMM processes the GetQuote request, and will change it to success
>>>> + * or error code after processing is complete. So wait till the status
>>>> + * changes from GET_QUOTE_IN_FLIGHT or the request timedout.
>>> s/timedout/being timed out?
>>>> + */
>>>> +static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
>>>> +{
>>>> +       int i = 0;
>>>> +
>>>> +       /*
>>>> +        * Quote requests usually take a few seconds to complete, so waking up
>>>> +        * once per second to recheck the status is fine for this use case.
>>>> +        */
>>>> +       while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout)
>>>> +               ssleep(1);
>>> Would not this loop cause soft lock (or even panic) if getquote waits
>>> for 30s? Should we not yield?
>>
>> Since we are sleeping for a second in each cycle (which will relinquish the CPU
>> to other threads), it should not create a soft lockup.
> 
> No, this would need to release the lock while sleeping or otherwise make
> this interruptible to make that happen. In fact it should release the
> lock or use mutex_lock_interruptible() and configfs-tsm should switch to
> interruptible synchronization.
> 
>>
>>>> +
>>>> +       return (i == timeout) ? -ETIMEDOUT : 0;
>>>> +}
>>>> +
>>>> +static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
>>>> +{
>>>> +       struct tdx_quote_buf *quote_buf = quote_data;
>>>> +       int ret;
>>>> +       u8 *buf;
>>>> +       u64 err;
>>>> +
>>>> +       guard(mutex)(&quote_lock);
>>> I understand that this does not support parallel getQuote requests but
>>> if the user space for some reason makes multiple requests, each
>>> request will finish until the previous ones are completed in kernel
>>
>> Softlockup will only happen if a thread hogs the CPU for a long time without
>> relinquishing it. In our case, user processes waiting for the lock will be in
>> sleep state. So it should not create a lockup issue.
> 
> Stuck in uninterruptible sleep is another failure mode of the lockup
> detector. The other threads waiting to acquire the mutex will fire being
> stuck behind this thread.
> 

Used interruptible variants for sleep and quote_lock. Final version looks like
below.

+/*
+ * wait_for_quote_completion() - Wait for Quote request completion
+ * @quote_buf: Address of Quote buffer.
+ * @timeout: Timeout in seconds to wait for the Quote generation.
+ *
+ * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
+ * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
+ * while VMM processes the GetQuote request, and will change it to success
+ * or error code after processing is complete. So wait till the status
+ * changes from GET_QUOTE_IN_FLIGHT or the request being timed out.
+ */
+static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
+{
+	int i = 0;
+
+	/*
+	 * Quote requests usually take a few seconds to complete, so waking up
+	 * once per second to recheck the status is fine for this use case.
+	 */
+	while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout) {
+		if (msleep_interruptible(MSEC_PER_SEC))
+			return -EINTR;
+	}
+
+	return (i == timeout) ? -ETIMEDOUT : 0;
+}
+
+static u8 *tdx_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
+{
+	struct tdx_quote_buf *quote_buf = quote_data;
+	int ret;
+	u8 *buf;
+	u64 err;
+
+	if (mutex_lock_interruptible(&quote_lock))
+		return ERR_PTR(-EINTR);
+
+	/*
+	 * If the previous request is timedout or interrupted, and the
+	 * Quote buf status is still in GET_QUOTE_IN_FLIGHT (owned by
+	 * VMM), don't permit any new request.
+	 */
+	if (quote_buf->status == GET_QUOTE_IN_FLIGHT) {
+		ret = -EBUSY;
+		goto done;
+	}
+
+	if (desc->inblob_len != TDX_REPORTDATA_LEN) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	/* TDX attestation only supports default format request */
+	if (desc->outblob_format != TSM_FORMAT_DEFAULT) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	u8 *reportdata __free(kfree) = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
+	if (!reportdata) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	u8 *tdreport __free(kfree) = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
+	if (!tdreport) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	memcpy(reportdata, desc->inblob, desc->inblob_len);
+
+	/* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
+	ret = tdx_mcall_get_report0(reportdata, tdreport);
+	if (ret) {
+		pr_err("GetReport call failed\n");
+		goto done;
+	}
+
+	memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
+
+	/* Update Quote buffer header */
+	quote_buf->version = GET_QUOTE_CMD_VER;
+	quote_buf->in_len = TDX_REPORT_LEN;
+
+	memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
+
+	err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
+	if (err) {
+		pr_err("GetQuote hypercall failed, status:%llx\n", err);
+		ret = -EIO;
+		goto done;
+	}
+
+	ret = wait_for_quote_completion(quote_buf, getquote_timeout);
+	if (ret) {
+		pr_err("GetQuote request timedout\n");
+		goto done;
+	}
+
+	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	*outblob_len = quote_buf->out_len;
+
+done:
+	mutex_unlock(&quote_lock);
+	return ret ? ERR_PTR(ret) : buf;
+}

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-09-13  3:52 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-07  2:54 [PATCH v1] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Kuppuswamy Sathyanarayanan
2023-09-08  0:03 ` kernel test robot
2023-09-08  1:30 ` Erdem Aktas
2023-09-08  2:29   ` Kuppuswamy Sathyanarayanan
2023-09-08  4:10     ` Dan Williams
2023-09-09  5:24       ` Kuppuswamy Sathyanarayanan
2023-09-13  3:51       ` Kuppuswamy Sathyanarayanan
2023-09-08  4:50 ` Huang, Kai
2023-09-08 16:19   ` Dan Williams
2023-09-11  0:10     ` Kuppuswamy Sathyanarayanan
2023-09-11 10:01       ` Huang, Kai
2023-09-11 10:00     ` Huang, Kai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).