| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2d5e6b7f-c499-aaa1-a308-cb17b5500c84@linux.intel.com>
Date: Mon, 1 Aug 2022 10:49:43 -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 v9 2/6] selftests: tdx: Test GetReport TDX attestation
feature
On 7/28/22 3:32 AM, Kai Huang wrote:
> On Wed, 2022-07-27 at 20:44 -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, attestation is used to verify the trustworthiness of a
>> TD. During the TD bring-up, Intel TDX module measures and records the
>> initial contents and configuration of TD, and at runtime, TD software
>> uses runtime measurement registers (RMTRs) to measure and record
>> details related to kernel image, command line params, ACPI tables,
>> initrd, etc. At TD runtime, Intel SGX attestation infrastructure is
>> re-used to attest to these measurement data.
>>
>> First step in the TDX attestation process is to get the TDREPORT data.
>> It is fixed size data structure generated by the TDX module which
>> includes the above mentioned measurements data, a MAC to protect the
>> integerity of the TDREPORT, and a 64-Byte of user specified data passed
>> during TDREPORT request which can uniquely identify the TDREPORT.
>>
>> Intel's TDX guest driver exposes TDX_CMD_GET_REPORT IOCTL interface to
>> get the TDREPORT from the user space.
>>
>> Add a kernel selftest module to test this ABI and verify the validity
>> of generated TDREPORT.
>>
>> 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>
>> ---
>> tools/testing/selftests/Makefile | 1 +
>> tools/testing/selftests/tdx/Makefile | 7 +
>> tools/testing/selftests/tdx/tdx_attest_test.c | 160 ++++++++++++++++++
>> 3 files changed, 168 insertions(+)
>> create mode 100644 tools/testing/selftests/tdx/Makefile
>> create mode 100644 tools/testing/selftests/tdx/tdx_attest_test.c
>>
>> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
>> index de11992dc577..807a839d69c4 100644
>> --- a/tools/testing/selftests/Makefile
>> +++ b/tools/testing/selftests/Makefile
>> @@ -69,6 +69,7 @@ TARGETS += sync
>> TARGETS += syscall_user_dispatch
>> TARGETS += sysctl
>> TARGETS += tc-testing
>> +TARGETS += tdx
>> TARGETS += timens
>> ifneq (1, $(quicktest))
>> TARGETS += timers
>> diff --git a/tools/testing/selftests/tdx/Makefile b/tools/testing/selftests/tdx/Makefile
>> new file mode 100644
>> index 000000000000..281db209f9d6
>> --- /dev/null
>> +++ b/tools/testing/selftests/tdx/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +CFLAGS += -O3 -Wl,-no-as-needed -Wall -static
>> +
>> +TEST_GEN_PROGS := tdx_attest_test
>> +
>> +include ../lib.mk
>> diff --git a/tools/testing/selftests/tdx/tdx_attest_test.c b/tools/testing/selftests/tdx/tdx_attest_test.c
>> new file mode 100644
>> index 000000000000..7155cc751eaa
>> --- /dev/null
>> +++ b/tools/testing/selftests/tdx/tdx_attest_test.c
>> @@ -0,0 +1,160 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Test TDX attestation feature
>> + *
>> + * Copyright (C) 2022 Intel Corporation. All rights reserved.
>> + *
>> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>> + */
>> +
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/time.h>
>> +#include <sys/types.h>
>> +#include <time.h>
>> +#include <unistd.h>
>> +
>> +#include "../kselftest_harness.h"
>> +#include "../../../../arch/x86/include/uapi/asm/tdx.h"
>> +
>> +#define devname "/dev/tdx-guest"
>> +#define HEX_DUMP_SIZE 8
>> +
>> +/*
>> + * struct td_info - It contains the measurements and initial configuration of
>> + * the TD that was locked at initialization and a set of measurement
>> + * registers that are run-time extendable. These values are copied from the
>> + * TDCS by the TDG.MR.REPORT function.
>> + */
>> +struct td_info {
>> + /* TD attributes (like debug, spet_disable, etc) */
>> + __u8 attr[8];
>> + __u64 xfam;
>> + /* Measurement registers */
>> + __u64 mrtd[6];
>> + __u64 mrconfigid[6];
>> + __u64 mrowner[6];
>> + __u64 mrownerconfig[6];
>> + /* Runtime measurement registers */
>> + __u64 rtmr[24];
>> + __u64 reserved[14];
>> +};
>> +
>> +/*
>> + * Trusted Execution Environment (TEE) report (TDREPORT_STRUCT) type,
>> + * sub type and version..
>> + */
>> +struct tdreport_type {
>> + /* 0 - SGX, 81 -TDX, rest are reserved */
>> + __u8 type;
>> + /* Default value is 0 */
>> + __u8 sub_type;
>> + /* Default value is 0 */
>> + __u8 version;
>> + __u8 reserved;
>> +};
>> +
>> +/*
>> + * struct reportmac - First field in the TEE report structure
>> + * (TRDREPORT_STRUCT). It is common to Intel’s TEE's e.g., SGX and TDX.
>> + * It is MAC-protected and contains hashes of the remainder of the report
>> + * structure which includes the TEE’s measurements, and where applicable,
>> + * the measurements of additional TCB elements not reflected in CPUSVN –
>> + * e.g., a SEAM’s measurements.
>> + */
>> +struct reportmac {
>> + struct tdreport_type type;
>> + __u8 reserved1[12];
>> + /* CPU security version */
>> + __u8 cpu_svn[16];
>> + /* SHA384 hash of TEE TCB INFO */
>> + __u8 tee_tcb_info_hash[48];
>> + /* SHA384 hash of TDINFO_STRUCT */
>> + __u8 tee_td_info_hash[48];
>> + /* User defined unique data passed in TDG.MR.REPORT request */
>> + __u8 reportdata[64];
>> + __u8 reserved2[32];
>> + __u8 mac[32];
>> +};
>> +
>> +struct tee_tcb_info {
>> + __u8 data[239];
>> +};
>> +
>> +struct tdreport_data {
>> + struct reportmac _reportmac;
>> + struct tee_tcb_info _tcb_info;
>> + __u8 reserved[17];
>> + struct td_info _tdinfo;
>> +};
>
> I think 'struct tdreport' is enough. The _data postfix only causes it to be
> more confusing.
Ok.
>
> Btw, as it appears you only verified reportdata below, is it worth to have all
> those data structures (and they are used by hardware but not __packed)? Perhaps
> a macro to define REPORTDATA offset in TDREPORT is good enough? Or maybe I am
> missing something.
I have added these data structs to make it easier for readers to understand
the contents of the TDREPORT. I thought a simple offset based check would look
like a magic number. If the maintainers are fine with offset based comparison,
I am ok with it.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Powered by blists - more mailing lists