Message-ID: <da4a6c3f4cb9118e10866cb1d624ad5ec5c96d7d.camel@intel.com>
Date:   Fri, 8 Sep 2023 04:50:09 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
CC:     "Yu, Guorui" <guorui.yu@...ux.alibaba.com>,
        "qinkun@...che.org" <qinkun@...che.org>,
        "wander@...hat.com" <wander@...hat.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
        "dionnaglaze@...gle.com" <dionnaglaze@...gle.com>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v1] virt: tdx-guest: Add Quote generation support using
 TSM_REPORTS


> 
> 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);

Powered by blists - more mailing lists