In TDX guest, attestation is used to verify the trustworthiness of a TD
to other entities before provisioning secrets to the TD.
One usage example is, when a TD guest uses encrypted drive and if the
decryption keys required to access the drive are stored in a secure 3rd
party keyserver, the key server can use attestation to verify TD's
trustworthiness and release the decryption keys to the TD.
The attestation process consists of two steps: TDREPORT generation and
Quote generation.
TDREPORT (TDREPORT_STRUCT) is a fixed-size data structure generated by
the TDX module which contains TD-specific information (such as TD
measurements), platform security version, and the MAC to protect the
integrity of the TDREPORT. The TD kernel uses TDCALL[TDG.MR.REPORT] to
get the TDREPORT from the TDX module. A user-provided 64-Byte
REPORTDATA is used as input and included in the TDREPORT. Typically it
can be some nonce provided by attestation service so the TDREPORT can
be verified uniquely. More details about TDREPORT can be found in
Intel TDX Module specification, section titled "TDG.MR.REPORT Leaf".
Also note that the MAC added to the TDREPORT is bound to the platform.
So TDREPORT can only be verified locally. Intel SGX Quote Enclave (QE)
is leveraged to verify the TDREPORT locally and convert it to a remote
verifiable Quote to support remote attestation of the TDREPORT.
After getting the TDREPORT, the second step of the attestation process
is to send it to QE or Quote Generation Service (QGS) to generate a TD
Quote. The QE sends the Quote back after it is generated. How is the
data sent and received is QE implementation and deployment specific.TD
userspace attestation software can use whatever communication channel
available (i.e. tcp/ip, vsock) to communicate with the QE using whatever
data format. TDX also defines TDVMCALLs to allow TD to ask VMM to
facilitate sending/receiving data between TD attestation software and
the QE. This support is documented in GHCI 1.0 spec "5.4 TD attestation".
Implement a basic attestation driver to allow TD userspace to get the
TDREPORT, which is sent to QE by the attestation software to generate
a Quote for remote verification.
Also note that explicit access permissions are not enforced in this
driver because the quote and measurements are not a secret. However
the access permissions of the device node can be used to set any
desired access policy. The udev default is usually root access
only.
Operations like getting TDREPORT or Quote generation involves sending
a blob of data as input and getting another blob of data as output. It
was considered to use a sysfs interface for this, but it doesn't fit
well into the standard sysfs model for configuring values. It would be
possible to do read/write on files, but it would need multiple file
descriptors, which would be somewhat messy. IOCTLs seems to be the best
fitting and simplest model for this use case. Also, the REPORTDATA used
in TDREPORT generation can possibly come from attestation service to
uniquely verify the Quote (like per instance verification). In such
case, since REPORTDATA is a secret, using sysfs to share it is insecure
compared to sending it via IOCTL.
Reviewed-by: Tony Luck <[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/Makefile | 2 +-
arch/x86/coco/tdx/attest.c | 118 ++++++++++++++++++++++++++++++++
arch/x86/include/uapi/asm/tdx.h | 42 ++++++++++++
3 files changed, 161 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/coco/tdx/attest.c
create mode 100644 arch/x86/include/uapi/asm/tdx.h
diff --git a/arch/x86/coco/tdx/Makefile b/arch/x86/coco/tdx/Makefile
index 46c55998557d..d2db3e6770e5 100644
--- a/arch/x86/coco/tdx/Makefile
+++ b/arch/x86/coco/tdx/Makefile
@@ -1,3 +1,3 @@
# SPDX-License-Identifier: GPL-2.0
-obj-y += tdx.o tdcall.o
+obj-y += tdx.o tdcall.o attest.o
diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
new file mode 100644
index 000000000000..a5f4111f9b18
--- /dev/null
+++ b/arch/x86/coco/tdx/attest.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * attest.c - TDX guest attestation interface driver.
+ *
+ * Implements user interface to trigger attestation process.
+ *
+ * Copyright (C) 2022 Intel Corporation
+ *
+ */
+
+#define pr_fmt(fmt) "x86/tdx: attest: " fmt
+
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/io.h>
+#include <asm/tdx.h>
+#include <uapi/asm/tdx.h>
+
+#define DRIVER_NAME "tdx-attest"
+
+/* TDREPORT module call leaf ID */
+#define TDX_GET_REPORT 4
+
+static struct miscdevice miscdev;
+
+static long tdx_get_report(void __user *argp)
+{
+ void *reportdata = NULL, *tdreport = NULL;
+ long ret = 0;
+
+ /* Allocate buffer space for REPORTDATA */
+ reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
+ if (!reportdata)
+ return -ENOMEM;
+
+ /* Allocate buffer space for TDREPORT */
+ tdreport = kmalloc(TDX_REPORT_LEN, GFP_KERNEL);
+ if (!tdreport) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ /* Copy REPORTDATA from the user buffer */
+ if (copy_from_user(reportdata, argp, TDX_REPORTDATA_LEN)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ /*
+ * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
+ *
+ * Get the TDREPORT using REPORTDATA as input. Refer to
+ * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
+ * Specification for detailed information.
+ */
+ ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
+ virt_to_phys(reportdata), 0, 0, NULL);
+ if (ret) {
+ pr_debug("TDREPORT TDCALL failed, status:%lx\n", ret);
+ ret = -EIO;
+ goto out;
+ }
+
+ /* Copy TDREPORT back to the user buffer */
+ if (copy_to_user(argp, tdreport, TDX_REPORT_LEN))
+ ret = -EFAULT;
+
+out:
+ kfree(reportdata);
+ kfree(tdreport);
+ return ret;
+}
+
+static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ void __user *argp = (void __user *)arg;
+ long ret = -EINVAL;
+
+ switch (cmd) {
+ case TDX_CMD_GET_REPORT:
+ ret = tdx_get_report(argp);
+ break;
+ default:
+ pr_debug("cmd %d not supported\n", cmd);
+ break;
+ }
+
+ return ret;
+}
+
+static const struct file_operations tdx_attest_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = tdx_attest_ioctl,
+ .llseek = no_llseek,
+};
+
+static int __init tdx_attestation_init(void)
+{
+ long ret;
+
+ /* Make sure we are in a valid TDX platform */
+ if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return -EIO;
+
+ miscdev.name = DRIVER_NAME;
+ miscdev.minor = MISC_DYNAMIC_MINOR;
+ miscdev.fops = &tdx_attest_fops;
+
+ ret = misc_register(&miscdev);
+ if (ret) {
+ pr_err("misc device registration failed\n");
+ return ret;
+ }
+
+ return 0;
+}
+device_initcall(tdx_attestation_init)
diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
new file mode 100644
index 000000000000..e06da56058a1
--- /dev/null
+++ b/arch/x86/include/uapi/asm/tdx.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_ASM_X86_TDX_H
+#define _UAPI_ASM_X86_TDX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
+#define TDX_REPORTDATA_LEN 64
+
+/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
+#define TDX_REPORT_LEN 1024
+
+/**
+ * struct tdx_report_req: Get TDREPORT using REPORTDATA as input.
+ *
+ * @reportdata : User-defined 64-Byte REPORTDATA to be included into
+ * TDREPORT. Typically it can be some nonce provided by
+ * attestation software so the generated TDREPORT can be
+ * uniquely verified.
+ * @tdreport : TDREPORT output from TDCALL[TDG.MR.REPORT] of size
+ * TDX_REPORT_LEN.
+ *
+ * Used in TDX_CMD_GET_REPORT IOCTL request.
+ */
+struct tdx_report_req {
+ union {
+ __u8 reportdata[TDX_REPORTDATA_LEN];
+ __u8 tdreport[TDX_REPORT_LEN];
+ };
+};
+
+/*
+ * TDX_CMD_GET_REPORT - Get TDREPORT using TDCALL[TDG.MR.REPORT)
+ *
+ * Return 0 on success, -EIO on TDCALL execution failure, and
+ * standard errno on other general error cases.
+ *
+ */
+#define TDX_CMD_GET_REPORT _IOWR('T', 0x01, struct tdx_report_req)
+
+#endif /* _UAPI_ASM_X86_TDX_H */
--
2.25.1
On Thu, May 12, 2022 at 03:19:48PM -0700, Kuppuswamy Sathyanarayanan wrote:
[snip]
> +
> +static int __init tdx_attestation_init(void)
> +{
> + long ret;
nit: the type of ret should be int
> +
> + /* Make sure we are in a valid TDX platform */
> + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> + return -EIO;
> +
> + miscdev.name = DRIVER_NAME;
> + miscdev.minor = MISC_DYNAMIC_MINOR;
> + miscdev.fops = &tdx_attest_fops;
> +
> + ret = misc_register(&miscdev);
> + if (ret) {
> + pr_err("misc device registration failed\n");
> + return ret;
> + }
> +
> + return 0;
> +}
[snip]
On 5/16/22 11:08 AM, Wander Lairson Costa wrote:
>> +
>> +static int __init tdx_attestation_init(void)
>> +{
>> + long ret;
> nit: the type of ret should be int
Agree. I will fix it in next version.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Thu, 2022-05-12 at 15:19 -0700, Kuppuswamy Sathyanarayanan wrote:
> In TDX guest, attestation is used to verify the trustworthiness of a TD
> to other entities before provisioning secrets to the TD.
>
> One usage example is, when a TD guest uses encrypted drive and if the
> decryption keys required to access the drive are stored in a secure 3rd
> party keyserver, the key server can use attestation to verify TD's
> trustworthiness and release the decryption keys to the TD.
>
> The attestation process consists of two steps: TDREPORT generation and
> Quote generation.
>
> TDREPORT (TDREPORT_STRUCT) is a fixed-size data structure generated by
> the TDX module which contains TD-specific information (such as TD
> measurements), platform security version, and the MAC to protect the
> integrity of the TDREPORT. The TD kernel uses TDCALL[TDG.MR.REPORT] to
> get the TDREPORT from the TDX module. A user-provided 64-Byte
> REPORTDATA is used as input and included in the TDREPORT. Typically it
> can be some nonce provided by attestation service so the TDREPORT can
> be verified uniquely. More details about TDREPORT can be found in
> Intel TDX Module specification, section titled "TDG.MR.REPORT Leaf".
>
> Also note that the MAC added to the TDREPORT is bound to the platform.
> So TDREPORT can only be verified locally.
>
"bound to the platform, so ...".
> Intel SGX Quote Enclave (QE)
> is leveraged to verify the TDREPORT locally and convert it to a remote
> verifiable Quote to support remote attestation of the TDREPORT.
First of all, sorry for having to modify this back and forth for multi-times.
However this sounds like besides the SGX QE, there are other ways can be
leveraged to generate the Quote. This isn't true based on current TDX
architecture.
So I still think below is slightly better:
TDREPORT can only be verified on local platform as the MAC key is bound to the
platform. To support remote verification of the TDREPORT, TDX leverages Intel
SGX Quote Enclave (QE) to verify the TDREPORT locally and convert it to a remote
verifiable Quote.
>
> After getting the TDREPORT, the second step of the attestation process
> is to send it to QE or Quote Generation Service (QGS) to generate a TD
> Quote. The QE sends the Quote back after it is generated. How is the
> data sent and received is QE implementation and deployment specific.TD
> userspace attestation software can use whatever communication channel
> available (i.e. tcp/ip, vsock) to communicate with the QE using whatever
> data format. TDX also defines TDVMCALLs to allow TD to ask VMM to
> facilitate sending/receiving data between TD attestation software and
> the QE. This support is documented in GHCI 1.0 spec "5.4 TD attestation".
This paragraph is used to provide more information to help to justify the
decision to provide a way to let userspace to get the TDREPORT. Now looks this
paragraph has details not quite related to this patch. For instance, I am not
sure whether we need to mention TDVMCALL at all.
Also, it _may_ be helpful to point out we cannot have QE inside the TD since TDX
doesn't support SGX within the TD, otherwise it's completely possible that the
TD attestation agent can just implement the QE by itself therefore doesn't need
any communication channel at all.
Anyway, perhaps just:
"
After getting the TDREPORT, the second step of the attestation process is to
send it to the QE to generate the Quote. TDX doesn't support SGX inside the TD,
so the QE can be deployed in the host, or in another legacy VM with SGX support.
How to send the TDREPORT to QE and receive the Quote is implementation and
deployment specific.
Implement a basic attestation driver to allow TD userspace to get the TDREPORT.
The TD userspace attestation software can get the TDREPORT and then choose
whatever communication channel available (i.e. vsock) to send the TDREPORT to QE
and receive the Quote.
"
Anyway I am not good at writing changelog so will leave to others in the future.
>
> Implement a basic attestation driver to allow TD userspace to get the
> TDREPORT, which is sent to QE by the attestation software to generate
> a Quote for remote verification.
>
> Also note that explicit access permissions are not enforced in this
> driver because the quote and measurements are not a secret. However
> the access permissions of the device node can be used to set any
> desired access policy. The udev default is usually root access
> only.
>
> Operations like getting TDREPORT or Quote generation involves sending
> a blob of data as input and getting another blob of data as output. It
> was considered to use a sysfs interface for this, but it doesn't fit
> well into the standard sysfs model for configuring values. It would be
> possible to do read/write on files, but it would need multiple file
> descriptors, which would be somewhat messy. IOCTLs seems to be the best
> fitting and simplest model for this use case. Also, the REPORTDATA used
> in TDREPORT generation can possibly come from attestation service to
> uniquely verify the Quote (like per instance verification). In such
> case, since REPORTDATA is a secret, using sysfs to share it is insecure
> compared to sending it via IOCTL.
>
> Reviewed-by: Tony Luck <[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/Makefile | 2 +-
> arch/x86/coco/tdx/attest.c | 118 ++++++++++++++++++++++++++++++++
> arch/x86/include/uapi/asm/tdx.h | 42 ++++++++++++
> 3 files changed, 161 insertions(+), 1 deletion(-)
> create mode 100644 arch/x86/coco/tdx/attest.c
> create mode 100644 arch/x86/include/uapi/asm/tdx.h
>
> diff --git a/arch/x86/coco/tdx/Makefile b/arch/x86/coco/tdx/Makefile
> index 46c55998557d..d2db3e6770e5 100644
> --- a/arch/x86/coco/tdx/Makefile
> +++ b/arch/x86/coco/tdx/Makefile
> @@ -1,3 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0
>
> -obj-y += tdx.o tdcall.o
> +obj-y += tdx.o tdcall.o attest.o
> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
> new file mode 100644
> index 000000000000..a5f4111f9b18
> --- /dev/null
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process.
> + *
> + * Copyright (C) 2022 Intel Corporation
> + *
> + */
> +
> +#define pr_fmt(fmt) "x86/tdx: attest: " fmt
> +
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/io.h>
> +#include <asm/tdx.h>
> +#include <uapi/asm/tdx.h>
> +
> +#define DRIVER_NAME "tdx-attest"
> +
> +/* TDREPORT module call leaf ID */
> +#define TDX_GET_REPORT 4
> +
Looks more white spaces than needed.
> +static struct miscdevice miscdev;
> +
> +static long tdx_get_report(void __user *argp)
> +{
> + void *reportdata = NULL, *tdreport = NULL;
> + long ret = 0;
> +
> + /* Allocate buffer space for REPORTDATA */
> + reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
> + if (!reportdata)
> + return -ENOMEM;
> +
> + /* Allocate buffer space for TDREPORT */
> + tdreport = kmalloc(TDX_REPORT_LEN, GFP_KERNEL);
> + if (!tdreport) {
> + ret = -ENOMEM;
> + goto out;
> + }
Perhaps you can allocate a single page, and use the first half as REPORTDATA and
the second part as TDREPORT. In this case, you can save one memory allocation
to simplify the code. The page will be freed anyway after this IOCTL.
> +
> + /* Copy REPORTDATA from the user buffer */
> + if (copy_from_user(reportdata, argp, TDX_REPORTDATA_LEN)) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + /*
> + * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
> + *
> + * Get the TDREPORT using REPORTDATA as input. Refer to
> + * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
> + * Specification for detailed information.
> + */
> + ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
> + virt_to_phys(reportdata), 0, 0, NULL);
> + if (ret) {
> + pr_debug("TDREPORT TDCALL failed, status:%lx\n", ret);
> + ret = -EIO;
> + goto out;
> + }
> +
> + /* Copy TDREPORT back to the user buffer */
> + if (copy_to_user(argp, tdreport, TDX_REPORT_LEN))
> + ret = -EFAULT;
> +
> +out:
> + kfree(reportdata);
> + kfree(tdreport);
> + return ret;
> +}
> +
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + void __user *argp = (void __user *)arg;
> + long ret = -EINVAL;
> +
> + switch (cmd) {
> + case TDX_CMD_GET_REPORT:
> + ret = tdx_get_report(argp);
> + break;
> + default:
> + pr_debug("cmd %d not supported\n", cmd);
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static const struct file_operations tdx_attest_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = tdx_attest_ioctl,
> + .llseek = no_llseek,
> +};
> +
> +static int __init tdx_attestation_init(void)
> +{
> + long ret;
> +
> + /* Make sure we are in a valid TDX platform */
> + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> + return -EIO;
> +
> + miscdev.name = DRIVER_NAME;
> + miscdev.minor = MISC_DYNAMIC_MINOR;
> + miscdev.fops = &tdx_attest_fops;
> +
> + ret = misc_register(&miscdev);
> + if (ret) {
> + pr_err("misc device registration failed\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +device_initcall(tdx_attestation_init)
> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
> new file mode 100644
> index 000000000000..e06da56058a1
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/tdx.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_ASM_X86_TDX_H
> +#define _UAPI_ASM_X86_TDX_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
> +#define TDX_REPORTDATA_LEN 64
> +
> +/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
> +#define TDX_REPORT_LEN 1024
> +
> +/**
> + * struct tdx_report_req: Get TDREPORT using REPORTDATA as input.
> + *
> + * @reportdata : User-defined 64-Byte REPORTDATA to be included into
> + * TDREPORT. Typically it can be some nonce provided by
> + * attestation software so the generated TDREPORT can be
"attestation software" -> "attestation service" or "verification service". Or
just remove the second sentence. Anyway, it's user-defined.
> + * uniquely verified.
> + * @tdreport : TDREPORT output from TDCALL[TDG.MR.REPORT] of size
> + * TDX_REPORT_LEN.
> + *
> + * Used in TDX_CMD_GET_REPORT IOCTL request.
> + */
> +struct tdx_report_req {
> + union {
> + __u8 reportdata[TDX_REPORTDATA_LEN];
> + __u8 tdreport[TDX_REPORT_LEN];
> + };
> +};
As a userspace ABI, one concern is this doesn't provide any space for future
extension. But probably it's OK since I don't see any possible additional input
for now. And although TDREPORT may have additional information in future
generation of TDX but the spec says the size is 1024 so perhaps this won't
change even in the future.
Anyway will leave to others.
> +
> +/*
> + * TDX_CMD_GET_REPORT - Get TDREPORT using TDCALL[TDG.MR.REPORT)
TDCALL[TDG.MR.REPORT) -> TDCALL[TDG.MR.REPORT]
> + *
> + * Return 0 on success, -EIO on TDCALL execution failure, and
> + * standard errno on other general error cases.
> + *
> + */
> +#define TDX_CMD_GET_REPORT _IOWR('T', 0x01, struct tdx_report_req)
> +
> +#endif /* _UAPI_ASM_X86_TDX_H */
Hi Kai,
On 5/16/22 7:54 PM, Kai Huang wrote:
> On Thu, 2022-05-12 at 15:19 -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, attestation is used to verify the trustworthiness of a TD
>> to other entities before provisioning secrets to the TD.
>>
>> One usage example is, when a TD guest uses encrypted drive and if the
>> decryption keys required to access the drive are stored in a secure 3rd
>> party keyserver, the key server can use attestation to verify TD's
>> trustworthiness and release the decryption keys to the TD.
>>
>> The attestation process consists of two steps: TDREPORT generation and
>> Quote generation.
>>
>> TDREPORT (TDREPORT_STRUCT) is a fixed-size data structure generated by
>> the TDX module which contains TD-specific information (such as TD
>> measurements), platform security version, and the MAC to protect the
>> integrity of the TDREPORT. The TD kernel uses TDCALL[TDG.MR.REPORT] to
>> get the TDREPORT from the TDX module. A user-provided 64-Byte
>> REPORTDATA is used as input and included in the TDREPORT. Typically it
>> can be some nonce provided by attestation service so the TDREPORT can
>> be verified uniquely. More details about TDREPORT can be found in
>> Intel TDX Module specification, section titled "TDG.MR.REPORT Leaf".
>>
>> Also note that the MAC added to the TDREPORT is bound to the platform.
>> So TDREPORT can only be verified locally.
>>
>
> "bound to the platform, so ...".
>
>> Intel SGX Quote Enclave (QE)
>> is leveraged to verify the TDREPORT locally and convert it to a remote
>> verifiable Quote to support remote attestation of the TDREPORT.
>
>
> First of all, sorry for having to modify this back and forth for multi-times.
>
> However this sounds like besides the SGX QE, there are other ways can be
> leveraged to generate the Quote. This isn't true based on current TDX
> architecture.
>
> So I still think below is slightly better:
>
> TDREPORT can only be verified on local platform as the MAC key is bound to the
> platform. To support remote verification of the TDREPORT, TDX leverages Intel
> SGX Quote Enclave (QE) to verify the TDREPORT locally and convert it to a remote
> verifiable Quote.
>
>>
>> After getting the TDREPORT, the second step of the attestation process
>> is to send it to QE or Quote Generation Service (QGS) to generate a TD
>> Quote. The QE sends the Quote back after it is generated. How is the
>> data sent and received is QE implementation and deployment specific.TD
>> userspace attestation software can use whatever communication channel
>> available (i.e. tcp/ip, vsock) to communicate with the QE using whatever
>> data format. TDX also defines TDVMCALLs to allow TD to ask VMM to
>> facilitate sending/receiving data between TD attestation software and
>> the QE. This support is documented in GHCI 1.0 spec "5.4 TD attestation".
>
> This paragraph is used to provide more information to help to justify the
> decision to provide a way to let userspace to get the TDREPORT. Now looks this
> paragraph has details not quite related to this patch. For instance, I am not
> sure whether we need to mention TDVMCALL at all.
>
> Also, it _may_ be helpful to point out we cannot have QE inside the TD since TDX
> doesn't support SGX within the TD, otherwise it's completely possible that the
> TD attestation agent can just implement the QE by itself therefore doesn't need
> any communication channel at all.
>
> Anyway, perhaps just:
>
> "
> After getting the TDREPORT, the second step of the attestation process is to
> send it to the QE to generate the Quote. TDX doesn't support SGX inside the TD,
> so the QE can be deployed in the host, or in another legacy VM with SGX support.
> How to send the TDREPORT to QE and receive the Quote is implementation and
> deployment specific.
>
> Implement a basic attestation driver to allow TD userspace to get the TDREPORT.
> The TD userspace attestation software can get the TDREPORT and then choose
> whatever communication channel available (i.e. vsock) to send the TDREPORT to QE
> and receive the Quote.
> "
>
> Anyway I am not good at writing changelog so will leave to others in the future.
Ok. I am fine with suggested changes. I will include them.
>
>>
>> Implement a basic attestation driver to allow TD userspace to get the
>> TDREPORT, which is sent to QE by the attestation software to generate
>> a Quote for remote verification.
>>
>> Also note that explicit access permissions are not enforced in this
>> driver because the quote and measurements are not a secret. However
>> the access permissions of the device node can be used to set any
>> desired access policy. The udev default is usually root access
>> only.
>>
>> Operations like getting TDREPORT or Quote generation involves sending
>> a blob of data as input and getting another blob of data as output. It
>> was considered to use a sysfs interface for this, but it doesn't fit
>> well into the standard sysfs model for configuring values. It would be
>> possible to do read/write on files, but it would need multiple file
>> descriptors, which would be somewhat messy. IOCTLs seems to be the best
>> fitting and simplest model for this use case. Also, the REPORTDATA used
>> in TDREPORT generation can possibly come from attestation service to
>> uniquely verify the Quote (like per instance verification). In such
>> case, since REPORTDATA is a secret, using sysfs to share it is insecure
>> compared to sending it via IOCTL.
>>
>> Reviewed-by: Tony Luck <[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/Makefile | 2 +-
>> arch/x86/coco/tdx/attest.c | 118 ++++++++++++++++++++++++++++++++
>> arch/x86/include/uapi/asm/tdx.h | 42 ++++++++++++
>> 3 files changed, 161 insertions(+), 1 deletion(-)
>> create mode 100644 arch/x86/coco/tdx/attest.c
>> create mode 100644 arch/x86/include/uapi/asm/tdx.h
>>
>> diff --git a/arch/x86/coco/tdx/Makefile b/arch/x86/coco/tdx/Makefile
>> index 46c55998557d..d2db3e6770e5 100644
>> --- a/arch/x86/coco/tdx/Makefile
>> +++ b/arch/x86/coco/tdx/Makefile
>> @@ -1,3 +1,3 @@
>> # SPDX-License-Identifier: GPL-2.0
>>
>> -obj-y += tdx.o tdcall.o
>> +obj-y += tdx.o tdcall.o attest.o
>> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
>> new file mode 100644
>> index 000000000000..a5f4111f9b18
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * attest.c - TDX guest attestation interface driver.
>> + *
>> + * Implements user interface to trigger attestation process.
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) "x86/tdx: attest: " fmt
>> +
>> +#include <linux/miscdevice.h>
>> +#include <linux/mm.h>
>> +#include <linux/io.h>
>> +#include <asm/tdx.h>
>> +#include <uapi/asm/tdx.h>
>> +
>> +#define DRIVER_NAME "tdx-attest"
>> +
>> +/* TDREPORT module call leaf ID */
>> +#define TDX_GET_REPORT 4
>> +
>
> Looks more white spaces than needed.
I left some extra space to align with future macro additions.
>
>> +static struct miscdevice miscdev;
>> +
>> +static long tdx_get_report(void __user *argp)
>> +{
>> + void *reportdata = NULL, *tdreport = NULL;
>> + long ret = 0;
>> +
>> + /* Allocate buffer space for REPORTDATA */
>> + reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
>> + if (!reportdata)
>> + return -ENOMEM;
>> +
>> + /* Allocate buffer space for TDREPORT */
>> + tdreport = kmalloc(TDX_REPORT_LEN, GFP_KERNEL);
>> + if (!tdreport) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>
> Perhaps you can allocate a single page, and use the first half as REPORTDATA and
> the second part as TDREPORT. In this case, you can save one memory allocation
> to simplify the code. The page will be freed anyway after this IOCTL.
IMO, I think it is more clear doing it this way (one for input and and
other for output). We only need ~1K(+ 64B) space for our use case. So
there is no need allocate a separate page for it. Also, it is much
easier to understand compared to allocating single buffer and
diving the buffer in half between them. So if it is not a big probelem
lets leave it this way.
>
>> +
>> + /* Copy REPORTDATA from the user buffer */
>> + if (copy_from_user(reportdata, argp, TDX_REPORTDATA_LEN)) {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> +
>> + /*
>> + * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
>> + *
>> + * Get the TDREPORT using REPORTDATA as input. Refer to
>> + * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
>> + * Specification for detailed information.
>> + */
>> + ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
>> + virt_to_phys(reportdata), 0, 0, NULL);
>> + if (ret) {
>> + pr_debug("TDREPORT TDCALL failed, status:%lx\n", ret);
>> + ret = -EIO;
>> + goto out;
>> + }
>> +
>> + /* Copy TDREPORT back to the user buffer */
>> + if (copy_to_user(argp, tdreport, TDX_REPORT_LEN))
>> + ret = -EFAULT;
>> +
>> +out:
>> + kfree(reportdata);
>> + kfree(tdreport);
>> + return ret;
>> +}
>> +
>> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + void __user *argp = (void __user *)arg;
>> + long ret = -EINVAL;
>> +
>> + switch (cmd) {
>> + case TDX_CMD_GET_REPORT:
>> + ret = tdx_get_report(argp);
>> + break;
>> + default:
>> + pr_debug("cmd %d not supported\n", cmd);
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static const struct file_operations tdx_attest_fops = {
>> + .owner = THIS_MODULE,
>> + .unlocked_ioctl = tdx_attest_ioctl,
>> + .llseek = no_llseek,
>> +};
>> +
>> +static int __init tdx_attestation_init(void)
>> +{
>> + long ret;
>> +
>> + /* Make sure we are in a valid TDX platform */
>> + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> + return -EIO;
>> +
>> + miscdev.name = DRIVER_NAME;
>> + miscdev.minor = MISC_DYNAMIC_MINOR;
>> + miscdev.fops = &tdx_attest_fops;
>> +
>> + ret = misc_register(&miscdev);
>> + if (ret) {
>> + pr_err("misc device registration failed\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +device_initcall(tdx_attestation_init)
>> diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
>> new file mode 100644
>> index 000000000000..e06da56058a1
>> --- /dev/null
>> +++ b/arch/x86/include/uapi/asm/tdx.h
>> @@ -0,0 +1,42 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef _UAPI_ASM_X86_TDX_H
>> +#define _UAPI_ASM_X86_TDX_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/ioctl.h>
>> +
>> +/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
>> +#define TDX_REPORTDATA_LEN 64
>> +
>> +/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
>> +#define TDX_REPORT_LEN 1024
>> +
>> +/**
>> + * struct tdx_report_req: Get TDREPORT using REPORTDATA as input.
>> + *
>> + * @reportdata : User-defined 64-Byte REPORTDATA to be included into
>> + * TDREPORT. Typically it can be some nonce provided by
>> + * attestation software so the generated TDREPORT can be
>
> "attestation software" -> "attestation service" or "verification service". Or
> just remove the second sentence. Anyway, it's user-defined.
I will change it to attestation service. We don't need to remove it.
Anyway more info is better.
>
>> + * uniquely verified.
>> + * @tdreport : TDREPORT output from TDCALL[TDG.MR.REPORT] of size
>> + * TDX_REPORT_LEN.
>> + *
>> + * Used in TDX_CMD_GET_REPORT IOCTL request.
>> + */
>> +struct tdx_report_req {
>> + union {
>> + __u8 reportdata[TDX_REPORTDATA_LEN];
>> + __u8 tdreport[TDX_REPORT_LEN];
>> + };
>> +};
>
> As a userspace ABI, one concern is this doesn't provide any space for future
> extension. But probably it's OK since I don't see any possible additional input
> for now. And although TDREPORT may have additional information in future
> generation of TDX but the spec says the size is 1024 so perhaps this won't
> change even in the future.
>
> Anyway will leave to others.
IMO, if the spec changes in future we can revisit it.
>
>> +
>> +/*
>> + * TDX_CMD_GET_REPORT - Get TDREPORT using TDCALL[TDG.MR.REPORT)
>
> TDCALL[TDG.MR.REPORT) -> TDCALL[TDG.MR.REPORT]
Ok.
>
>> + *
>> + * Return 0 on success, -EIO on TDCALL execution failure, and
>> + * standard errno on other general error cases.
>> + *
>> + */
>> +#define TDX_CMD_GET_REPORT _IOWR('T', 0x01, struct tdx_report_req)
>> +
>> +#endif /* _UAPI_ASM_X86_TDX_H */
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 5/22/22 7:52 PM, Kai Huang wrote:
> On Tue, 2022-05-17 at 07:54 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>> +struct tdx_report_req {
>>>> + union {
>>>> + __u8 reportdata[TDX_REPORTDATA_LEN];
>>>> + __u8 tdreport[TDX_REPORT_LEN];
>>>> + };
>>>> +};
>>>
>>> As a userspace ABI, one concern is this doesn't provide any space for future
>>> extension. But probably it's OK since I don't see any possible additional
>>> input
>>> for now. And although TDREPORT may have additional information in future
>>> generation of TDX but the spec says the size is 1024 so perhaps this won't
>>> change even in the future.
>>>
>>> Anyway will leave to others.
>>
>> IMO, if the spec changes in future we can revisit it.
>
> I don't think the problem is how to revisit _this_ ABI. The problem is, once it
> is introduced, you cannot break the ABI for the compatibility of supporting the
> userspace software written for old platforms. So basically you cannot just
> increase the TDX_REPORT_LEN to a larger value. This means if we have a larger
> than 1024B TDREPORT in future, the old userspace TD attestation software which
> uses this ABI will not work anymore on the new platforms.
>
> If we need to make sure this ABI work for _ANY_ TDX platforms, I think we either
> need to make sure TDREPORT will always be 1024B for _ANY_ TDX platforms, or we
> need to have a flexible ABI which doesn't assume TDREPORT size.
>
> For instance, we might need another IOCTL (or other interfaces such as /sysfs)
> to query the TDREPORT size, and make this IOCTL like below:
>
> struct tdx_report_req {
> __u8 reportdata[TDX_REPORTDATA_LEN];
> __u8 reserved[...];
> __u8 tdreport[0];
> };
>
> The actual TDREPORT buffer size is allocated by userspace after it queries the
> TDREPORT size.
I don't want to over design it just based on the assumption that length
will change in the future. I don't see any statement in spec supporting
the possibility of length changes. IMO, since the possibility is very
small, we don't need to overthink about it.
@maintainers, please let me know if you think otherwise.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Tue, 2022-05-17 at 07:54 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > +struct tdx_report_req {
> > > + union {
> > > + __u8 reportdata[TDX_REPORTDATA_LEN];
> > > + __u8 tdreport[TDX_REPORT_LEN];
> > > + };
> > > +};
> >
> > As a userspace ABI, one concern is this doesn't provide any space for future
> > extension. But probably it's OK since I don't see any possible additional
> > input
> > for now. And although TDREPORT may have additional information in future
> > generation of TDX but the spec says the size is 1024 so perhaps this won't
> > change even in the future.
> >
> > Anyway will leave to others.
>
> IMO, if the spec changes in future we can revisit it.
I don't think the problem is how to revisit _this_ ABI. The problem is, once it
is introduced, you cannot break the ABI for the compatibility of supporting the
userspace software written for old platforms. So basically you cannot just
increase the TDX_REPORT_LEN to a larger value. This means if we have a larger
than 1024B TDREPORT in future, the old userspace TD attestation software which
uses this ABI will not work anymore on the new platforms.
If we need to make sure this ABI work for _ANY_ TDX platforms, I think we either
need to make sure TDREPORT will always be 1024B for _ANY_ TDX platforms, or we
need to have a flexible ABI which doesn't assume TDREPORT size.
For instance, we might need another IOCTL (or other interfaces such as /sysfs)
to query the TDREPORT size, and make this IOCTL like below:
struct tdx_report_req {
__u8 reportdata[TDX_REPORTDATA_LEN];
__u8 reserved[...];
__u8 tdreport[0];
};
The actual TDREPORT buffer size is allocated by userspace after it queries the
TDREPORT size.
--
Thanks,
-Kai