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

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47f2547406893baaaca7de5cd84955424940b32b.camel@intel.com>
Date: Fri, 6 Dec 2024 16:13:48 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Li, Xiaoyao" <xiaoyao.li@...el.com>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "seanjc@...gle.com" <seanjc@...gle.com>, "Edgecombe,
 Rick P" <rick.p.edgecombe@...el.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "Chatre, Reinette"
	<reinette.chatre@...el.com>, "Hansen, Dave" <dave.hansen@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Zhao, Yan Y"
	<yan.y.zhao@...el.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>, "tony.lindgren@...ux.intel.com"
	<tony.lindgren@...ux.intel.com>
Subject: Re: [PATCH v2 03/25] x86/virt/tdx: Read essential global metadata for
 KVM

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?