linux-kernel - Re: [PATCH v7] x86/tdx: Dump TDX Version During TD Bootup

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 27 Oct 2023 08:56:54 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "Sun, Yi" <yi.sun@...el.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "x86@...nel.org" <x86@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
CC:     "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Yan, Dongcheng" <dongcheng.yan@...el.com>,
        "nik.borisov@...e.com" <nik.borisov@...e.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "yi.sun@...ux.intel.com" <yi.sun@...ux.intel.com>,
        "Su, Heng" <heng.su@...el.com>
Subject: Re: [PATCH v7] x86/tdx: Dump TDX Version During TD Bootup


> +/*
> + * Detect TDX Module version info from TDG.SYS.RD TDCALL
> + */
> +static void detect_tdx_version(void)
> +{
> +	struct tdx_module_args args = {};
> +	u16 major_version, minor_version;
> +	u32 vendor_id;
> +	u64 ret;
> +
> +	args.rdx = TDX_SYS_VENDOR_ID_FID;
> +	ret = __tdcall_ret(TDG_SYS_RD, &args);
> +	if (ret)
> +		goto err_out;

I am not sure we need to detect vendor ID?  I believe TDX must be from Intel?

Also, I think either "err" or "out" is sufficient?

> +
> +	vendor_id = args.r8;
> +
> +	memset(&args, 0, sizeof(args));
> +	args.rdx = TDX_SYS_MAJOR_FID;
> +	ret = __tdcall_ret(TDG_SYS_RD, &args);
> +	if (ret)
> +		goto err_out;
> +
> +	major_version = args.r8;
> +
> +	memset(&args, 0, sizeof(args));
> +	args.rdx = TDX_SYS_MINOR_FID;
> +	ret = __tdcall_ret(TDG_SYS_RD, &args);
> +	if (ret)
> +		goto err_out;
> +
> +	minor_version = args.r8;
> +
> +	pr_info("Guest detected. version:%u.%u VendorID:%x\n",
> +		major_version, minor_version, vendor_id);
> +
> +	return;
> +
> +err_out:
> +	if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> +		pr_info("TDG.SYS.RD not available\n");
> +	else
> +		pr_info("TDG.SYS.RD unknown error (%llu), reading field %llu\n",
> +			ret, args.rdx);
> +
> +	pr_info("Assuming TDX version:1.x (x<5) VendorID:%x\n",
> +		TDX_VENDOR_INTEL);

You removed "Guest detected" in tdx_early_init(), but didn't add it here, so
unfortunately the "Guest detected" will get lost here.

Also, I suppose for the case TDG.SYS.RD isn't supported there's still some value
to print "Assume module version: 1.x (x<5)", but when TDG.SYS.RD failed
unexpectedly I don't think we can make that assumption, and printing "assuming
..." could give user misinformation which is worse than not printing IMHO.

So, I think you can keep the printing of "Guest detected" in tdx_early_init(),
and somehow refine the code to print something like below:

For module that does TDG.SYS.RD successfully:

  tdx: Guest detected
  tdx: Module version: %u.%u

For module that doesn't support TDG.SYS.RD:

  tdx: Guest detected
  tdx: Failed to get module version: TDG.SYS.RD unsupported. Assume module
version: 1.x (x < 5).

For module that TDG.SYS.RD failed unexpectedly:

  tdx: Guest detected
  tdx: Failed to get module version: TDG.SYS.RD failed to read 0x%llu: 0x%llu