| 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
| ||
|
Message-ID: <9a06e2cf469cbca2777ac2c4ef70579e6bb934d5.camel@intel.com>
Date: Tue, 15 Oct 2024 11:34:41 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Hansen, Dave" <dave.hansen@...el.com>, "seanjc@...gle.com"
<seanjc@...gle.com>, "bp@...en8.de" <bp@...en8.de>, "peterz@...radead.org"
<peterz@...radead.org>, "hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com"
<mingo@...hat.com>, "Williams, Dan J" <dan.j.williams@...el.com>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>, "tglx@...utronix.de"
<tglx@...utronix.de>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "nik.borisov@...e.com"
<nik.borisov@...e.com>, "Hunter, Adrian" <adrian.hunter@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Edgecombe,
Rick P" <rick.p.edgecombe@...el.com>, "x86@...nel.org" <x86@...nel.org>,
"Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH v5 2/8] x86/virt/tdx: Rework TD_SYSINFO_MAP to support
build-time verification
On Mon, 2024-10-14 at 12:13 -0700, Dan Williams wrote:
> 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 understood. Thanks for letting me know.
The 'for() loop' approach would need the original 'struct field_mapping' to hold
the mapping between field ID and the offset/size info, though.
>
> 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.
I am looking into how to do build-time verification while still using the
original 'struct field_mapping' approach. If we can do this, I hope this can
address your concern about doing runtime check instead of build-time?
Adrian provided one suggestion [*] that we can use __builtin_choose_expr() to
achieve this:
"
BUILD_BUG_ON() requires a function, but it is still
be possible to add a build time check in TD_SYSINFO_MAP
e.g.
#define TD_SYSINFO_CHECK_SIZE(_field_id, _size) \
__builtin_choose_expr(MD_FIELD_ELE_SIZE(_field_id) == _size, _size,
(void)0)
#define _TD_SYSINFO_MAP(_field_id, _offset, _size) \
{ .field_id = _field_id, \
.offset = _offset, \
.size = TD_SYSINFO_CHECK_SIZE(_field_id, _size) }
#define TD_SYSINFO_MAP(_field_id, _struct, _member) \
_TD_SYSINFO_MAP(MD_FIELD_ID_##_field_id, \
offsetof(_struct, _member), \
sizeof(typeof(((_struct *)0)->_member)))
"
I tried this, and it worked for most cases where the field ID is a simple
integer constant, but I got build error for the CMRs:
for (u16 i = 0; i < cmr_info->num_cmrs; i++) {
const struct field_mapping fields[] = {
TD_SYSINFO_CMRINFO_MAP(CMR_BASE0 + i, cmr_base[i]),
TD_SYSINFO_CMRINFO_MAP(CMR_SIZE0 + i, cmr_size[i]),
};
...
}
.. where field ID for CMR[i] is calculated by CMR0.
The MD_FIELD_ELE_SIZE(_field_id) works for 'CMR_BASE0 + i' for BUILD_BUG_ON(),
but somehow the compiler fails to determine the 'MD_FIELD_ELE_SIZE(_field_id) ==
_size' as a constant_express and caused build failure. I am still looking into
this.
[*]
https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@intel.com/T/#m379ce041f025dc20e7b58fa6dbdc484c2ce53af4
> 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;
>
If we go with this approach, it seems we can even get rid of the @success.
int ret = 0;
#define READ_SYS_INFO(_field_id, _member) \
read_sys_metadata_field(MD_FIELD_ID_##_field_id, \
&sysinfo_tdmr->_member)
ret |= READ_SYS_INFO(MAX_TDMRS, max_tdmrs);
...
#undef READ_SYS_INFO
return ret;
The tdh_sys_rd() always return -EIO when TDH.SYS.RD fails, so the above either
return 0 when all reads were successful or -EIO when there's any failed.
I can also go with route if Dave is fine?
Powered by blists - more mailing lists