Message-ID: <5473f606bd8e60dd7b8d58a540285d126a1361bd.camel@intel.com>
Date:   Mon, 02 May 2022 14:31:03 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.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

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

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

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.

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

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

> +	}
> +
> +	/* 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.

> +	 */
> +	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.

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

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

> +		return ret;
> +	}
> +
> +	pr_debug("module initialization success\n");

I don't think it's a module anymore?

Also perhaps just pr_info()?

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

> + *
> + * @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.

> +
> +/* Get TDREPORT from the TDX module using TDCALL[TDG.MR.REPORT) */

Just get the TDREPORT is enough I guess.

> +#define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, struct tdx_report_req)
> +
> +#endif /* _UAPI_ASM_X86_TDX_H */

Powered by blists - more mailing lists