Re: [edk2-devel] [PATCH RFC v3 04/22] OvmfPkg/MemEncryptSevLib: extend Es Workarea to include hv features

["Brijesh Singh" <brijesh.singh@amd.com>]

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
>