Re: [PATCH v9 2/6] selftests: tdx: Test GetReport TDX attestation feature

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

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