On 6/8/21 3:49 AM, Laszlo Ersek wrote: > On 06/07/21 15:37, Brijesh Singh wrote: > >> Also, I was trying to avoid the cases where the malicious hypervisor >> changing the feature value after the GHCB negotiation is completed. >> e.g, during the reset vector they give us one feature value and change >> them during SEC or PEI or DXE instances run. They can't break the >> security of SNP by doing so, but I thought its good to avoid querying >> the features on every phase. > If there is *feasible* security problem (attack), then my "information > flow purity" criteria are irrelevant. Security trumps all. > > My understanding is though (per your explanation above) that there is no > real security problem here. > > Furthermore, we're not going to query the feature set in every phase. > We're going to set the PCDs in the PEI phase; there shouldn't be > hardware querying in the DXE phase. That's quite standard business in OVMF. > > >>> Instead, we should have a new MemEncryptSevLib function that outputs the >>> FEATURES bitmask. It should be similar to MemEncryptSevGetEncryptionMask(), >>> but it should return a RETURN_STATUS, and produce the FEATURES bitmask >>> through an output parameter. >> This is one of thing which I noticed last week, we are actually creating >> a circular dependency. The MemEncryptSevLib provides the routines which >> are used by the VmgExitLib. The MemEncryptSevLib should *not* depend on >> the VmgExitLib. If we extend the MemEncryptSevLib to provide a routine >> to query the features then we will be required to include the >> VmgExitLib. The patch #13 extends the MemEncryptSevLib to provide >> functions for the page validation and it requires the VmgExitLib. I am >> trying to see what I can do to not create this circular dependency and >> still keep code reuse. > As far as I remember, a circular dependency is only a problem if both > library instances in question have constructors. If a library instance > needs no construction (has no constructor), then it does not partake in > the topological sorting (for initialization) performed by BaseTools. > > In this case, at the end of your RFCv3 series (minus patch#21), no INF > file in either "OvmfPkg/Library/BaseMemEncryptSevLib" or > "OvmfPkg/Library/VmgExitLib" seems to specify a CONSTRUCTOR, so purely > from the build perspective, there should be no issue.
I have to debug it, but it did appear that this circular dependency caused problem for the SEV guest when SMM is enabled. If SMM is enabled, then I get a random #UD as soon as I link the VmgExit to MemEncryptSevLib. I will see what I find. > > But, I have another idea here: > >>> The SEC instance of the function should return RETURN_UNSUPPORTED. >>> >>> The PEI instance should use the GHCB MSR protocol, with the help of the >>> AsmCpuId(), AsmWriteMsr64(), AsmReadMsr64() and AsmVmgExit() BaseLib >>> functions. >>> >>> The DXE instance should read back the PCD. > the above basically tells us that this library API would be a single-use > interface. It wouldn't work in SEC, it would do actual work in PEI > (namely, in PlatformPei), and it wouldn't do any "real work" in DXE. > > To me, the boundary is not crystal clear, but the above struggles > *suggest* that we might not want this API to be a MemEncryptSevLib > function (or any library function) at all. If abstracting out the API > causes more pain than it does good, then let's just not abstract it. > > Meaning, you could open-code the fetching of features (using VmgExitLib) > in PlatormPei, set the PCD, and be done with it. The only place where > the PCD (PcdGhcbHypervisorFeatures) is actually used (as far as I can > see) is patch#21, in UefiCpuPkg. We could reasonably say that the "real > API", namely the declaration of the PCD, *already exists*, namely in the > "UefiCpuPkg/UefiCpuPkg.dec" file, from your commit e6994b1d5226 > ("UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs", 2021-06-04). > > There are other examples where a cross-package PCD is set once and for > all in OvmfPkg/PlatformPei (random example: > "PcdCpuBootLogicalProcessorNumber"). We don't have to turn everything > into a lib class API, especially if it causes more pain than help. This is much ideal, I would prefer to avoid creating a new library or add a function into the existing library if this function is only going to be used once during the PEI to query the feature. > ... But maybe I just need to accept that we have to repurpose > SEC_SEV_ES_WORK_AREA, considering it a super-early "HOB list" of sorts. > Same as the PEI phase is considered the "HOB producer phase", outputting > a bunch of disparate bits of info, we could consider the SEV-ES parts of > the Reset Vector such an "early info bits" producer phase. I think this > is a very big conceptual step away from the original purpose of > SEC_SEV_ES_WORK_AREA (note the *name* of the structure: "work area"! > HOBs are not "work areas", they are effectively read-only, once > produced). But perhaps this is what we need (and then with proper > documentation). > > NB however that HOBs have types, GUIDed HOBs have GUIDs, the HOB types > are specified in PI, and GUIDs are expressly declared to stand for > various purposes at least in edk2 DEC files. All that helps with > discerning the information flow. So... I'd still prefer keeping > SEC_SEV_ES_WORK_AREA as minimal as possible. > > Tom, any comments? > > Thank you Brijesh for raising great points! > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76223): https://edk2.groups.io/g/devel/message/76223 Mute This Topic: https://groups.io/mt/83113765/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-