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

[Huang, Kai]

On Fri, 2024-12-06 at 16:37 +0800, Xiaoyao Li wrote:
>  +static int get_tdx_sys_info_td_conf(struct tdx_sys_info_td_conf *sysinfo_td_conf)
> > +{
> > +	int ret = 0;
> > +	u64 val;
> > +	int i, j;
> > +
> > +	if (!ret && !(ret = read_sys_metadata_field(0x1900000300000000, &val)))
> > +		sysinfo_td_conf->attributes_fixed0 = val;
> > +	if (!ret && !(ret = read_sys_metadata_field(0x1900000300000001, &val)))
> > +		sysinfo_td_conf->attributes_fixed1 = val;
> > +	if (!ret && !(ret = read_sys_metadata_field(0x1900000300000002, &val)))
> > +		sysinfo_td_conf->xfam_fixed0 = val;
> > +	if (!ret && !(ret = read_sys_metadata_field(0x1900000300000003, &val)))
> > +		sysinfo_td_conf->xfam_fixed1 = val;
> > +	if (!ret && !(ret = read_sys_metadata_field(0x9900000100000004, &val)))
> > +		sysinfo_td_conf->num_cpuid_config = val;
> > +	if (!ret && !(ret = read_sys_metadata_field(0x9900000100000008, &val)))
> > +		sysinfo_td_conf->max_vcpus_per_td = val;
> > +	for (i = 0; i < sysinfo_td_conf->num_cpuid_config; i++)
> 
> 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.

+Dave.

I thought 32 (which is also auto-generated from the "Num Fields" in the JSON
file) is architectural, but looking at the TDX 1.5 spec, it seems there's no
place mentioning such.

I think we can add:

	if (sysinfo_td_conf->num_cpuid_config <= 32)
		return -EINVAL;

.. which will make reading global metadata failure, and result in module
initialization failure.  Basically it means if one day some TDX module comes
with >32 entries, some old versions of kernel won't be able to supportit.  But I
think it should be fine.

This also reminds me reading the CMRs is similar, but I don't think we need to
do similar check for reading CMRs because "maximum number of CMRs is 32" is
architectural behaviour as mentioned in the TDX 1.5 base spec:

4.1.3.1 Intel TDX ISA Background: Convertible Memory Ranges (CMRs)

...

* The maximum number of CMRs is implementation specific. It is not explicitly
enumerated; it is deduced from Family/Model/Stepping information provided by
CPUID.
   * The maximum number of CMRs is 32.

Hi Dave,

Do you have any comments?