Re: [PATCH v2 03/25] x86/virt/tdx: Read essential global metadata for KVM

[Edgecombe, Rick P]

On Sat, 2024-12-07 at 00:00 +0000, Huang, Kai wrote:
> > On 12/6/24 08:13, Huang, Kai wrote:
> > > It is not safe. We need to check
> > > 
> > >        sysinfo_td_conf->num_cpuid_config <= 32.
> > > 
> > > If the TDX module version is not matched with the json file that was
> > > used to generate the tdx_global_metadata.h, the num_cpuid_config
> > > reported by the actual TDX module might exceed 32 which causes
> > > out-of-bound array access.
> > 
> > The JSON *IS* the ABI description. It can't change between versions of the
> > TDX module. It can only be extended. The "32" is not in the spec because the
> > spec refers to the JSON!
> 
> Ah, yeah, agreed, the "spec refers to the JSON".  :-)

So we heard back from TDX module folks that they were thinking the 32 could
change to be larger (thanks Kai for checking). We need to continue education
with them around what KVM is depending on as TDX Module ABI. And we should get
something clearer than these JSONs.

But in the meantime, we could tell TDX module team they need an opt-in to change
this field. We could also add an actual check to fail cleanly:

diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
index 44c2b3e079de..744549bdf1dd 100644
--- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
+++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
@@ -97,6 +97,10 @@ static int get_tdx_sys_info_td_conf(struct
tdx_sys_info_td_conf *sysinfo_td_conf
        u64 val;
        int i, j;
 
+       if (sysinfo_td_conf->num_cpuid_config >
+           ARRAY_SIZE(sysinfo_td_conf->cpuid_config_leaves))
+               return 1;
+
        if (!ret && !(ret = read_sys_metadata_field(0x1900000300000000, &val)))
                sysinfo_td_conf->attributes_fixed0 = val;
        if (!ret && !(ret = read_sys_metadata_field(0x1900000300000001, &val)))

Or we could dynamically allocate these arrays based on num_cpuid_config.

I'd lean towards switching to the dynamic allocation, because it will be cleaner
and less churn for this array expanding in the future.