lkml.org 
[lkml]   [2022]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v6 1/5] x86/tdx: Add TDX Guest attestation interface driver
From
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@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 | 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

\
 
 \ /
  Last update: 2022-05-17 16:55    [W:0.139 / U:0.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site