Message-ID: <e5aed619-20ce-7eb3-22a3-64b51de9cce3@linux.intel.com>
Date:   Mon, 2 May 2022 08:52:12 -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 v5 1/3] x86/tdx: Add TDX Guest attestation interface
 driver

Hi Kai,

On 5/1/22 7:31 PM, Kai Huang wrote:
> On Sun, 2022-05-01 at 11:34 -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".
>>
>> After getting the TDREPORT, the second step of the attestation process
>> is to send the TDREPORT to Quoting Enclave (QE) or Quote Generation
>> Service (QGS) to generate the Quote. However the method of sending the
>> TDREPORT to QE/QGS, communication channel used and data format used is
>> specific to the implementation of QE/QGS.
>>
>> A typical implementation is, TD userspace attestation software gets the
>> TDREPORT from TD kernel, sends it to QE/QGS, and QE/QGS returns the
>> Quote. TD attestation software can use any available communication
>> channel to talk to QE/QGS, such as using vsock and tcp/ip.
>>
>> To support the case that those communication channels are not directly
>> available to the TD, TDX also defines TDVMCALLs to allow TD to ask VMM
>> to help with sending the TDREPORT and receiving the Quote. This support
>> is documented in the GHCI spec section titled "5.4 TD attestation".
> 
> Sorry I didn't think thoroughly in the reply to v4.  I think it's still
> necessary to mention TDREPORT can only be verified locally, because otherwise
> Quote isn't conceptually needed.  And above 3 paragraphs are too verbose I
> guess.  How about below:
> 
> "
> TDREPORT can only be verified locally as the MAC key is bound to the platform.
> TDX attestation leverages Intel SGX Quote Enclave (QE) to verify the TDREPORT
> locally and convert it to a remote verifiable Quote to support remote
> attestation of the TDREPORT.
> 
> TD userspace attestation software firstly gets the TDREPORT from the TD kernel,
> and then sends it to the QE 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 ...

Final version looks like below:

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)
can be 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 here.

> "
> 
>>
>> 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.
> 
> The IOCTL vs /sysfs isn't discussed.

My previous versions had it. But I removed it in later revisions
because I thought the commit log is alredy long and IOCTL interface
justification is less important compared to other details.

> 
> For instance, after rough thinking, why is the IOCTL better than below approach
> using /sysfs?
> 
> echo <REPORTDATA> > /sys/kernel/coco/tdx/attest/reportdata
> cat /sys/kernel/coco/tdx/attest/tdreport
> 
> Each "echo <REPORTDATA>" to '/sys/.../reportdata' triggers the driver to call
> TDCALL to get the TDREPORT, which is available at '/sys/.../tdreport'.
> 
> The benefit of using IOCTL I can think of now is it is perhaps more secure, as
> with IOCTL the REPORTDATA and the TDREPORT is visible to the process which calls
> the IOCTL, while using the /sysfs they are potentially visible to any process.
> Especially the REPORTDATA, i.e. it can come from attestation service after the
> TD attestation agent sets up a secure connection with it.
> 
> Anyway, my 2cents above.

IMO, since TDREPORT is not a secret we don't have to hightlight security
concern here. How about following?

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 here.



> 
>>
>> 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      | 138 ++++++++++++++++++++++++++++++++
>>   arch/x86/include/uapi/asm/tdx.h |  36 +++++++++
>>   3 files changed, 175 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..4543a0264ce7
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,138 @@
>> +// 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/module.h>
> 
> Still need this?
> 
>> +#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>
> 
> Still need above two?
> 
>> +#include <linux/jiffies.h>
> 
> Don't need this either.
> 
>> +#include <linux/io.h>
>> +#include <asm/apic.h>
>> +#include <asm/tdx.h>
>> +#include <asm/irq_vectors.h>
>> +#include <uapi/asm/tdx.h>
> 
> Please get rid of unnecessary headers.

Ok. I will remove unused headers.

> 
>> +
>> +#define DRIVER_NAME "tdx-attest"
>> +
>> +/* TDREPORT module call leaf ID */
>> +#define TDX_GET_REPORT			4
>> +
>> +/* Upper 32 bits has the status code, so mask it */
>> +#define TDCALL_STATUS_CODE_MASK		0xffffffff00000000
>> +#define TDCALL_STATUS_CODE(a)		((a) & TDCALL_STATUS_CODE_MASK)
>> +
>> +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 failed;
> 
> Perhaps 'failed' -> 'out'.  That code is for both error and non-error case.

Ok.

> 
>> +	}
>> +
>> +	/* Copy REPORTDATA from the user buffer */
>> +	if (copy_from_user(reportdata, argp, TDX_REPORTDATA_LEN)) {
>> +		ret = -EFAULT;
>> +		goto failed;
>> +	}
>> +
>> +	/*
>> +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
>> +	 *
>> +	 * Pass the physical address of user generated REPORTDATA
>> +	 * and the physical address of the output buffer to the TDX
>> +	 * module to generate the TDREPORT. 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".
> 
> I guess you can get rid of the entire second paragraph.  If the reference to the
> spec is useful, then keep it but other sentences are not quite useful.  Perhaps:
> 
> 	Get the TDREPORT using REPORTDATA as input.  Refer to 22.3.3
> 	TDG.MR.REPORT leaf in the TDX Module 1.0 Specification for detail
> 	information.

How about following?

Pass REPORTDATA as input and generate TDREPORT using "TDG.MR.REPORT"
TDCALL. Refer to 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",
>> +				TDCALL_STATUS_CODE(ret));
> 
> You can just print out the exact error code.  It's more informative and can help
> to debug.

As per spec, only upper 32 bits has status code. 0:32 does not have any
useful info.

> 
>> +		ret = -EIO;
>> +		goto failed;
>> +	}
>> +
>> +	/* Copy TDREPORT back to the user buffer */
>> +	if (copy_to_user(argp, tdreport, TDX_REPORT_LEN))
>> +		ret = -EFAULT;
>> +
>> +failed:
>> +	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 = 0;
> 
> If you initialize ret to -EINVAL here, then ...

Ok. I will initialize it to -EINVAL.

>> +
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_REPORT:
>> +		ret = tdx_get_report(argp);
>> +		break;
>> +	default:
>> +		pr_debug("cmd %d not supported\n", cmd);
>> +		ret = -EINVAL;
> 
> You don't have to set it here.

Ok.

> 
>> +		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");
> 
> pr_debug() is used when __tdx_module_call() fails, and in the default case in
> tdx_attest_ioctl() too.
> 
> Shouldn't those error msg be printed using the same way?

For IOCTL case, I expect userspace to print the error. But for init
code error, it needs to be handled by kernel. So I have used pr_err
here.

> 
>> +		return ret;
>> +	}
>> +
>> +	pr_debug("module initialization success\n");
> 
> I don't think it's a module anymore?

Agree. I will remove the keyword module here.

> 
> Also perhaps just pr_info()?

Misc device creation itself is sufficient proof of successful
loading. So I will remove this debug message completely.

> 
>> +
>> +	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..9a7377723667
>> --- /dev/null
>> +++ b/arch/x86/include/uapi/asm/tdx.h
>> @@ -0,0 +1,36 @@
>> +/* 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
> 
> I prefer TDX_TDREPORT_LEN.
> 
>> +
>> +/**
>> + * struct tdx_report_req: Get TDREPORT from the TDX module.
> 
> Just get the TDREPORT is enough I guess.

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];
>> +	};
>> +};
> 
> I am not sure overriding the input is a good idea, but will leave to others.

TDCALL uses it that way. So I have followed the same model.

> 
>> +
>> +/* Get TDREPORT from the TDX module using TDCALL[TDG.MR.REPORT) */
> 
> Just get the TDREPORT is enough I guess.

May be following?

Get TDREPORT using TDCALL[TDG.MR.REPORT)

> 
>> +#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