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

["Laszlo Ersek" <lersek@redhat.com>]

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.

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.

... 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