TDX guest supports encrypted disk as root or secondary drives.
Decryption keys required to access such drives are usually maintained
by 3rd party key servers. Attestation is required by 3rd party key
servers to get the key for an encrypted disk volume, or possibly other
encrypted services. Attestation is used to prove to the key server that
the TD guest is running in a valid TD and the kernel and virtual BIOS
and other environment are secure.
During the boot process various components before the kernel accumulate
hashes in the TDX module, which can then combined into a report. This
would typically include a hash of the bios, bios configuration, boot
loader, command line, kernel, initrd. After checking the hashes the
key server will securely release the keys.
The actual details of the attestation protocol depend on the particular
key server configuration, but some parts are common and need to
communicate with the TDX module.
This communication is implemented in the attestation driver.
The supported steps are:
1. TD guest generates the TDREPORT that contains version information
about the Intel TDX module, measurement of the TD, along with a
TD-specified nonce.
2. TD guest shares the TDREPORT with TD host via GetQuote hypercall
which is used by the host to generate a quote via quoting
enclave (QE).
3. Quote generation completion notification is sent to TD OS via
callback interrupt vector configured by TD using
SetupEventNotifyInterrupt hypercall.
4. After receiving the generated TDQUOTE, a remote verifier can be
used to verify the quote and confirm the trustworthiness of the
TD.
Attestation agent uses IOCTLs implemented by the attestation driver to
complete the various steps of the attestation process.
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.
TDX_CMD_GEN_QUOTE IOCTL can be used to create an computation on the
host, but TDX assumes that the host is able to deal with malicious
guest flooding it anyways.
The interaction with the TDX module is like a RPM protocol here. There
are several operations (get tdreport, get quote) that need to input a
blob, and then output another blob. 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
here. There is one ioctl per operation, that takes the input blob and
returns the output blob, and as well as auxiliary ioctls to return the
blob lengths. The ioctls are documented in the header file.
[Chenyi Qiang: Proposed struct tdx_gen_quote for passing user buffer]
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Acked-by: Kirill A. Shutemov <[email protected]>
Acked-by: Hans de Goede <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
Changes since v2:
* Combined attestation related global variables into
struct attest_dev to make the code clean.
* Added support to pass TDREPORT and GetQuote TDCALLs
error code back to user on failure.
* Modified the driver to use platform device driver
model and added check to ensure only one device is
allowed.
drivers/platform/x86/intel/Kconfig | 2 +-
drivers/platform/x86/intel/Makefile | 1 +
drivers/platform/x86/intel/tdx/Kconfig | 13 +
drivers/platform/x86/intel/tdx/Makefile | 3 +
.../platform/x86/intel/tdx/intel_tdx_attest.c | 302 ++++++++++++++++++
include/uapi/misc/tdx.h | 42 +++
6 files changed, 362 insertions(+), 1 deletion(-)
create mode 100644 drivers/platform/x86/intel/tdx/Kconfig
create mode 100644 drivers/platform/x86/intel/tdx/Makefile
create mode 100644 drivers/platform/x86/intel/tdx/intel_tdx_attest.c
create mode 100644 include/uapi/misc/tdx.h
diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig
index 1f01a8a23c57..a2e2a5a29bde 100644
--- a/drivers/platform/x86/intel/Kconfig
+++ b/drivers/platform/x86/intel/Kconfig
@@ -12,7 +12,7 @@ source "drivers/platform/x86/intel/speed_select_if/Kconfig"
source "drivers/platform/x86/intel/telemetry/Kconfig"
source "drivers/platform/x86/intel/wmi/Kconfig"
source "drivers/platform/x86/intel/uncore-frequency/Kconfig"
-
+source "drivers/platform/x86/intel/tdx/Kconfig"
config INTEL_HID_EVENT
tristate "Intel HID Event"
diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile
index c61bc3e97121..6b7c94051519 100644
--- a/drivers/platform/x86/intel/Makefile
+++ b/drivers/platform/x86/intel/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_INTEL_SKL_INT3472) += int3472/
obj-$(CONFIG_INTEL_PMC_CORE) += pmc/
obj-$(CONFIG_INTEL_PMT_CLASS) += pmt/
obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += speed_select_if/
+obj-$(CONFIG_INTEL_TDX_GUEST) += tdx/
obj-$(CONFIG_INTEL_TELEMETRY) += telemetry/
obj-$(CONFIG_INTEL_WMI) += wmi/
obj-$(CONFIG_INTEL_UNCORE_FREQ_CONTROL) += uncore-frequency/
diff --git a/drivers/platform/x86/intel/tdx/Kconfig b/drivers/platform/x86/intel/tdx/Kconfig
new file mode 100644
index 000000000000..332a10313b49
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# X86 TDX Platform Specific Drivers
+#
+
+config INTEL_TDX_ATTESTATION
+ tristate "Intel TDX attestation driver"
+ depends on INTEL_TDX_GUEST
+ help
+ The TDX attestation driver provides IOCTL interfaces to the user to
+ request TDREPORT from the TDX module or request quote from the VMM
+ or to get quote buffer size. It is mainly used to get secure disk
+ decryption keys from the key server.
diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
new file mode 100644
index 000000000000..94eea6108fbd
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_INTEL_TDX_ATTESTATION) += intel_tdx_attest.o
diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
new file mode 100644
index 000000000000..9124db800d4f
--- /dev/null
+++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel_tdx_attest.c - TDX guest attestation interface driver.
+ *
+ * Implements user interface to trigger attestation process and
+ * read the TD Quote result.
+ *
+ * Copyright (C) 2021-2022 Intel Corporation
+ *
+ * Author:
+ * Kuppuswamy Sathyanarayanan <[email protected]>
+ */
+
+#define pr_fmt(fmt) "x86/tdx: attest: " fmt
+
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/uaccess.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/set_memory.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/jiffies.h>
+#include <linux/io.h>
+#include <asm/apic.h>
+#include <asm/tdx.h>
+#include <asm/irq_vectors.h>
+#include <uapi/misc/tdx.h>
+
+#define DRIVER_NAME "tdx-attest"
+
+/* Used in Quote memory allocation */
+#define QUOTE_SIZE (2 * PAGE_SIZE)
+/* Used in Get Quote request memory allocation */
+#define GET_QUOTE_MAX_SIZE (4 * PAGE_SIZE)
+/* Get Quote timeout in msec */
+#define GET_QUOTE_TIMEOUT (5000)
+
+struct attest_dev {
+ /* Mutex to serialize attestation requests */
+ struct mutex lock;
+ /* Completion object to track GetQuote completion status */
+ struct completion req_compl;
+ /* Buffer used to copy report data in attestation handler */
+ u8 report_buf[TDX_REPORT_DATA_LEN] __aligned(64);
+ /* Data pointer used to get TD Quote data in attestation handler */
+ void *tdquote_buf;
+ /* Data pointer used to get TDREPORT data in attestation handler */
+ void *tdreport_buf;
+ /* DMA handle used to allocate and free tdquote DMA buffer */
+ dma_addr_t handle;
+ struct miscdevice miscdev;
+};
+
+static struct platform_device *pdev;
+
+static void attestation_callback_handler(void)
+{
+ struct attest_dev *adev = platform_get_drvdata(pdev);
+
+ complete(&adev->req_compl);
+}
+
+static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct attest_dev *adev = platform_get_drvdata(pdev);
+ void __user *argp = (void __user *)arg;
+ struct tdx_gen_quote tdquote_req;
+ long ret = 0, err;
+
+ mutex_lock(&adev->lock);
+
+ switch (cmd) {
+ case TDX_CMD_GET_TDREPORT:
+ if (copy_from_user(adev->report_buf, argp,
+ TDX_REPORT_DATA_LEN)) {
+ ret = -EFAULT;
+ break;
+ }
+
+ /* Generate TDREPORT_STRUCT */
+ err = tdx_mcall_tdreport(adev->tdreport_buf, adev->report_buf);
+ if (err) {
+ ret = put_user(err, (long __user *)argp);
+ ret = -EIO;
+ break;
+ }
+
+ if (copy_to_user(argp, adev->tdreport_buf, TDX_TDREPORT_LEN))
+ ret = -EFAULT;
+ break;
+ case TDX_CMD_GEN_QUOTE:
+ reinit_completion(&adev->req_compl);
+
+ /* Copy TDREPORT data from user buffer */
+ if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) {
+ ret = -EFAULT;
+ break;
+ }
+
+ if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) {
+ ret = -EINVAL;
+ break;
+ }
+
+ if (copy_from_user(adev->tdquote_buf, (void __user *)tdquote_req.buf,
+ tdquote_req.len)) {
+ ret = -EFAULT;
+ break;
+ }
+
+ /* Submit GetQuote Request */
+ err = tdx_hcall_get_quote(adev->tdquote_buf, GET_QUOTE_MAX_SIZE);
+ if (err) {
+ ret = put_user(err, (long __user *)argp);
+ ret = -EIO;
+ break;
+ }
+
+ /* Wait for attestation completion */
+ ret = wait_for_completion_interruptible_timeout(
+ &adev->req_compl,
+ msecs_to_jiffies(GET_QUOTE_TIMEOUT));
+ if (ret <= 0) {
+ ret = -EIO;
+ break;
+ }
+
+ /* ret will be positive if completed. */
+ ret = 0;
+
+ if (copy_to_user((void __user *)tdquote_req.buf, adev->tdquote_buf,
+ tdquote_req.len))
+ ret = -EFAULT;
+
+ break;
+ case TDX_CMD_GET_QUOTE_SIZE:
+ ret = put_user(QUOTE_SIZE, (u64 __user *)argp);
+ break;
+ default:
+ pr_err("cmd %d not supported\n", cmd);
+ break;
+ }
+
+ mutex_unlock(&adev->lock);
+
+ return ret;
+}
+
+static const struct file_operations tdx_attest_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = tdx_attest_ioctl,
+ .llseek = no_llseek,
+};
+
+/* Helper function to cleanup attestation related allocations */
+static void _tdx_attest_remove(struct attest_dev *adev)
+{
+ misc_deregister(&adev->miscdev);
+
+ tdx_remove_ev_notify_handler();
+
+ if (adev->tdquote_buf)
+ dma_free_coherent(&pdev->dev, GET_QUOTE_MAX_SIZE,
+ adev->tdquote_buf, adev->handle);
+
+ if (adev->tdreport_buf)
+ free_pages((unsigned long)adev->tdreport_buf, 0);
+
+ kfree(adev);
+}
+
+static int tdx_attest_probe(struct platform_device *attest_pdev)
+{
+ struct device *dev = &attest_pdev->dev;
+ struct attest_dev *adev;
+ long ret = 0;
+
+ /* Only single device is allowed */
+ if (pdev)
+ return -EBUSY;
+
+ adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+ if (!adev)
+ return -ENOMEM;
+
+ mutex_init(&adev->lock);
+ init_completion(&adev->req_compl);
+ pdev = attest_pdev;
+ platform_set_drvdata(pdev, adev);
+
+ /*
+ * tdreport_data needs to be 64-byte aligned.
+ * Full page alignment is more than enough.
+ */
+ adev->tdreport_buf = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+ 0);
+ if (!adev->tdreport_buf) {
+ ret = -ENOMEM;
+ goto failed;
+ }
+
+ ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+ if (ret) {
+ pr_err("dma set coherent mask failed\n");
+ goto failed;
+ }
+
+ /* Allocate DMA buffer to get TDQUOTE data from the VMM */
+ adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE,
+ &adev->handle,
+ GFP_KERNEL | __GFP_ZERO);
+ if (!adev->tdquote_buf) {
+ ret = -ENOMEM;
+ goto failed;
+ }
+
+ /* Register attestation event notify handler */
+ tdx_setup_ev_notify_handler(attestation_callback_handler);
+
+ adev->miscdev.name = DRIVER_NAME;
+ adev->miscdev.minor = MISC_DYNAMIC_MINOR;
+ adev->miscdev.fops = &tdx_attest_fops;
+ adev->miscdev.parent = dev;
+
+ ret = misc_register(&adev->miscdev);
+ if (ret) {
+ pr_err("misc device registration failed\n");
+ goto failed;
+ }
+
+ pr_debug("module initialization success\n");
+
+ return 0;
+
+failed:
+ _tdx_attest_remove(adev);
+
+ pr_debug("module initialization failed\n");
+
+ return ret;
+}
+
+static int tdx_attest_remove(struct platform_device *attest_pdev)
+{
+ struct attest_dev *adev = platform_get_drvdata(attest_pdev);
+
+ mutex_lock(&adev->lock);
+ _tdx_attest_remove(adev);
+ mutex_unlock(&adev->lock);
+ pr_debug("module is successfully removed\n");
+ return 0;
+}
+
+static struct platform_driver tdx_attest_driver = {
+ .probe = tdx_attest_probe,
+ .remove = tdx_attest_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ },
+};
+
+static int __init tdx_attest_init(void)
+{
+ int ret;
+
+ /* Make sure we are in a valid TDX platform */
+ if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return -EIO;
+
+ ret = platform_driver_register(&tdx_attest_driver);
+ if (ret) {
+ pr_err("failed to register driver, err=%d\n", ret);
+ return ret;
+ }
+
+ pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
+ if (IS_ERR(pdev)) {
+ ret = PTR_ERR(pdev);
+ pr_err("failed to allocate device, err=%d\n", ret);
+ platform_driver_unregister(&tdx_attest_driver);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit tdx_attest_exit(void)
+{
+ platform_device_unregister(pdev);
+ platform_driver_unregister(&tdx_attest_driver);
+}
+
+module_init(tdx_attest_init);
+module_exit(tdx_attest_exit);
+
+MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <[email protected]>");
+MODULE_DESCRIPTION("TDX attestation driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/misc/tdx.h b/include/uapi/misc/tdx.h
new file mode 100644
index 000000000000..9920f36c79fe
--- /dev/null
+++ b/include/uapi/misc/tdx.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_MISC_TDX_H
+#define _UAPI_MISC_TDX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */
+#define TDX_REPORT_DATA_LEN 64
+
+/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */
+#define TDX_TDREPORT_LEN 1024
+
+/*
+ * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX
+ * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes
+ * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful
+ * TDREPORT data is copied to the user buffer.
+ */
+#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.
+ */
+#define TDX_CMD_GEN_QUOTE _IOR('T', 0x02, __u64)
+
+/*
+ * TDX_CMD_GET_QUOTE_SIZE IOCTL is used to get the TD Quote size info in bytes.
+ * This will be used for determining the input buffer allocation size when
+ * using TDX_CMD_GEN_QUOTE IOCTL.
+ */
+#define TDX_CMD_GET_QUOTE_SIZE _IOR('T', 0x03, __u64)
+
+struct tdx_gen_quote {
+ __u64 buf;
+ __u64 len;
+};
+
+#endif /* _UAPI_MISC_TDX_H */
--
2.25.1
On 4/19/22 1:13 AM, Borislav Petkov wrote:
> On Tue, Apr 19, 2022 at 07:47:33PM +1200, Kai Huang wrote:
>> From this perspective, I am not sure what's the value of having a dedicated
>> INTEL_TDX_ATTESTATION Kconfig. The attestation support code should be turned on
>> unconditionally when CONFIG_INTEL_TDX_GUEST is on. The code can also be just
>> under arch/x86/coco/tdx/ I guess?
>>
>> But I'll leave this to maintainers.
>
> Similar story with the unaccepted memory gunk. If it is not going to
> be used outside of encrypted guests, why are we polluting our already
> insanely humongous Kconfig space with more symbols?
>
Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION.
Boris, this is a simple platform driver which adds IOCTL interfaces to
allow user space to get TDREPORT and TDQuote support.
So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ?
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Tue, Apr 19, 2022 at 07:47:33PM +1200, Kai Huang wrote:
> From this perspective, I am not sure what's the value of having a dedicated
> INTEL_TDX_ATTESTATION Kconfig. The attestation support code should be turned on
> unconditionally when CONFIG_INTEL_TDX_GUEST is on. The code can also be just
> under arch/x86/coco/tdx/ I guess?
>
> But I'll leave this to maintainers.
Similar story with the unaccepted memory gunk. If it is not going to
be used outside of encrypted guests, why are we polluting our already
insanely humongous Kconfig space with more symbols?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/19/22 7:13 AM, Dave Hansen wrote:
>> >From this perspective, I am not sure what's the value of having a dedicated
>> INTEL_TDX_ATTESTATION Kconfig. The attestation support code should be turned on
>> unconditionally when CONFIG_INTEL_TDX_GUEST is on. The code can also be just
>> under arch/x86/coco/tdx/ I guess?
> How much code are we talking about? What's the difference in the size
> of the binaries with this compiled in?
Current driver size is ~300 lines. It adds ~500 bytes to the kernel
binary if it is built-in.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Tue, 2022-04-19 at 19:47 +1200, Kai Huang wrote:
> On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# X86 TDX Platform Specific Drivers
> > +#
> > +
> > +config INTEL_TDX_ATTESTATION
> > + tristate "Intel TDX attestation driver"
> > + depends on INTEL_TDX_GUEST
> > + help
> > + The TDX attestation driver provides IOCTL interfaces to the user to
> > + request TDREPORT from the TDX module or request quote from the VMM
> > + or to get quote buffer size. It is mainly used to get secure disk
> > + decryption keys from the key server.
> > diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
> > new file mode 100644
> > index 000000000000..94eea6108fbd
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_INTEL_TDX_ATTESTATION) += intel_tdx_attest.o
> > diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> > new file mode 100644
> > index 000000000000..9124db800d4f
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
>
>
> From security's perspective, attestation is an essential part of TDX. That
> being said, w/o attestation support in TD guest, I guess nobody will seriously
> use TD guest.
>
> From this perspective, I am not sure what's the value of having a dedicated
> INTEL_TDX_ATTESTATION Kconfig. The attestation support code should be turned on
> unconditionally when CONFIG_INTEL_TDX_GUEST is on. The code can also be just
> under arch/x86/coco/tdx/ I guess?
>
> But I'll leave this to maintainers.
In fact after slightly thinking more, I think you can split TDREPORT TDCALL
support with GetQuote/SetupEventNotifyInterrupt support. The reason is as I
said, GetQuote isn't mandatory to support attestation. TD attestation agent can
use i.e. vsock, tcp/ip, to communicate to QE directly. Whether kernel needs to
support GetQuote is actually arguable.
So IMHO you can split this attestation driver into two parts:
1) A "basic" driver which supports reporting TDREPORT to userspace
2) Additional support of GetQuote/SetupEventNotifyInterrupt.
The 1) can even be in a single patch (I guess it won't be complicated). It is
easy to review (and i.e. can be merged separately), and with it, you will
immediately have one way to support attestation.
2) can be reviewed separately, perhaps with one additional Kconfig option (i.e.
CONFIG_INTEL_TDX_ATTESTATION_GET_QUOTE). I think this part has most of the
complexity things in terms of review.
--
Thanks,
-Kai
On 4/19/22 1:16 AM, Kai Huang wrote:
> In fact after slightly thinking more, I think you can split TDREPORT TDCALL
> support with GetQuote/SetupEventNotifyInterrupt support. The reason is as I
> said, GetQuote isn't mandatory to support attestation. TD attestation agent can
> use i.e. vsock, tcp/ip, to communicate to QE directly. Whether kernel needs to
> support GetQuote is actually arguable.
IMO, we should not use a usage model to categorize "GetQuote" support
as a mandatory or non-mandatory requirement.
For customers who use VSOCK, they can get away without GetQuote
TDVMCALL support. But for customers who do not want to use
VSOCK model, this is a required support. AFAIK, our current customer
requirement is to use TDVMCALL approach for attestation support.
If your suggestion is to split GetQuote support as separate
patch to make it easier for review, I am fine with such
suggestion.
Maintainers, any opinion? Would you prefer to split the
driver into two patches?
>
> So IMHO you can split this attestation driver into two parts:
>
> 1) A "basic" driver which supports reporting TDREPORT to userspace
> 2) Additional support of GetQuote/SetupEventNotifyInterrupt.
>
> The 1) can even be in a single patch (I guess it won't be complicated). It is
> easy to review (and i.e. can be merged separately), and with it, you will
> immediately have one way to support attestation.
>
> 2) can be reviewed separately, perhaps with one additional Kconfig option (i.e.
> CONFIG_INTEL_TDX_ATTESTATION_GET_QUOTE). I think this part has most of the
GetQuote IOCTL support is a very simple feature support, so, IMO, we
don't need to complicate it with additional config.
> complexity things in terms of review.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Fri, Apr 15, 2022 at 03:01:09PM -0700,
Kuppuswamy Sathyanarayanan <[email protected]> wrote:
...
> diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> new file mode 100644
> index 000000000000..9124db800d4f
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
...
> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct attest_dev *adev = platform_get_drvdata(pdev);
> + void __user *argp = (void __user *)arg;
> + struct tdx_gen_quote tdquote_req;
> + long ret = 0, err;
> +
> + mutex_lock(&adev->lock);
> +
> + switch (cmd) {
> + case TDX_CMD_GET_TDREPORT:
> + if (copy_from_user(adev->report_buf, argp,
> + TDX_REPORT_DATA_LEN)) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + /* Generate TDREPORT_STRUCT */
> + err = tdx_mcall_tdreport(adev->tdreport_buf, adev->report_buf);
> + if (err) {
> + ret = put_user(err, (long __user *)argp);
> + ret = -EIO;
> + break;
> + }
> +
> + if (copy_to_user(argp, adev->tdreport_buf, TDX_TDREPORT_LEN))
> + ret = -EFAULT;
> + break;
> + case TDX_CMD_GEN_QUOTE:
> + reinit_completion(&adev->req_compl);
> +
> + /* Copy TDREPORT data from user buffer */
> + if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (copy_from_user(adev->tdquote_buf, (void __user *)tdquote_req.buf,
> + tdquote_req.len)) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + /* Submit GetQuote Request */
> + err = tdx_hcall_get_quote(adev->tdquote_buf, GET_QUOTE_MAX_SIZE);
> + if (err) {
> + ret = put_user(err, (long __user *)argp);
> + ret = -EIO;
> + break;
> + }
> +
> + /* Wait for attestation completion */
> + ret = wait_for_completion_interruptible_timeout(
> + &adev->req_compl,
> + msecs_to_jiffies(GET_QUOTE_TIMEOUT));
If timeout occurs, the state of adev->tdquote_buf is unknown. It's not safe
to continue to using adev->tdquote_buf. VMM would continue to processing
getquote request with this buffer. What if TDX_CMD_GEN_QUOTE is issued again,
and tdquote_buf is re-used?
--
Isaku Yamahata <[email protected]>
On 4/19/22 7:24 AM, Dave Hansen wrote:
>> Current driver size is ~300 lines. It adds ~500 bytes to the kernel
>> binary if it is built-in.
> That doesn't sound like good use of a Kconfig option to me. Just
> explain in the cover letter:
>
> Any distribution enabling TDX is also expected to need
> attestation. The compiled size is quite small (500 bytes).
Ok. I will add it.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote:
> On 4/19/22 00:47, Kai Huang wrote:
> > > From security's perspective, attestation is an essential part of TDX. That
> > being said, w/o attestation support in TD guest, I guess nobody will seriously
> > use TD guest.
>
> Are you saying you can't think of a single threat model where there's a
> benefit to running a TDX guest without attestation? Will TDX only be
> used in environments where secrets are provisioned to guests on the
> basis of attestation?
>
> >
I don't think anyone should provision secret to a TD before it get attested that
it is a genuine TD that he/she expected. If someone does that, he/she takes the
risk of losing the secret. Of course if someone just want to try a TD then w/o
attestation is totally fine.
--
Thanks,
-Kai
On Tue, Apr 19, 2022 at 05:48:57AM -0700, Sathyanarayanan Kuppuswamy wrote:
> Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION.
Sounds like you didn't read my mail. Kai was questioning the need for a
Kconfig symbol at all. And me too.
If this thing is not going to be used by anything else besides TDX
guests, why does it need a Kconfig option at all?!
> Boris, this is a simple platform driver which adds IOCTL interfaces to allow
> user space to get TDREPORT and TDQuote support.
>
> So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ?
See above.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/19/22 15:21, Kai Huang wrote:
> On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote:
>> On 4/19/22 00:47, Kai Huang wrote:
>>>> From security's perspective, attestation is an essential part of TDX. That
>>> being said, w/o attestation support in TD guest, I guess nobody will seriously
>>> use TD guest.
>> Are you saying you can't think of a single threat model where there's a
>> benefit to running a TDX guest without attestation? Will TDX only be
>> used in environments where secrets are provisioned to guests on the
>> basis of attestation?
>>
> I don't think anyone should provision secret to a TD before it get attested that
> it is a genuine TD that he/she expected. If someone does that, he/she takes the
> risk of losing the secret. Of course if someone just want to try a TD then w/o
> attestation is totally fine.
Yeah, but you said:
w/o attestation support in TD guest, I guess nobody will
seriously use TD guest.
I'm trying to get to the bottom of that. That's a much more broad
statement than something about when it's safe to deploy secrets.
There are lots of secrets deployed in (serious) VMs today. There are
lots of secrets deployed in (serious) SEV VMs that don't have
attestation. Yet, the world somehow hasn't come crashing down.
I think it's crazy to say that nobody will deploy secrets to TDX VMs
without attestation. I think it's a step father into crazy land to say
that no one will "seriously" use TDX guests without attestation.
Let's be honest about this and not live in some fantasy world, please.
On 4/19/22 00:47, Kai Huang wrote:
>>From security's perspective, attestation is an essential part of TDX. That
> being said, w/o attestation support in TD guest, I guess nobody will seriously
> use TD guest.
Are you saying you can't think of a single threat model where there's a
benefit to running a TDX guest without attestation? Will TDX only be
used in environments where secrets are provisioned to guests on the
basis of attestation?
>>From this perspective, I am not sure what's the value of having a dedicated
> INTEL_TDX_ATTESTATION Kconfig. The attestation support code should be turned on
> unconditionally when CONFIG_INTEL_TDX_GUEST is on. The code can also be just
> under arch/x86/coco/tdx/ I guess?
How much code are we talking about? What's the difference in the size
of the binaries with this compiled in?
On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# X86 TDX Platform Specific Drivers
> +#
> +
> +config INTEL_TDX_ATTESTATION
> + tristate "Intel TDX attestation driver"
> + depends on INTEL_TDX_GUEST
> + help
> + The TDX attestation driver provides IOCTL interfaces to the user to
> + request TDREPORT from the TDX module or request quote from the VMM
> + or to get quote buffer size. It is mainly used to get secure disk
> + decryption keys from the key server.
> diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
> new file mode 100644
> index 000000000000..94eea6108fbd
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_INTEL_TDX_ATTESTATION) += intel_tdx_attest.o
> diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> new file mode 100644
> index 000000000000..9124db800d4f
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
From security's perspective, attestation is an essential part of TDX. That
being said, w/o attestation support in TD guest, I guess nobody will seriously
use TD guest.
From this perspective, I am not sure what's the value of having a dedicated
INTEL_TDX_ATTESTATION Kconfig. The attestation support code should be turned on
unconditionally when CONFIG_INTEL_TDX_GUEST is on. The code can also be just
under arch/x86/coco/tdx/ I guess?
But I'll leave this to maintainers.
Hi Boris,
On 4/20/22 3:00 PM, Borislav Petkov wrote:
> On Tue, Apr 19, 2022 at 05:48:57AM -0700, Sathyanarayanan Kuppuswamy wrote:
>> Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION.
>
> Sounds like you didn't read my mail. Kai was questioning the need for a
> Kconfig symbol at all. And me too.
>
> If this thing is not going to be used by anything else besides TDX
> guests, why does it need a Kconfig option at all?!
Sorry, it is a typo. I meant we can just compile it with
TDX guest config (CONFIG_INTEL_TDX_GUEST).
>
>> Boris, this is a simple platform driver which adds IOCTL interfaces to allow
>> user space to get TDREPORT and TDQuote support.
>>
>> So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ?
>
> See above.
I agree with dropping the new CONFIG option. But regarding driver
location, which is preferred (platform/x86/intel/tdx or arch/x86/coco/tdx/)?
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Tue, 2022-04-19 at 07:00 -0700, Sathyanarayanan Kuppuswamy wrote:
>
> On 4/19/22 1:16 AM, Kai Huang wrote:
> > In fact after slightly thinking more, I think you can split TDREPORT TDCALL
> > support with GetQuote/SetupEventNotifyInterrupt support. The reason is as I
> > said, GetQuote isn't mandatory to support attestation. TD attestation agent can
> > use i.e. vsock, tcp/ip, to communicate to QE directly. Whether kernel needs to
> > support GetQuote is actually arguable.
>
> IMO, we should not use a usage model to categorize "GetQuote" support
> as a mandatory or non-mandatory requirement.
>
> For customers who use VSOCK, they can get away without GetQuote
> TDVMCALL support. But for customers who do not want to use
> VSOCK model, this is a required support. AFAIK, our current customer
> requirement is to use TDVMCALL approach for attestation support.
>
> If your suggestion is to split GetQuote support as separate
> patch to make it easier for review, I am fine with such
> suggestion.
>
I am not saying we should get rid of GetQuote support. If there's customer
wants this with a good reason, we can certainly support it. I understand that
some customer wants to deploy QE in host and don't want additional communication
channel (i.e. vsock) between guest and host, which may add additional attack
window and/or customer's validation resource.
My point is regardless whether we need to support GetQuote, logically this
driver can be split to two parts as I said: 1) basic TDREPORT support to
userspace; 2) additional GetQuote support. And I think there are many benefits
if you do in this way as I commented below.
> >
> > So IMHO you can split this attestation driver into two parts:
> >
> > 1) A "basic" driver which supports reporting TDREPORT to userspace
> > 2) Additional support of GetQuote/SetupEventNotifyInterrupt.
> >
> > The 1) can even be in a single patch (I guess it won't be complicated). It is
> > easy to review (and i.e. can be merged separately), and with it, you will
> > immediately have one way to support attestation.
> >
> > 2) can be reviewed separately, perhaps with one additional Kconfig option (i.e.
> > CONFIG_INTEL_TDX_ATTESTATION_GET_QUOTE). I think this part has most of the
>
>
> GetQuote IOCTL support is a very simple feature support, so, IMO, we
> don't need to complicate it with additional config.
>
> >
Additional Kconfig can reduce attack window by turning it off for people don't
need it. Anyway no strong opinion here.
--
Thanks,
-Kai
Hi,
On 4/20/22 4:18 PM, Kai Huang wrote:
> On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
>> TDX guest supports encrypted disk as root or secondary drives.
>> Decryption keys required to access such drives are usually maintained
>> by 3rd party key servers. Attestation is required by 3rd party key
>> servers to get the key for an encrypted disk volume, or possibly other
>> encrypted services. Attestation is used to prove to the key server that
>> the TD guest is running in a valid TD and the kernel and virtual BIOS
>> and other environment are secure.
>>
>> During the boot process various components before the kernel accumulate
>> hashes in the TDX module, which can then combined into a report. This
>> would typically include a hash of the bios, bios configuration, boot
>> loader, command line, kernel, initrd. After checking the hashes the
>> key server will securely release the keys.
>>
>> The actual details of the attestation protocol depend on the particular
>> key server configuration, but some parts are common and need to
>> communicate with the TDX module.
>
> As we discussed "key provisioning from key server to support disk decryption" is
> only one use case of attestation, so I don't think you need 3 paragraphs to talk
> about details of this use case here. The attestation flow is documented in GHCI
Not everyone understands the attestation context and usage. So I have
attempted to explain one of the use-case in detail.
> so it's clear. The attestation flow (and what this patch does) does not have
> any direct relation to the "disk decryption" details above. I think you can
> discard above entirely or using one or two simple sentences to explain.
Ok. I will try to summarize the details in few lines
>
> Also, as you agreed you will remove the 8K shared memory assumption:
>
> https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/
Yes. I have already removed this restriction. This will be part of my
next submission.
>
> and if you agree with my approach (again, I recommend) to split the driver to
> two parts (reorganize your patches essentially):
Yes. I have moved the GetQuote support to a new patch (but without new
config).
>
> https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/#m9e3c5115df0be0b53d41f987e1eda1715255d1d8
>
> I'll review again once you finish updating the driver.
Thanks.
>
> Btw some minor comments below.
>
>
> [...]
>
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
>
> Perhaps attest.c is enough, no matter where the file will reside.
Noted. I will change it.
>
>> @@ -0,0 +1,302 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * intel_tdx_attest.c - TDX guest attestation interface driver.
>> + *
>> + * Implements user interface to trigger attestation process and
>> + * read the TD Quote result.
>> + *
>> + * Copyright (C) 2021-2022 Intel Corporation
>
> For upstream I guess just need 2022.
>
> [...]
Noted. I will change it.
>
>> +struct attest_dev {
>> + /* Mutex to serialize attestation requests */
>> + struct mutex lock;
>
> I think we need a comment to explain why the driver doesn't support multiple
> GetQuote requests in parallel. In fact the updated GHCI spec doesn't explicitly
> say GetQuote cannot be done in parallel. It has a new "GET_QUOTE_IN_FLIGHT"
> flag introduced, which can be used to determine which Quote is finished I think.
>
> I am fine with only supporting GetQuote in serialized way, but perhaps we need
> to call out the reason somewhere.
If we want to support multiple GetQuote requests in parallel, then we
need some way to uniquely identify the GetQuote requests. So that when
we get completion notification, we can understand which request is
completed. This part is not mentioned/discussed in ABI spec. So we want
to serialize the requests for now.
I will include the details about it in the commit log.
>
> [...]
>
>> +
>> + /* Allocate DMA buffer to get TDQUOTE data from the VMM */
>> + adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE,
>> + &adev->handle,
>> + GFP_KERNEL | __GFP_ZERO);
>> + if (!adev->tdquote_buf) {
>> + ret = -ENOMEM;
>> + goto failed;
>> + }
>
> The buffer needs to be shared. Guest must have called MapGPA to convert the
> buffer to shared. Is this guaranteed by calling dma_alloc_coherent(), since
> seems I didn't see any MapGPA in the driver? Anyway this deserves a comment I
> think.
Yes. It is taken care by dma_alloc_*() API. DMA memory is marked by
default as shared in TDX guest. I will add comment about it.
>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Wed, 2022-04-20 at 23:57 -0700, Isaku Yamahata wrote:
> On Wed, Apr 20, 2022 at 07:42:06PM -0700,
> Sathyanarayanan Kuppuswamy <[email protected]> wrote:
>
> >
> >
> > On 4/20/22 5:11 PM, Kai Huang wrote:
> > > On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > > If we want to support multiple GetQuote requests in parallel, then we
> > > > need some way to uniquely identify the GetQuote requests. So that when
> > > > we get completion notification, we can understand which request is
> > > > completed. This part is not mentioned/discussed in ABI spec. So we want
> > > > to serialize the requests for now.
> > > >
> > >
> > > Yes it's unfortunate that this part (whether concurrent GetQuote requests are
> > > supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am
> > > fine with only supporting GetQuote requests one by one. AFAICT there's no
> > > request to support concurrent GetQuote requests anyway. What concerns me is
> > > exactly how explain this.
> > >
> > > As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue
> > > multiple GetQuote requests, and when you receive the interrupt, you check which
> > > buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote
> > > ready. However I am not 100% sure whether above will always work. Interrupt
> > > can get lost when there are multiple Quotes ready in multiple buffer in very
> > > short time period, etc? Perhaps Isaku can provide more input here.
> >
> > Either supported or not, it should be mentioned in the GHCI spec. Currently,
> > there are no details related to it. If it is supported, the specification
> > should include the protocol to use.
> >
> > I will check with Isaku about it.
>
> The spec says that TD can call multiple GetQuote requests in parallel.
>
> TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's
> implementation specific that how many concurrent requests are allowed. The TD
> should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple
> requests simultaneously
>
> As Kai said, there is no requirement for multiple GetQuote in parallel, it's
> okay to support only single request at the same time.
>
> While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA. The
> attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before
> sending next request.
Sorry I missed this in the spec. Then as I mentioned above, TD should check
which buffer has GET_QUOTE_IN_FLIGHT bit cleared to determine which GetQuote
request is done? I guess this is the only way.
Anyway, supporting single request only is fine to me. Just needs some
explanation in comments or commit message.
--
Thanks,
-Kai
On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
> If we want to support multiple GetQuote requests in parallel, then we
> need some way to uniquely identify the GetQuote requests. So that when
> we get completion notification, we can understand which request is
> completed. This part is not mentioned/discussed in ABI spec. So we want
> to serialize the requests for now.
>
Yes it's unfortunate that this part (whether concurrent GetQuote requests are
supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am
fine with only supporting GetQuote requests one by one. AFAICT there's no
request to support concurrent GetQuote requests anyway. What concerns me is
exactly how explain this.
As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue
multiple GetQuote requests, and when you receive the interrupt, you check which
buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote
ready. However I am not 100% sure whether above will always work. Interrupt
can get lost when there are multiple Quotes ready in multiple buffer in very
short time period, etc? Perhaps Isaku can provide more input here.
Anyway, how about explaining in this way:
"The GHCI spec doesn't clearly say whether TDX can support or how to support
multiple GetQuote requests in parallel. Attestation request is not supposed to
be frequent and should not be in performance critical path. Only support
GetQuote requests in serialized way for now."
--
Thanks,
-Kai
On 4/20/22 5:11 PM, Kai Huang wrote:
> On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
>> If we want to support multiple GetQuote requests in parallel, then we
>> need some way to uniquely identify the GetQuote requests. So that when
>> we get completion notification, we can understand which request is
>> completed. This part is not mentioned/discussed in ABI spec. So we want
>> to serialize the requests for now.
>>
>
> Yes it's unfortunate that this part (whether concurrent GetQuote requests are
> supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am
> fine with only supporting GetQuote requests one by one. AFAICT there's no
> request to support concurrent GetQuote requests anyway. What concerns me is
> exactly how explain this.
>
> As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue
> multiple GetQuote requests, and when you receive the interrupt, you check which
> buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote
> ready. However I am not 100% sure whether above will always work. Interrupt
> can get lost when there are multiple Quotes ready in multiple buffer in very
> short time period, etc? Perhaps Isaku can provide more input here.
Either supported or not, it should be mentioned in the GHCI spec.
Currently, there are no details related to it. If it is supported, the
specification should include the protocol to use.
I will check with Isaku about it.
>
> Anyway, how about explaining in this way:
>
> "The GHCI spec doesn't clearly say whether TDX can support or how to support
> multiple GetQuote requests in parallel. Attestation request is not supposed to
> be frequent and should not be in performance critical path. Only support
> GetQuote requests in serialized way for now."
It seems good enough. I will use it.
>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 4/19/22 6:20 PM, Isaku Yamahata wrote:
> If timeout occurs, the state of adev->tdquote_buf is unknown. It's not safe
> to continue to using adev->tdquote_buf. VMM would continue to processing
> getquote request with this buffer. What if TDX_CMD_GEN_QUOTE is issued again,
> and tdquote_buf is re-used?
This part is not clearly discussed in the specification. May be spec
should define some reasonable timeout and teardown details.
Regarding not using this buffer again, what happens if we de-allocate
it on timeout and the host still updates it?
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Wed, Apr 20, 2022 at 03:09:44PM -0700, Sathyanarayanan Kuppuswamy wrote:
> I agree with dropping the new CONFIG option. But regarding driver
> location, which is preferred (platform/x86/intel/tdx or arch/x86/coco/tdx/)?
Well, since this is not really a driver but the attestation part of TDX,
then arch/x86/coco/tdx/ sounds like the place.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> TDX guest supports encrypted disk as root or secondary drives.
> Decryption keys required to access such drives are usually maintained
> by 3rd party key servers. Attestation is required by 3rd party key
> servers to get the key for an encrypted disk volume, or possibly other
> encrypted services. Attestation is used to prove to the key server that
> the TD guest is running in a valid TD and the kernel and virtual BIOS
> and other environment are secure.
>
> During the boot process various components before the kernel accumulate
> hashes in the TDX module, which can then combined into a report. This
> would typically include a hash of the bios, bios configuration, boot
> loader, command line, kernel, initrd. After checking the hashes the
> key server will securely release the keys.
>
> The actual details of the attestation protocol depend on the particular
> key server configuration, but some parts are common and need to
> communicate with the TDX module.
As we discussed "key provisioning from key server to support disk decryption" is
only one use case of attestation, so I don't think you need 3 paragraphs to talk
about details of this use case here. The attestation flow is documented in GHCI
so it's clear. The attestation flow (and what this patch does) does not have
any direct relation to the "disk decryption" details above. I think you can
discard above entirely or using one or two simple sentences to explain.
Also, as you agreed you will remove the 8K shared memory assumption:
https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/
and if you agree with my approach (again, I recommend) to split the driver to
two parts (reorganize your patches essentially):
https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/#m9e3c5115df0be0b53d41f987e1eda1715255d1d8
I'll review again once you finish updating the driver.
Btw some minor comments below.
[...]
> --- /dev/null
> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
Perhaps attest.c is enough, no matter where the file will reside.
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel_tdx_attest.c - TDX guest attestation interface driver.
> + *
> + * Implements user interface to trigger attestation process and
> + * read the TD Quote result.
> + *
> + * Copyright (C) 2021-2022 Intel Corporation
For upstream I guess just need 2022.
[...]
> +struct attest_dev {
> + /* Mutex to serialize attestation requests */
> + struct mutex lock;
I think we need a comment to explain why the driver doesn't support multiple
GetQuote requests in parallel. In fact the updated GHCI spec doesn't explicitly
say GetQuote cannot be done in parallel. It has a new "GET_QUOTE_IN_FLIGHT"
flag introduced, which can be used to determine which Quote is finished I think.
I am fine with only supporting GetQuote in serialized way, but perhaps we need
to call out the reason somewhere.
[...]
> +
> + /* Allocate DMA buffer to get TDQUOTE data from the VMM */
> + adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE,
> + &adev->handle,
> + GFP_KERNEL | __GFP_ZERO);
> + if (!adev->tdquote_buf) {
> + ret = -ENOMEM;
> + goto failed;
> + }
The buffer needs to be shared. Guest must have called MapGPA to convert the
buffer to shared. Is this guaranteed by calling dma_alloc_coherent(), since
seems I didn't see any MapGPA in the driver? Anyway this deserves a comment I
think.
--
Thanks,
-Kai
On Wed, Apr 20, 2022 at 07:42:06PM -0700,
Sathyanarayanan Kuppuswamy <[email protected]> wrote:
>
>
> On 4/20/22 5:11 PM, Kai Huang wrote:
> > On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > If we want to support multiple GetQuote requests in parallel, then we
> > > need some way to uniquely identify the GetQuote requests. So that when
> > > we get completion notification, we can understand which request is
> > > completed. This part is not mentioned/discussed in ABI spec. So we want
> > > to serialize the requests for now.
> > >
> >
> > Yes it's unfortunate that this part (whether concurrent GetQuote requests are
> > supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am
> > fine with only supporting GetQuote requests one by one. AFAICT there's no
> > request to support concurrent GetQuote requests anyway. What concerns me is
> > exactly how explain this.
> >
> > As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue
> > multiple GetQuote requests, and when you receive the interrupt, you check which
> > buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote
> > ready. However I am not 100% sure whether above will always work. Interrupt
> > can get lost when there are multiple Quotes ready in multiple buffer in very
> > short time period, etc? Perhaps Isaku can provide more input here.
>
> Either supported or not, it should be mentioned in the GHCI spec. Currently,
> there are no details related to it. If it is supported, the specification
> should include the protocol to use.
>
> I will check with Isaku about it.
The spec says that TD can call multiple GetQuote requests in parallel.
TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's
implementation specific that how many concurrent requests are allowed. The TD
should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple
requests simultaneously
As Kai said, there is no requirement for multiple GetQuote in parallel, it's
okay to support only single request at the same time.
While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA. The
attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before
sending next request.
--
Isaku Yamahata <[email protected]>
On Tue, 2022-04-19 at 15:49 -0700, Dave Hansen wrote:
> On 4/19/22 15:21, Kai Huang wrote:
> > On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote:
> > > On 4/19/22 00:47, Kai Huang wrote:
> > > > > From security's perspective, attestation is an essential part of TDX. That
> > > > being said, w/o attestation support in TD guest, I guess nobody will seriously
> > > > use TD guest.
> > > Are you saying you can't think of a single threat model where there's a
> > > benefit to running a TDX guest without attestation? Will TDX only be
> > > used in environments where secrets are provisioned to guests on the
> > > basis of attestation?
> > >
> > I don't think anyone should provision secret to a TD before it get attested that
> > it is a genuine TD that he/she expected. If someone does that, he/she takes the
> > risk of losing the secret. Of course if someone just want to try a TD then w/o
> > attestation is totally fine.
>
> Yeah, but you said:
>
> w/o attestation support in TD guest, I guess nobody will
> seriously use TD guest.
>
> I'm trying to get to the bottom of that. That's a much more broad
> statement than something about when it's safe to deploy secrets.
>
> There are lots of secrets deployed in (serious) VMs today. There are
> lots of secrets deployed in (serious) SEV VMs that don't have
> attestation. Yet, the world somehow hasn't come crashing down.
>
> I think it's crazy to say that nobody will deploy secrets to TDX VMs
> without attestation. I think it's a step father into crazy land to say
> that no one will "seriously" use TDX guests without attestation.
>
> Let's be honest about this and not live in some fantasy world, please.
OK agree. No argument about this.
--
Thanks,
-Kai
On 4/19/22 07:19, Sathyanarayanan Kuppuswamy wrote:
> On 4/19/22 7:13 AM, Dave Hansen wrote:
>>> >From this perspective, I am not sure what's the value of having a
>>> dedicated
>>> INTEL_TDX_ATTESTATION Kconfig. The attestation support code should
>>> be turned on
>>> unconditionally when CONFIG_INTEL_TDX_GUEST is on. The code can also
>>> be just
>>> under arch/x86/coco/tdx/ I guess?
>> How much code are we talking about? What's the difference in the size
>> of the binaries with this compiled in?
>
> Current driver size is ~300 lines. It adds ~500 bytes to the kernel
> binary if it is built-in.
That doesn't sound like good use of a Kconfig option to me. Just
explain in the cover letter:
Any distribution enabling TDX is also expected to need
attestation. The compiled size is quite small (500 bytes).
On Thu, Apr 21, 2022 at 07:53:39AM -0700,
Sathyanarayanan Kuppuswamy <[email protected]> wrote:
> On 4/20/22 11:57 PM, Isaku Yamahata wrote:
> > On Wed, Apr 20, 2022 at 07:42:06PM -0700,
> > Sathyanarayanan Kuppuswamy <[email protected]> wrote:
> >
> > TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's
> > implementation specific that how many concurrent requests are allowed. The TD
> > should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple
> > requests simultaneously
>
> Do you know why we should handle VMCALL_RETRY case? IIUC, as per
> above spec, if each request we send uses different GPA buffer, then we
> should not even worry about checking for IN_FLIGHT status. right?
Not correct. User space VMM, e.g. qemu, may return RETRY error for various
reasons even if different GPAs are used or even if only single request is issued
at the same time. Other user space VMM (there are severals alternatives to qemu)
would support TDX in future. They would choose different way to implement.
Attestation driver should check IN_FLIGHT always before processing shared GPA.
Interrupt notifies only that it needs attention from attestation driver. It
doesn't guarantee that IN_FLIGHT is cleared. Interrupt indicates only that the
state may be changed. It may not be changed. VMM inject the iterrupt
spuriously.
--
Isaku Yamahata <[email protected]>
On 4/21/22 2:10 AM, Borislav Petkov wrote:
> Well, since this is not really a driver but the attestation part of TDX,
> then arch/x86/coco/tdx/ sounds like the place.
Ok. I will move it to arch/x86/coco/tdx/attest.c
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 4/20/22 11:57 PM, Isaku Yamahata wrote:
> On Wed, Apr 20, 2022 at 07:42:06PM -0700,
> Sathyanarayanan Kuppuswamy <[email protected]> wrote:
>
>>
>>
>> On 4/20/22 5:11 PM, Kai Huang wrote:
>>> On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote:
>>>> If we want to support multiple GetQuote requests in parallel, then we
>>>> need some way to uniquely identify the GetQuote requests. So that when
>>>> we get completion notification, we can understand which request is
>>>> completed. This part is not mentioned/discussed in ABI spec. So we want
>>>> to serialize the requests for now.
>>>>
>>>
>>> Yes it's unfortunate that this part (whether concurrent GetQuote requests are
>>> supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am
>>> fine with only supporting GetQuote requests one by one. AFAICT there's no
>>> request to support concurrent GetQuote requests anyway. What concerns me is
>>> exactly how explain this.
>>>
>>> As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue
>>> multiple GetQuote requests, and when you receive the interrupt, you check which
>>> buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote
>>> ready. However I am not 100% sure whether above will always work. Interrupt
>>> can get lost when there are multiple Quotes ready in multiple buffer in very
>>> short time period, etc? Perhaps Isaku can provide more input here.
>>
>> Either supported or not, it should be mentioned in the GHCI spec. Currently,
>> there are no details related to it. If it is supported, the specification
>> should include the protocol to use.
>>
>> I will check with Isaku about it.
>
> The spec says that TD can call multiple GetQuote requests in parallel.
Sorry, I missed the above content. Thanks for pointing out.
>
> TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's
> implementation specific that how many concurrent requests are allowed. The TD
> should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple
> requests simultaneously
Do you know why we should handle VMCALL_RETRY case? IIUC, as per
above spec, if each request we send uses different GPA buffer, then we
should not even worry about checking for IN_FLIGHT status. right?
>
> As Kai said, there is no requirement for multiple GetQuote in parallel, it's
> okay to support only single request at the same time.
For now I will leave it as single request at a time.
>
> While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA. The
> attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before
> sending next request.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 4/21/22 12:04 AM, Isaku Yamahata wrote:
> On Tue, Apr 19, 2022 at 06:26:43PM -0700,
> Sathyanarayanan Kuppuswamy <[email protected]> wrote:
>
>> On 4/19/22 6:20 PM, Isaku Yamahata wrote:
>>> If timeout occurs, the state of adev->tdquote_buf is unknown. It's not safe
>>> to continue to using adev->tdquote_buf. VMM would continue to processing
>>> getquote request with this buffer. What if TDX_CMD_GEN_QUOTE is issued again,
>>> and tdquote_buf is re-used?
>>
>> This part is not clearly discussed in the specification. May be spec
>> should define some reasonable timeout and teardown details.
>>
>> Regarding not using this buffer again, what happens if we de-allocate
>> it on timeout and the host still updates it?
>
> Until GET_QUOTE_IN_FLIGHT is cleared, the shared page is owned by VMM, TD
> attestation driver shouldn't reuse/free the pages.
>
> In the case of this driver, I think of two options
> - don't timeout. wait for interrupt to arrive and check the shared GPA state.
> - allow timeout. When the next request comes, check the shared GPA state.
> If it's still GET_QUOTE_IN_FLIGHT, return EBUSY.
Out of the above two options, I think option 1 is better. It is simpler
to serialize them using mutex compared to checking the shared buffer of
previous request.
>
> It's possible for VMM to keep the shared GPA forever maliciously(DoS) or
> unintentionally due to bug. TD can't do much about it.
I will add a note about it in commit log.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Tue, Apr 19, 2022 at 06:26:43PM -0700,
Sathyanarayanan Kuppuswamy <[email protected]> wrote:
> On 4/19/22 6:20 PM, Isaku Yamahata wrote:
> > If timeout occurs, the state of adev->tdquote_buf is unknown. It's not safe
> > to continue to using adev->tdquote_buf. VMM would continue to processing
> > getquote request with this buffer. What if TDX_CMD_GEN_QUOTE is issued again,
> > and tdquote_buf is re-used?
>
> This part is not clearly discussed in the specification. May be spec
> should define some reasonable timeout and teardown details.
>
> Regarding not using this buffer again, what happens if we de-allocate
> it on timeout and the host still updates it?
Until GET_QUOTE_IN_FLIGHT is cleared, the shared page is owned by VMM, TD
attestation driver shouldn't reuse/free the pages.
In the case of this driver, I think of two options
- don't timeout. wait for interrupt to arrive and check the shared GPA state.
- allow timeout. When the next request comes, check the shared GPA state.
If it's still GET_QUOTE_IN_FLIGHT, return EBUSY.
It's possible for VMM to keep the shared GPA forever maliciously(DoS) or
unintentionally due to bug. TD can't do much about it.
--
Isaku Yamahata <[email protected]>