linux-kernel - Re: [PATCH v5 2/8] x86/virt/tdx: Rework TD_SYSINFO

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: <c3b1e743-6d34-49ce-8e60-a41038f27c61@intel.com>
Date: Mon, 14 Oct 2024 08:56:08 -0700
From: Dave Hansen <dave.hansen@...el.com>
To: Kai Huang <kai.huang@...el.com>, kirill.shutemov@...ux.intel.com,
 tglx@...utronix.de, bp@...en8.de, peterz@...radead.org, mingo@...hat.com,
 hpa@...or.com, dan.j.williams@...el.com, seanjc@...gle.com,
 pbonzini@...hat.com
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
 rick.p.edgecombe@...el.com, isaku.yamahata@...el.com,
 adrian.hunter@...el.com, nik.borisov@...e.com
Subject: Re: [PATCH v5 2/8] x86/virt/tdx: Rework TD_SYSINFO_MAP to support
 build-time verification

On 10/14/24 04:31, Kai Huang wrote:
> +#define READ_SYS_INFO(_field_id, _member)				\
> +	ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
> +					&sysinfo_tdmr->_member)
>  
> -	return 0;
> +	READ_SYS_INFO(MAX_TDMRS,	     max_tdmrs);
> +	READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> +	READ_SYS_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> +	READ_SYS_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> +	READ_SYS_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);

I know what Dan asked for here, but I dislike how this ended up.

The existing stuff *has* type safety, despite the void*.  It at least
checks the size, which is the biggest problem.

Also, this isn't really an unrolled loop.  It still effectively has
gotos, just like the for loop did.  It just buries the goto in the "ret
= ret ?: " construct.  It hides the control flow logic.

Logically, this whole function is

	ret = read_something1();
	if (ret)
		goto out;

	ret = read_something2();
	if (ret)
		goto out;

	...

I'd *much* rather have that goto be:

	for () {
		ret = read_something();
		if (ret)
			break; // aka. goto out
	}

Than have something *look* like straight control flow when it isn't.