2022-04-23 00:17:52

by Kuppuswamy Sathyanarayanan

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

In TDX guest, the second stage in attestation process is quote
generation and signing. GetQuote hypercall can be used by the TD guest
to request VMM facilitate the quote generation via a Quoting Enclave
(QE). More details about GetQuote hypercall can be found in TDX
Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
titled "TDG.VP.VMCALL<GetQuote>.

Since GetQuote is an asynchronous request hypercall, it will not block
till the quote is generated. So VMM uses callback interrupt vector
configured by SetupEventNotifyInterrupt hypercall to notify the guest
about quote generation completion or failure. Upon receiving the
completion notification, status can be found in the Quote data header.

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

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

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

Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Acked-by: Kirill A. Shutemov <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
arch/x86/coco/tdx/attest.c | 118 +++++++++++++++++++++++++++++++-
arch/x86/coco/tdx/tdx.c | 37 ++++++++++
arch/x86/include/asm/tdx.h | 2 +
arch/x86/include/uapi/asm/tdx.h | 36 ++++++++++
4 files changed, 191 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
index b776e81f6c20..d485163d3222 100644
--- a/arch/x86/coco/tdx/attest.c
+++ b/arch/x86/coco/tdx/attest.c
@@ -32,6 +32,11 @@
static struct platform_device *pdev;
static struct miscdevice miscdev;

+/* Completion object to track GetQuote completion status */
+static DECLARE_COMPLETION(req_compl);
+/* Mutex to serialize GetQuote requests */
+static DEFINE_MUTEX(quote_lock);
+
static long tdx_get_tdreport(void __user *argp)
{
void *report_buf = NULL, *tdreport_buf = NULL;
@@ -79,6 +84,95 @@ static long tdx_get_tdreport(void __user *argp)
return ret;
}

+static long tdx_get_tdquote(void __user *argp)
+{
+ struct tdx_quote_hdr *quote_hdr;
+ struct tdx_quote_req quote_req;
+ void *quote_buf = NULL;
+ dma_addr_t handle;
+ long ret = 0, err;
+ u64 quote_buf_len;
+
+ mutex_lock(&quote_lock);
+
+ reinit_completion(&req_compl);
+
+ /* Copy Quote request struct from user buffer */
+ if (copy_from_user(&quote_req, argp, sizeof(struct tdx_quote_req)))
+ return -EFAULT;
+
+ /* Make sure the length & timeout is valid */
+ if (quote_req.len <= 0 || quote_req.timeout <= 0)
+ return -EINVAL;
+
+ /* Align with page size to meet 4K alignment */
+ quote_buf_len = PAGE_ALIGN(quote_req.len);
+
+ /*
+ * Allocate DMA buffer to get TDQUOTE data from the VMM.
+ * dma_alloc_coherent() API internally marks allocated
+ * memory as shared with VMM. So explicit shared mapping is
+ * not required.
+ */
+ quote_buf = dma_alloc_coherent(&pdev->dev, quote_buf_len, &handle,
+ GFP_KERNEL | __GFP_ZERO);
+ if (!quote_buf) {
+ ret = -ENOMEM;
+ goto quote_failed;
+ }
+
+ /* Copy TDREPORT from user Quote data buffer to kernel Quote buffer */
+ if (copy_from_user(quote_buf, (void __user *)quote_req.buf, quote_req.len)) {
+ ret = -EFAULT;
+ goto quote_failed;
+ }
+
+ /* Submit GetQuote Request */
+ err = tdx_hcall_get_quote(quote_buf, quote_buf_len);
+ if (err) {
+ /* if failed, copy hypercall error code to user buffer */
+ ret = put_user(err, (long __user *)argp);
+ ret = -EIO;
+ goto quote_failed;
+ }
+
+ /* Wait for attestation completion */
+ ret = wait_for_completion_interruptible_timeout(
+ &req_compl,
+ msecs_to_jiffies(quote_req.timeout));
+ if (ret <= 0) {
+ ret = -EIO;
+ goto quote_failed;
+ }
+
+ /* Copy generated Quote data back to user buffer */
+ if (copy_to_user((void __user *)quote_req.buf, quote_buf, quote_buf_len)) {
+ ret = -EFAULT;
+ goto quote_failed;
+ }
+
+ quote_hdr = (struct tdx_quote_hdr *)quote_buf;
+
+ /* Make sure quote generation is successful */
+ if (!quote_hdr->status)
+ ret = 0;
+ else
+ ret = -EIO;
+
+quote_failed:
+ if (quote_buf)
+ dma_free_coherent(&pdev->dev, quote_buf_len, quote_buf, handle);
+
+ mutex_unlock(&quote_lock);
+
+ return ret;
+}
+
+static void attestation_callback_handler(void)
+{
+ complete(&req_compl);
+}
+
static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
@@ -89,6 +183,9 @@ static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
case TDX_CMD_GET_TDREPORT:
ret = tdx_get_tdreport(argp);
break;
+ case TDX_CMD_GEN_QUOTE:
+ ret = tdx_get_tdquote(argp);
+ break;
default:
pr_err("cmd %d not supported\n", cmd);
break;
@@ -103,6 +200,14 @@ static const struct file_operations tdx_attest_fops = {
.llseek = no_llseek,
};

+/* Helper function to cleanup attestation related allocations */
+static void _tdx_attest_remove(void)
+{
+ misc_deregister(&miscdev);
+
+ tdx_remove_ev_notify_handler();
+}
+
static int tdx_attest_probe(struct platform_device *attest_pdev)
{
struct device *dev = &attest_pdev->dev;
@@ -114,6 +219,15 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)

pdev = attest_pdev;

+ ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+ if (ret) {
+ pr_err("dma set coherent mask failed\n");
+ goto failed;
+ }
+
+ /* Register attestation event notify handler */
+ tdx_setup_ev_notify_handler(attestation_callback_handler);
+
miscdev.name = DRIVER_NAME;
miscdev.minor = MISC_DYNAMIC_MINOR;
miscdev.fops = &tdx_attest_fops;
@@ -130,7 +244,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)
return 0;

failed:
- misc_deregister(&miscdev);
+ _tdx_attest_remove();

pr_debug("module initialization failed\n");

@@ -139,7 +253,7 @@ static int tdx_attest_probe(struct platform_device *attest_pdev)

static int tdx_attest_remove(struct platform_device *attest_pdev)
{
- misc_deregister(&miscdev);
+ _tdx_attest_remove();
pr_debug("module is successfully removed\n");
return 0;
}
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index d0c62b94a1f6..cba22a8d4084 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -25,6 +25,7 @@

/* TDX hypercall Leaf IDs */
#define TDVMCALL_MAP_GPA 0x10001
+#define TDVMCALL_GET_QUOTE 0x10002
#define TDVMCALL_SETUP_NOTIFY_INTR 0x10004

/* MMIO direction */
@@ -214,6 +215,42 @@ static long tdx_hcall_set_notify_intr(u8 vector)
return 0;
}

+/*
+ * tdx_hcall_get_quote() - Request for TDQUOTE using TDREPORT.
+ *
+ * @data : Address of 4KB aligned GPA memory which contains
+ * TDREPORT_STRUCT.
+ * @len : Length of the GPA in bytes.
+ *
+ * return 0 on success or failure error number.
+ */
+long tdx_hcall_get_quote(void *data, u64 len)
+{
+ u64 ret;
+
+ /*
+ * Use confidential guest TDX check to ensure this API is only
+ * used by TDX guest platforms.
+ */
+ if (!data || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return -EINVAL;
+
+ /*
+ * Pass the physical address of tdreport data to the VMM
+ * and trigger the TDQUOTE generation. It is not a blocking
+ * call, hence completion of this request will be notified to
+ * the TD guest via a callback interrupt. More info about ABI
+ * can be found in TDX Guest-Host-Communication Interface
+ * (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
+ */
+ ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
+ len, 0, 0);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
static u64 get_cc_mask(void)
{
struct tdx_module_output out;
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 89ed09809c13..90c2a5f6c40c 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));

void tdx_remove_ev_notify_handler(void);

+long tdx_hcall_get_quote(void *data, u64 len);
+
#else

static inline void tdx_early_init(void) { };
diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
index c21f9d6fe88b..69259b7841a9 100644
--- a/arch/x86/include/uapi/asm/tdx.h
+++ b/arch/x86/include/uapi/asm/tdx.h
@@ -20,4 +20,40 @@
*/
#define TDX_CMD_GET_TDREPORT _IOWR('T', 0x01, __u64)

+/*
+ * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
+ * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
+ * buffer of quote size. Once IOCTL is successful quote data is copied back to
+ * the user buffer. On failure, TDCALL error code is copied back to the user
+ * buffer.
+ */
+#define TDX_CMD_GEN_QUOTE _IOR('T', 0x02, __u64)
+
+struct tdx_quote_req {
+ /* Buffer address to store Quote data */
+ __u64 buf;
+ /* Length of the Quote buffer */
+ __u64 len;
+ /* Quote generation timeout value in ms */
+ __u32 timeout;
+};
+
+/*
+ * Format of quote data header. More details can be found in
+ * TDX Guest-Host Communication Interface (GHCI) for Intel TDX
+ * 1.0, section titled "TDG.VP.VMCALL<GetQuote>"
+ */
+struct tdx_quote_hdr {
+ /* Quote version, filled by TD */
+ __u64 version;
+ /* Status code of Quote request, filled by VMM */
+ __u64 status;
+ /* Length of TDREPORT, filled by TD */
+ __u32 in_len;
+ /* Length of Quote, filled by VMM */
+ __u32 out_len;
+ /* Actual Quote data */
+ __u64 data;
+};
+
#endif /* _UAPI_ASM_X86_TDX_H */
--
2.25.1

2022-04-27 11:01:02

by Isaku Yamahata

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

On Fri, Apr 22, 2022 at 04:34:18PM -0700,
Kuppuswamy Sathyanarayanan <[email protected]> wrote:

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

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


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

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


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

status check should be before copy_to_user() above.


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

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


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

Isn't X86_FEATURE_TDX_GUEST checked on module loading time?

> +
> + /*
> + * Pass the physical address of tdreport data to the VMM
> + * and trigger the TDQUOTE generation. It is not a blocking
> + * call, hence completion of this request will be notified to
> + * the TD guest via a callback interrupt. More info about ABI
> + * can be found in TDX Guest-Host-Communication Interface
> + * (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
> + */
> + ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
> + len, 0, 0);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> static u64 get_cc_mask(void)
> {
> struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 89ed09809c13..90c2a5f6c40c 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));
>
> void tdx_remove_ev_notify_handler(void);
>
> +long tdx_hcall_get_quote(void *data, u64 len);
> +
> #else
>
> static inline void tdx_early_init(void) { };
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> index c21f9d6fe88b..69259b7841a9 100644
> --- a/arch/x86/include/uapi/asm/tdx.h
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -20,4 +20,40 @@
> */
> #define TDX_CMD_GET_TDREPORT _IOWR('T', 0x01, __u64)
>
> +/*
> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
> + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input
> + * buffer of quote size. Once IOCTL is successful quote data is copied back to
> + * the user buffer. On failure, TDCALL error code is copied back to the user
> + * buffer.
> + */
> +#define TDX_CMD_GEN_QUOTE _IOR('T', 0x02, __u64)
> +
> +struct tdx_quote_req {
> + /* Buffer address to store Quote data */
> + __u64 buf;
> + /* Length of the Quote buffer */
> + __u64 len;
> + /* Quote generation timeout value in ms */
> + __u32 timeout;

What's the point of timeout?


> +};
> +
> +/*
> + * Format of quote data header. More details can be found in
> + * TDX Guest-Host Communication Interface (GHCI) for Intel TDX
> + * 1.0, section titled "TDG.VP.VMCALL<GetQuote>"
> + */
> +struct tdx_quote_hdr {
> + /* Quote version, filled by TD */
> + __u64 version;
> + /* Status code of Quote request, filled by VMM */
> + __u64 status;

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


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

--
Isaku Yamahata <[email protected]>

2022-04-27 11:06:47

by Kai Huang

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> + * call, hence completion of this request will be notified to
> + * the TD guest via a callback interrupt. More info about ABI
> + * can be found in TDX Guest-Host-Communication Interface
> + * (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
> + */
> + ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
> + len, 0, 0);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> static u64 get_cc_mask(void)
> {
> struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 89ed09809c13..90c2a5f6c40c 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));
>
> void tdx_remove_ev_notify_handler(void);
>
> +long tdx_hcall_get_quote(void *data, u64 len);
> +
> #else
>
> static inline void tdx_early_init(void) { };
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> index c21f9d6fe88b..69259b7841a9 100644
> --- a/arch/x86/include/uapi/asm/tdx.h
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -20,4 +20,40 @@
> */
> #define TDX_CMD_GET_TDREPORT _IOWR('T', 0x01, __u64)
>
> +/*
> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User

Replace "TD QUOTE" to some consistent name.

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

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

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

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

> + */
> +#define TDX_CMD_GEN_QUOTE _IOR('T', 0x02, __u64)
> +
> +struct tdx_quote_req {
> + /* Buffer address to store Quote data */
> + __u64 buf;
> + /* Length of the Quote buffer */
> + __u64 len;
> + /* Quote generation timeout value in ms */
> + __u32 timeout;
> +};
> +
> +/*
> + * Format of quote data header. More details can be found in
> + * TDX Guest-Host Communication Interface (GHCI) for Intel TDX
> + * 1.0, section titled "TDG.VP.VMCALL<GetQuote>"
> + */
> +struct tdx_quote_hdr {
> + /* Quote version, filled by TD */
> + __u64 version;
> + /* Status code of Quote request, filled by VMM */
> + __u64 status;
> + /* Length of TDREPORT, filled by TD */
> + __u32 in_len;
> + /* Length of Quote, filled by VMM */
> + __u32 out_len;
> + /* Actual Quote data */
> + __u64 data;
> +};

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



--
Thanks,
-Kai

2022-04-30 18:14:30

by Kuppuswamy Sathyanarayanan

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

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

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

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

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

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

Good catch. I will fix it in next version.

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

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

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

Isaku/Kai, any comments?

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

Yes. I will fix this in next version.

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

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2022-05-01 17:20:54

by Kuppuswamy Sathyanarayanan

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

Hi,

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

I will remove it in next version.

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

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


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

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

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

Agree ?

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

Explained above.

>
>
>> +};
>> +
>> +/*
>> + * Format of quote data header. More details can be found in
>> + * TDX Guest-Host Communication Interface (GHCI) for Intel TDX
>> + * 1.0, section titled "TDG.VP.VMCALL<GetQuote>"
>> + */
>> +struct tdx_quote_hdr {
>> + /* Quote version, filled by TD */
>> + __u64 version;
>> + /* Status code of Quote request, filled by VMM */
>> + __u64 status;
>
> If you export version and status, also define related constants for user space.

Ok.

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

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2022-05-02 23:35:03

by Wander Lairson Costa

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

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

[snip]

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

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

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

The assigment to ret is unused.

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

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

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

Shouldn't copy_to_user be called after checking the status?

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

Won't misc_deregister be called even if misc_register fails?

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

2022-05-02 23:53:46

by Kuppuswamy Sathyanarayanan

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

Hi Kai,

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

Yes.

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

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

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

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

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

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

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

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

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

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

As for names, I am fine with quote_buf and quote_len

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

Yes. Moved it to initcall of attestation driver.

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

Ok.

>
>> + * call, hence completion of this request will be notified to
>> + * the TD guest via a callback interrupt. More info about ABI
>> + * can be found in TDX Guest-Host-Communication Interface
>> + * (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
>> + */
>> + ret = _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(data)),
>> + len, 0, 0);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> static u64 get_cc_mask(void)
>> {
>> struct tdx_module_output out;
>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>> index 89ed09809c13..90c2a5f6c40c 100644
>> --- a/arch/x86/include/asm/tdx.h
>> +++ b/arch/x86/include/asm/tdx.h
>> @@ -73,6 +73,8 @@ void tdx_setup_ev_notify_handler(void (*handler)(void));
>>
>> void tdx_remove_ev_notify_handler(void);
>>
>> +long tdx_hcall_get_quote(void *data, u64 len);
>> +
>> #else
>>
>> static inline void tdx_early_init(void) { };
>> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
>> index c21f9d6fe88b..69259b7841a9 100644
>> --- a/arch/x86/include/uapi/asm/tdx.h
>> +++ b/arch/x86/include/uapi/asm/tdx.h
>> @@ -20,4 +20,40 @@
>> */
>> #define TDX_CMD_GET_TDREPORT _IOWR('T', 0x01, __u64)
>>
>> +/*
>> + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User
>
> Replace "TD QUOTE" to some consistent name.

Ok.

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

Noted. I will fix the description in next version.

>
>> Once IOCTL is successful quote data is copied back to
>> + * the user buffer. On failure, TDCALL error code is copied back to the user
>> + * buffer.
>
> The output data may be more than the Quote, the same as above.
>
>> + */
>> +#define TDX_CMD_GEN_QUOTE _IOR('T', 0x02, __u64)
>> +
>> +struct tdx_quote_req {
>> + /* Buffer address to store Quote data */
>> + __u64 buf;
>> + /* Length of the Quote buffer */
>> + __u64 len;
>> + /* Quote generation timeout value in ms */
>> + __u32 timeout;
>> +};
>> +
>> +/*
>> + * Format of quote data header. More details can be found in
>> + * TDX Guest-Host Communication Interface (GHCI) for Intel TDX
>> + * 1.0, section titled "TDG.VP.VMCALL<GetQuote>"
>> + */
>> +struct tdx_quote_hdr {
>> + /* Quote version, filled by TD */
>> + __u64 version;
>> + /* Status code of Quote request, filled by VMM */
>> + __u64 status;
>> + /* Length of TDREPORT, filled by TD */
>> + __u32 in_len;
>> + /* Length of Quote, filled by VMM */
>> + __u32 out_len;
>> + /* Actual Quote data */
>> + __u64 data;
>> +};
>
> Needs to be '__u64 data[0]'. The first 8B data isn't header.

Ok.

>
>
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer