On 29.11.2022 21:56, Andrew Cooper wrote: > On 29/11/2022 14:55, Jan Beulich wrote: >> By defining the constant to zero when !SHADOW_PAGING we give compilers >> the chance to eliminate a little more dead code elsewhere in the tree. >> Plus, as a minor benefit, the general reference count can be one bit >> wider. (To simplify things, have PGC_page_table change places with >> PGC_extra.) >> >> Signed-off-by: Jan Beulich <jbeul...@suse.com> > > Ahead of making this change, can we please rename it to something less > confusing, and fix it's comment which is wrong. > > PGC_shadowed_pt is the best I can think of.
Can do, sure. >> --- >> tboot.c's update_pagetable_mac() is suspicious: It effectively is a >> no-op even prior to this change when !SHADOW_PAGING, which can't be >> quite right. If (guest) page tables are relevant to include in the >> verification, shouldn't this look for PGT_l<N>_page_table as well? How >> to deal with HAP guests there is entirely unclear. > > Considering the caller, it MACs every domheap page for domains with > CDF_s3_integrity. > > The tboot logical also blindly assumes that any non-idle domain has an > Intel IOMMU context with it. This only doesn't (trivially) expose > because struct domain_iommu is embedded in struct domain (rather than > allocated separately), and reaching into the wrong part of the arch > union is only mitigated by the tboot logic not being invoked on > non-intel systems. (Also the idle domain check is useless, given that > it's in a for_each_domain() loop). > > It does look a little like the caller is wanting to MAC all Xen data > that describes the guest, but doing this unilaterally for all shadowed > guests seems wrong beside the per-domain s3_integrity setting. Question is - do we care about addressing this (when, as said, it's unclear how to deal with HAP domains; maybe their actively used p2m pages would need including instead)? Or should we rather consider ripping out tboot support? Jan
