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: <670d6d4cab43d_3ee229434@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 14 Oct 2024 12:13:16 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Dave Hansen <dave.hansen@...el.com>, 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

Dave Hansen wrote:
> 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.

Yeah, the hiding of the control flow was the weakest part of the
suggestion. My main gripe was runtime validation of details that could
be validated at compile time.

There is no real need for control flow at all, i.e. early exit is not
needed as these are not resources that need to be unwound. It simply
needs to count whether all of the reads happened, so something like this
is sufficient:

    success += READ_SYS_INFO(MAX_TDMRS,             max_tdmrs);
    success += READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
    success += READ_SYS_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
    success += READ_SYS_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
    success += READ_SYS_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
    
    if (success != 5)
    	return false;


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ