Re: [PATCH v4 1/3] x86/tdx: Add TDX Guest attestation interface driver

Sathyanarayanan Kuppuswamy
2022-04-27T22:08:51Z

Hi Isaku,

On 4/26/22 10:45 PM, Isaku Yamahata wrote:
> On Fri, Apr 22, 2022 at 04:34:16PM -0700,
> Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>> In TDX guest, attestation is used to verify the trustworthiness of a TD
>> to other entities before making any secure communication.
>>
>> One usage example is, when a TD guest uses encrypted drive and the
>> decryption keys required to access the drive are stored in a secure
>> 3rd party keyserver, TD guest can use the quote generated via the
>> attestation process to prove its trustworthiness with keyserver and
>> get access to the storage keys.
>>
>> General steps involved in attestation process 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.
>>
>> More details on above mentioned steps can be found in TDX Guest-Host
>> Communication Interface (GHCI) for Intel TDX 1.0, section titled
>> "TD attestation".
>>
>> To allow the attestation agent (user application) to implement this
>> feature, add an IOCTL interface to get TDREPORT and TDQUOTE from the
>> user space. Since attestation agent can also use methods like vosck or
>> TCP/IP to get the TDQUOTE, adding an IOCTL interface for it is an
>> optional feature. So to simplify the driver, first add support for
>> TDX_CMD_GET_TDREPORT IOCTL. Support for TDQUOTE IOCTL will be added by
>> follow-on patches.
>>
>> TDREPORT can be generated by sending a TDCALL with leaf ID as 0x04.
>> More details about the TDREPORT TDCALL can be found in Intel Trust
>> Domain Extensions (Intel TDX) Module specification, section titled
>> "TDG.MR.REPORT Leaf". Add a wrapper function (tdx_mcall_tdreport())
>> to get the TDREPORT from the TDX Module. This API will be used by the
>> interface driver to request for TDREPORT.
>>
>> 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.
>>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>> arch/x86/coco/tdx/Makefile | 2 +-
>> arch/x86/coco/tdx/attest.c | 191 ++++++++++++++++++++++++++++++++
>> arch/x86/coco/tdx/tdx.c | 45 ++++++++
>> arch/x86/include/asm/tdx.h | 2 +
>> arch/x86/include/uapi/asm/tdx.h | 23 ++++
>> 5 files changed, 262 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..b776e81f6c20
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * attest.c - TDX guest attestation interface driver.
>> + *
>> + * Implements user interface to trigger attestation process and
>> + * read the TD Quote result.
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + *
>> + */
>> +
>> +#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/asm/tdx.h>
>> +
>> +#define DRIVER_NAME "tdx-attest"
>> +
>> +static struct platform_device *pdev;
>> +static struct miscdevice miscdev;
>> +
>> +static long tdx_get_tdreport(void __user *argp)
>> +{
>> + void *report_buf = NULL, *tdreport_buf = NULL;
>> + long ret = 0, err;
>> +
>> + /* Allocate space for report data */
>> + report_buf = kmalloc(TDX_REPORT_DATA_LEN, GFP_KERNEL);
>> + if (!report_buf)
>> + return -ENOMEM;
>> +
>> + /*
>> + * Allocate space for TDREPORT buffer (1024-byte aligned).
>> + * Full page alignment is more than enough.
>> + */
>> + tdreport_buf = (void *)get_zeroed_page(GFP_KERNEL);
>> + if (!tdreport_buf) {
>> + ret = -ENOMEM;
>> + goto tdreport_failed;
>> + }
>> +
>> + /* Copy report data to kernel buffer */
>> + if (copy_from_user(report_buf, argp, TDX_REPORT_DATA_LEN)) {
>> + ret = -EFAULT;
>> + goto tdreport_failed;
>> + }
>> +
>> + /* Generate TDREPORT using report data in report_buf */
>> + err = tdx_mcall_tdreport(tdreport_buf, report_buf);
>> + if (err) {
>> + /* If failed, pass TDCALL error code back to user */
>> + ret = put_user(err, (long __user *)argp);
>
> ret is ignored. ret should be checked or ignore it explicitly by (void).

Since we are already in error path and we are going to return -EIO, I
did not check for it. I will ignore it explictly in next version.

>
>
>> + ret = -EIO;
>> + goto tdreport_failed;
>> + }
>> +
>> + /* Copy TDREPORT data back to user buffer */
>> + if (copy_to_user(argp, tdreport_buf, TDX_TDREPORT_LEN))
>> + ret = -EFAULT;
>> +
>> +tdreport_failed:
>> + kfree(report_buf);
>> + if (tdreport_buf)
>> + free_pages((unsigned long)tdreport_buf, 0);
>> +
>> + return ret;
>> +}
>> +
>> +static long tdx_attest_ioctl(struct file *file, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + void __user *argp = (void __user *)arg;
>> + long ret = 0;
>> +
>> + switch (cmd) {
>> + case TDX_CMD_GET_TDREPORT:
>> + ret = tdx_get_tdreport(argp);
>> + break;
>> + default:
>> + pr_err("cmd %d not supported\n", cmd);
>
> pr_debug()? At least error should be returned to user space.

Ok. I will change it to pr_debug(). I have already addressed the
issue of not setting error value in default case.

>
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static const struct file_operations tdx_attest_fops = {
>> + .owner = THIS_MODULE,
>> + .unlocked_ioctl = tdx_attest_ioctl,
>> + .llseek = no_llseek,
>> +};
>> +
>> +static int tdx_attest_probe(struct platform_device *attest_pdev)
>> +{
>> + struct device *dev = &attest_pdev->dev;
>> + long ret = 0;
>> +
>> + /* Only single device is allowed */
>> + if (pdev)
>> + return -EBUSY;
>> +
>> + pdev = attest_pdev;
>> +
>> + miscdev.name = DRIVER_NAME;
>> + miscdev.minor = MISC_DYNAMIC_MINOR;
>> + miscdev.fops = &tdx_attest_fops;
>> + miscdev.parent = dev;
>> +
>> + ret = misc_register(&miscdev);
>> + if (ret) {
>> + pr_err("misc device registration failed\n");
>> + goto failed;
>> + }
>> +
>> + pr_debug("module initialization success\n");
>> +
>> + return 0;
>> +
>> +failed:
>> + misc_deregister(&miscdev);
>> +
>> + pr_debug("module initialization failed\n");
>> +
>> + return ret;
>> +}
>> +
>> +static int tdx_attest_remove(struct platform_device *attest_pdev)
>> +{
>> + misc_deregister(&miscdev);
>> + 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 <sathyanarayanan.kuppuswamy@linux.intel.com>");
>> +MODULE_DESCRIPTION("TDX attestation driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 03deb4d6920d..2a79ca92a52d 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -11,10 +11,12 @@
>> #include <asm/insn.h>
>> #include <asm/insn-eval.h>
>> #include <asm/pgtable.h>
>> +#include <asm/io.h>
>>
>> /* TDX module Call Leaf IDs */
>> #define TDX_GET_INFO 1
>> #define TDX_GET_VEINFO 3
>> +#define TDX_GET_REPORT 4
>> #define TDX_ACCEPT_PAGE 6
>>
>> /* TDX hypercall Leaf IDs */
>> @@ -34,6 +36,10 @@
>> #define VE_GET_PORT_NUM(e) ((e) >> 16)
>> #define VE_IS_IO_STRING(e) ((e) & BIT(4))
>>
>> +/* TDX Module call error codes */
>> +#define TDCALL_RETURN_CODE_MASK 0xffffffff00000000
>> +#define TDCALL_RETURN_CODE(a) ((a) & TDCALL_RETURN_CODE_MASK)
>> +
>> /*
>> * Wrapper for standard use of __tdx_hypercall with no output aside from
>> * return code.
>> @@ -98,6 +104,45 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
>> panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
>> }
>>
>> +/*
>> + * tdx_mcall_tdreport() - Generate TDREPORT_STRUCT using TDCALL.
>> + *
>> + * @data : Address of 1024B aligned data to store
>> + * TDREPORT_STRUCT.
>> + * @reportdata : Address of 64B aligned report data
>> + *
>> + * return 0 on success or failure error number.
>
> Is "failure error number" TDX status code? not -Evalue?

It is not -Evalue. I can use the term "TDX error code"

>
>
>> + */
>> +long tdx_mcall_tdreport(void *data, void *reportdata)
>> +{
>> + u64 ret;
>> +
>> + /*
>> + * Check for a valid TDX guest to ensure this API is only
>> + * used by TDX guest platform. Also make sure "data" and
>> + * "reportdata" pointers are valid.
>> + */
>> + if (!data || !reportdata || !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> + return -EINVAL;
>> +
>> + /*
>> + * Pass the physical address of user generated report data
>> + * and the physical address of output buffer to the TDX module
>> + * to generate the TD report. Generated data contains
>> + * measurements/configuration data of the TD guest. More info
>> + * about ABI can be found in TDX 1.0 Module specification, sec
>> + * titled "TDG.MR.REPORT".
>> + */
>> + ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(data),
>> + virt_to_phys(reportdata), 0, 0, NULL);
>> +
>> + if (ret)
>> + return TDCALL_RETURN_CODE(ret);
>
> Why is status code masked?

Only upper 32 bit has error code. Check "Function Completion Status
Codes" in TDX module spec.

>
>
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(tdx_mcall_tdreport);
>
> Can this function be put into attest.c. Instead, wecan export __tdx_module_call.

We don't need to export it anymore. I will remove it in next version.

Regarding moving it to attest.c, currently all TDCALL/TDVMCALL wrappers
are left in one place (in tdx.c). For example check tdx_kvm_hypercall()
implementation in tdx.c. I don't see any specific need to break this
uniformity for our use case. Grouping them together will make
it easier if we are fixing some core issues in core functions like
__tdx_hypercall() in future.

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer