linux-kernel - Re: [PATCH v5] x86/tdx: Dump TDX version During the 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]

Message-ID: <3787eac9-cf18-4210-b11b-a34068887bb3@intel.com>
Date:   Thu, 12 Oct 2023 08:44:10 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Yi Sun <yi.sun@...el.com>, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, dave.hansen@...ux.intel.com, peterz@...radead.org,
        x86@...nel.org
Cc:     kirill.shutemov@...ux.intel.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com, kai.huang@...el.com,
        nik.borisov@...e.com, linux-kernel@...r.kernel.org,
        heng.su@...el.com, yi.sun@...ux.intel.com,
        Dongcheng Yan <dongcheng.yan@...el.com>
Subject: Re: [PATCH v5] x86/tdx: Dump TDX version During the TD Bootup

On 10/12/23 06:41, Yi Sun wrote:
> It is essential for TD users to be aware of the vendor and version of
> the current TDX. Additionally, they can reference the TDX version when
> reporting bugs or issues.

... and they will all have to pull this "essential" information out of
dmesg?

If this is essential, why stick it in dmesg where it can be overwritten?

> Furthermore, the applications or device drivers running in TD can achieve
> enhanced reliability and flexibility by following the TDX Module ABI
> specification, because there are significant differences between different
> versions of TDX, as mentioned in the "Intel® TDX Module Incompatibilities
> between v1.0 and v1.5" reference.

This is orthogonal to this patch.  No applications or device drivers can
do anything with the result of these version queries.

> Add function detect_tdx_version to fetch and dump the version of the
> TDX, which is called during TD initialization. Obtain the info by calling
> TDG.SYS.RD, including the major and minor version numbers and vendor ID.

You don't need to rewrite the code in text form.

> The TDCALL TDG.SYS.RD originates from TDX version 1.5. If the error
> TDCALL_INVALID_OPERAND occurs, it should be treated as TDX version 1.0.

I don't understand what this is trying to say.

>  #define TDREPORT_SUBTYPE_0	0
>  
> +/*
> + * TDX metadata base field id, used by TDCALL TDG.SYS.RD
> + * See TDX ABI Spec Global Metadata Fields
> + */
> +#define TDX_SYS_VENDOR_ID_FID		0x0800000200000000ULL
> +#define TDX_SYS_MINOR_FID		0x0800000100000003ULL
> +#define TDX_SYS_MAJOR_FID		0x0800000100000004ULL
> +#define TDX_VENDOR_INTEL		0x8086
> +
>  /* Called from __tdx_hypercall() for unrecoverable failure */
>  noinstr void __noreturn __tdx_hypercall_failed(void)
>  {
> @@ -800,6 +809,63 @@ static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
>  	return true;
>  }
>  
> +/*
> + * Detect TDX Module version info from TDG.SYS.RD TDCALL
> + */
> +static void detect_tdx_version(void)
> +{
> +	struct tdx_module_args args = {};
> +	u32 vendor_id = TDX_VENDOR_INTEL;

What's the purpose of this assignment?

> +	u16 major_version = 0;
> +	u16 minor_version = 0;
> +	u64 ret;
> +
> +	/*
> +	 * TDCALL leaf TDG_SYS_RD
> +	 */
> +	args.rdx = TDX_SYS_VENDOR_ID_FID;
> +	ret = __tdcall_ret(TDG_SYS_RD, &args);
> +	/*
> +	 * The TDCALL TDG.SYS.RD originates from TDX version 1.5.
> +	 * Treat TDCALL_INVALID_OPERAND error as TDX version 1.0.
> +	 */
> +	if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> +		goto version_1_0;
> +	if (ret) {
> +		WARN(1, "TDX detection: TDG.SYS.RD(VENDOR_ID) error, return %llu\n",
> +		     ret);
> +		return;
> +	}

We do not need random warnings at every step of the way.  Worst case,
make a version of tdcall that will spew an error in common code.

> +	vendor_id = (u32)args.r8;

What's the purpose of the cast?

> +	args.rdx = TDX_SYS_MAJOR_FID;
> +	ret = __tdcall_ret(TDG_SYS_RD, &args);

Does args need to be re-zeroed between tdcalls?

> +	if (ret) {
> +		WARN(1, "TDX detection: TDG.SYS.RD(MAJOR) error, return %llu\n",
> +		     ret);
> +		return;
> +	}
> +	major_version = (u16)args.r8;
> +
> +	args.rdx = TDX_SYS_MINOR_FID;
> +	ret = __tdcall_ret(TDG_SYS_RD, &args);
> +	if (ret) {
> +		WARN(1, "TDX detection: TDG.SYS.RD(MINOR) error, return %llu\n",
> +		     ret);
> +		return;
> +	}
> +	minor_version = (u16)args.r8;
> +
> +	pr_info("TDX detected. TDX version:%u.%u VendorID:%x\n",
> +		major_version, minor_version, vendor_id);
> +
> +	return;
> +
> +	/* TDX 1.0 does not have the TDCALL TDG.SYS.RD */
> +version_1_0:
> +	pr_info("TDX detected. TDG.SYS.RD not available, assuming TDX version: 1.x (x<5)\n");
> +}
> +
>  void __init tdx_early_init(void)
>  {
>  	struct tdx_module_args args = {
> @@ -867,5 +933,5 @@ void __init tdx_early_init(void)
>  	 */
>  	x86_cpuinit.parallel_bringup = false;
>  
> -	pr_info("Guest detected\n");
> +	detect_tdx_version();
>  }
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index f74695dea217..1a0cacad5a0c 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -17,6 +17,7 @@
>  #define TDG_MR_REPORT			4
>  #define TDG_MEM_PAGE_ACCEPT		6
>  #define TDG_VM_WR			8
> +#define TDG_SYS_RD			11

*This* is the place to document that TDG_SYS_RD is not available on old
TDX modules.