On 4/5/22 21:49, Kai Huang wrote:
> TDX provides increased levels of memory confidentiality and integrity.
> This requires special hardware support for features like memory
> encryption and storage of memory integrity checksums. Not all memory
> satisfies these requirements.
>
> As a result, TDX introduced the concept of a "Convertible Memory Region"
> (CMR). During boot, the firmware builds a list of all of the memory
> ranges which can provide the TDX security guarantees. The list of these
> ranges, along with TDX module information, is available to the kernel by
> querying the TDX module via TDH.SYS.INFO SEAMCALL.
>
> Host kernel can choose whether or not to use all convertible memory
> regions as TDX memory. Before TDX module is ready to create any TD
> guests, all TDX memory regions that host kernel intends to use must be
> configured to the TDX module, using specific data structures defined by
> TDX architecture. Constructing those structures requires information of
> both TDX module and the Convertible Memory Regions. Call TDH.SYS.INFO
> to get this information as preparation to construct those structures.
>
> Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
> ---
> arch/x86/virt/vmx/tdx/tdx.c | 131 ++++++++++++++++++++++++++++++++++++
> arch/x86/virt/vmx/tdx/tdx.h | 61 +++++++++++++++++
> 2 files changed, 192 insertions(+)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index ef2718423f0f..482e6d858181 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -80,6 +80,11 @@ static DEFINE_MUTEX(tdx_module_lock);
>
> static struct p_seamldr_info p_seamldr_info;
>
> +/* Base address of CMR array needs to be 512 bytes aligned. */
> +static struct cmr_info tdx_cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
> +static int tdx_cmr_num;
> +static struct tdsysinfo_struct tdx_sysinfo;
I really dislike mixing hardware and software structures. Please make
it clear which of these are fully software-defined and which are part of
the hardware ABI.
> static bool __seamrr_enabled(void)
> {
> return (seamrr_mask & SEAMRR_ENABLED_BITS) == SEAMRR_ENABLED_BITS;
> @@ -468,6 +473,127 @@ static int tdx_module_init_cpus(void)
> return seamcall_on_each_cpu(&sc);
> }
>
> +static inline bool cmr_valid(struct cmr_info *cmr)
> +{
> + return !!cmr->size;
> +}
> +
> +static void print_cmrs(struct cmr_info *cmr_array, int cmr_num,
> + const char *name)
> +{
> + int i;
> +
> + for (i = 0; i < cmr_num; i++) {
> + struct cmr_info *cmr = &cmr_array[i];
> +
> + pr_info("%s : [0x%llx, 0x%llx)\n", name,
> + cmr->base, cmr->base + cmr->size);
> + }
> +}
> +
> +static int sanitize_cmrs(struct cmr_info *cmr_array, int cmr_num)
> +{
> + int i, j;
> +
> + /*
> + * Intel TDX module spec, 20.7.3 CMR_INFO:
> + *
> + * TDH.SYS.INFO leaf function returns a MAX_CMRS (32) entry
> + * array of CMR_INFO entries. The CMRs are sorted from the
> + * lowest base address to the highest base address, and they
> + * are non-overlapping.
> + *
> + * This implies that BIOS may generate invalid empty entries
> + * if total CMRs are less than 32. Skip them manually.
> + */
> + for (i = 0; i < cmr_num; i++) {
> + struct cmr_info *cmr = &cmr_array[i];
> + struct cmr_info *prev_cmr = NULL;
> +
> + /* Skip further invalid CMRs */
> + if (!cmr_valid(cmr))
> + break;
> +
> + if (i > 0)
> + prev_cmr = &cmr_array[i - 1];
> +
> + /*
> + * It is a TDX firmware bug if CMRs are not
> + * in address ascending order.
> + */
> + if (prev_cmr && ((prev_cmr->base + prev_cmr->size) >
> + cmr->base)) {
> + pr_err("Firmware bug: CMRs not in address ascending order.\n");
> + return -EFAULT;
-EFAULT is a really weird return code to use for this. I'd use -EINVAL.
> + }
> + }
> +
> + /*
> + * Also a sane BIOS should never generate invalid CMR(s) between
> + * two valid CMRs. Sanity check this and simply return error in
> + * this case.
> + *
> + * By reaching here @i is the index of the first invalid CMR (or
> + * cmr_num). Starting with next entry of @i since it has already
> + * been checked.
> + */
> + for (j = i + 1; j < cmr_num; j++)
> + if (cmr_valid(&cmr_array[j])) {
> + pr_err("Firmware bug: invalid CMR(s) among valid CMRs.\n");
> + return -EFAULT;
> + }
Please add brackets for the for().
> + /*
> + * Trim all tail invalid empty CMRs. BIOS should generate at
> + * least one valid CMR, otherwise it's a TDX firmware bug.
> + */
> + tdx_cmr_num = i;
> + if (!tdx_cmr_num) {
> + pr_err("Firmware bug: No valid CMR.\n");
> + return -EFAULT;
> + }
> +
> + /* Print kernel sanitized CMRs */
> + print_cmrs(tdx_cmr_array, tdx_cmr_num, "Kernel-sanitized-CMR");
> +
> + return 0;
> +}
> +
> +static int tdx_get_sysinfo(void)
> +{
> + struct tdx_module_output out;
> + u64 tdsysinfo_sz, cmr_num;
> + int ret;
> +
> + BUILD_BUG_ON(sizeof(struct tdsysinfo_struct) != TDSYSINFO_STRUCT_SIZE);
> +
> + ret = seamcall(TDH_SYS_INFO, __pa(&tdx_sysinfo), TDSYSINFO_STRUCT_SIZE,
> + __pa(tdx_cmr_array), MAX_CMRS, NULL, &out);
> + if (ret)
> + return ret;
> +
> + /*
> + * If TDH.SYS.CONFIG succeeds, RDX contains the actual bytes
> + * written to @tdx_sysinfo and R9 contains the actual entries
> + * written to @tdx_cmr_array. Sanity check them.
> + */
> + tdsysinfo_sz = out.rdx;
> + cmr_num = out.r9;
Please vertically align things like this:
tdsysinfo_sz = out.rdx;
cmr_num = out.r9;
> + if (WARN_ON_ONCE((tdsysinfo_sz > sizeof(tdx_sysinfo)) || !tdsysinfo_sz ||
> + (cmr_num > MAX_CMRS) || !cmr_num))
> + return -EFAULT;
Sanity checking is good, but this makes me wonder how much is too much.
I don't see a lot of code for instance checking if sys_write() writes
more than how much it was supposed to.
Why are these sanity checks necessary here? Is the TDX module expected
to be *THAT* buggy? The thing that's providing, oh, basically all of
the security guarantees of this architecture. It's overflowing the
buffers you hand it?
> + pr_info("TDX module: vendor_id 0x%x, major_version %u, minor_version %u, build_date %u, build_num %u",
> + tdx_sysinfo.vendor_id, tdx_sysinfo.major_version,
> + tdx_sysinfo.minor_version, tdx_sysinfo.build_date,
> + tdx_sysinfo.build_num);
> +
> + /* Print BIOS provided CMRs */
> + print_cmrs(tdx_cmr_array, cmr_num, "BIOS-CMR");
> +
> + return sanitize_cmrs(tdx_cmr_array, cmr_num);
> +}
Does sanitize_cmrs() sanitize anything? It looks to me like it *checks*
the CMRs. But, sanitizing is an active operation that writes to the
data being sanitized. This looks read-only to me. check_cmrs() would
be a better name for a passive check.
> static int init_tdx_module(void)
> {
> int ret;
> @@ -482,6 +608,11 @@ static int init_tdx_module(void)
> if (ret)
> goto out;
>
> + /* Get TDX module information and CMRs */
> + ret = tdx_get_sysinfo();
> + if (ret)
> + goto out;
Couldn't we get rid of that comment if you did something like:
ret = tdx_get_sysinfo(&tdx_cmr_array, &tdx_sysinfo);
and preferably make the variables function-local.
> /*
> * Return -EFAULT until all steps of TDX module
> * initialization are done.
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index b8cfdd6e12f3..2f21c45df6ac 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -29,6 +29,66 @@ struct p_seamldr_info {
> u8 reserved2[88];
> } __packed __aligned(P_SEAMLDR_INFO_ALIGNMENT);
>
> +struct cmr_info {
> + u64 base;
> + u64 size;
> +} __packed;
> +
> +#define MAX_CMRS 32
> +#define CMR_INFO_ARRAY_ALIGNMENT 512
> +
> +struct cpuid_config {
> + u32 leaf;
> + u32 sub_leaf;
> + u32 eax;
> + u32 ebx;
> + u32 ecx;
> + u32 edx;
> +} __packed;
> +
> +#define TDSYSINFO_STRUCT_SIZE 1024
> +#define TDSYSINFO_STRUCT_ALIGNMENT 1024
> +
> +struct tdsysinfo_struct {
> + /* TDX-SEAM Module Info */
> + u32 attributes;
> + u32 vendor_id;
> + u32 build_date;
> + u16 build_num;
> + u16 minor_version;
> + u16 major_version;
> + u8 reserved0[14];
> + /* Memory Info */
> + u16 max_tdmrs;
> + u16 max_reserved_per_tdmr;
> + u16 pamt_entry_size;
> + u8 reserved1[10];
> + /* Control Struct Info */
> + u16 tdcs_base_size;
> + u8 reserved2[2];
> + u16 tdvps_base_size;
> + u8 tdvps_xfam_dependent_size;
> + u8 reserved3[9];
> + /* TD Capabilities */
> + u64 attributes_fixed0;
> + u64 attributes_fixed1;
> + u64 xfam_fixed0;
> + u64 xfam_fixed1;
> + u8 reserved4[32];
> + u32 num_cpuid_config;
> + /*
> + * The actual number of CPUID_CONFIG depends on above
> + * 'num_cpuid_config'. The size of 'struct tdsysinfo_struct'
> + * is 1024B defined by TDX architecture. Use a union with
> + * specific padding to make 'sizeof(struct tdsysinfo_struct)'
> + * equal to 1024.
> + */
> + union {
> + struct cpuid_config cpuid_configs[0];
> + u8 reserved5[892];
> + };
> +} __packed __aligned(TDSYSINFO_STRUCT_ALIGNMENT);
> +
> /*
> * P-SEAMLDR SEAMCALL leaf function
> */
> @@ -38,6 +98,7 @@ struct p_seamldr_info {
> /*
> * TDX module SEAMCALL leaf functions
> */
> +#define TDH_SYS_INFO 32
> #define TDH_SYS_INIT 33
> #define TDH_SYS_LP_INIT 35
> #define TDH_SYS_LP_SHUTDOWN 44
