Date:   Tue, 17 May 2022 07:54:42 -0700
From:   Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     Kai Huang <kai.huang@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org
Cc:     "H . Peter Anvin" <hpa@...or.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Wander Lairson Costa <wander@...hat.com>,
        Isaku Yamahata <isaku.yamahata@...il.com>,
        marcelo.cerri@...onical.com, tim.gardner@...onical.com,
        khalid.elmously@...onical.com, philip.cox@...onical.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface
 driver

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 <tony.luck@...el.com>
>> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>> ---
>>   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

Powered by blists - more mailing lists