Re: [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field


Laszlo Ersek
 

On 06/04/21 16:15, Laszlo Ersek wrote:
On 05/27/21 01:10, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

Extend the workarea to include the SEV-SNP enabled fields. This will be set
when SEV-SNP is active in the guest VM.

Cc: James Bottomley <jejb@...>
Cc: Min Xu <min.m.xu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Erdem Aktas <erdemaktas@...>
Signed-off-by: Brijesh Singh <brijesh.singh@...>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/Include/Library/MemEncryptSevLib.h | 3 ++-
OvmfPkg/PlatformPei/AmdSev.c | 26 ++++++++++++++++++++++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 12 ++++++++++
OvmfPkg/ResetVector/ResetVector.nasmb | 1 +
5 files changed, 42 insertions(+), 1 deletion(-)
(1) Please split this in two patches -- the PlatformPei changes should
be a separate patch. And, I think those should come second, the
ResetVector + header file change should come first.


diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..bc1dcac48343 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -110,6 +110,7 @@ [Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
+ gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled

[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index 2425d8ba0a36..24507de55c5d 100644
--- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
+++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
@@ -49,7 +49,8 @@ typedef struct {
//
typedef struct _SEC_SEV_ES_WORK_AREA {
UINT8 SevEsEnabled;
- UINT8 Reserved1[7];
+ UINT8 SevSnpEnabled;
+ UINT8 Reserved2[6];

UINT64 RandomData;

diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index a8bf610022ba..67b78fd5fa36 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -22,6 +22,27 @@

#include "Platform.h"

+/**
+
+ Initialize SEV-SNP support if running as an SEV-SNP guest.
+
+ **/
+STATIC
+VOID
+AmdSevSnpInitialize (
+ VOID
+ )
+{
+ RETURN_STATUS PcdStatus;
+
+ if (!MemEncryptSevSnpIsEnabled ()) {
+ return;
+ }
+
+ PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE);
+ ASSERT_RETURN_ERROR (PcdStatus);
+}
+
/**

Initialize SEV-ES support if running as an SEV-ES guest.
@@ -209,4 +230,9 @@ AmdSevInitialize (
// Check and perform SEV-ES initialization if required.
//
AmdSevEsInitialize ();
+
+ //
+ // Check and perform SEV-SNP initialization if required.
+ //
+ AmdSevSnpInitialize ();
}
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 5fae8986d9da..6838cdeec9c3 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -81,6 +81,11 @@ CheckSevFeatures:
; the MSR check below will set the first byte of the workarea to one.
mov byte[SEV_ES_WORK_AREA], 0

+ ; Set the SevSnpEnabled field in workarea to zero to communicate to the SEC
+ ; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this function
+ ; will set it to 1.
+ mov byte[SEV_ES_WORK_AREA_SNP], 0
+
;
; Set up exception handlers to check for SEV-ES
; Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
@@ -136,6 +141,13 @@ CheckSevFeatures:
; phase that SEV-ES is enabled.
mov byte[SEV_ES_WORK_AREA], 1

+ bt eax, 2
+ jnc GetSevEncBit
+
+ ; Set the second byte of the workarea to one to communicate to the SEC
+ ; phase that the SEV-SNP is enabled
+ mov byte[SEV_ES_WORK_AREA_SNP], 1
+
GetSevEncBit:
; Get pte bit position to enable memory encryption
; CPUID Fn8000_001F[EBX] - Bits 5:0
(2) Please mention in the commit message (of the ResetVector patch),
and/or a comment here in the code, that SEV-SNP is never enabled if
SEV-ES is disabled.

Section "15.34.10 SEV_STATUS MSR" in the APM (doc#24593 v3.37) does not
spell out this dependency.

Furthermore, the mSevStatus / mSevEsStatus / mSevSnpStatus variable
assignments in patch#2 do not form a "dependency cascade" like the one
seen here in the reset vector code.

While "SEV-ES depends on SEV" seems obvious to me (and so the related,
existent jumps in the assembly code are not surprising), the statement
"SEV-SNP depends on SEV-ES" is not *that* obvious to me. Thus a comment
would be welcome.

For *both* patches split out of this one:

Reviewed-by: Laszlo Ersek <lersek@...>
(3) Actually, no.

This patch should be reduced to the following files only:

- OvmfPkg/PlatformPei/AmdSev.c
- OvmfPkg/PlatformPei/PlatformPei.inf

and the following changes should be dropped completely:

- OvmfPkg/Include/Library/MemEncryptSevLib.h
- OvmfPkg/ResetVector/Ia32/PageTables64.asm
- OvmfPkg/ResetVector/ResetVector.nasmb

Specifically, the "SEC_SEV_ES_WORK_AREA.SevSnpEnabled" field should
never be introduced.

The reason is apparent only from patch #10 -- "OvmfPkg/PlatformPei:
register GHCB gpa for the SEV-SNP guest".

The core idea is that in patch#10, in the SEC module, you can implement
SevSnpIsEnabled() by just reading MSR_SEV_STATUS, and checking the SNP
bit. Namely, while the SevSnpIsEnabled() call is made in
SevEsProtocolCheck(), i.e., before exception handling is set up in the
SEC module -- and so you indeed cannot call CPUID --, you don't *have*
to call CPUID at that call site. Where you call SevSnpIsEnabled() in
SevEsProtocolCheck(), you already know that SEV-ES is enabled, so it's
safe to just read the exact same SEV status MSR that the SEV-ES status
comes from in the first place, without any CPUID safety check.

(

General request: please be explicit in the commit messages about the
data flow between modules, and why you are doing what you are doing.
Arriving at the above analysis took me 3+ hours this morning, when --
while reviewing patch#4 -- I took issue with the proliferation of the
new fields in SEC_SEV_ES_WORK_AREA. SEC_SEV_ES_WORK_AREA is *not* a
convenience dump. We should restrict its use as much as possible.

I double-checked how SEC_SEV_ES_WORK_AREA had evolved historically:


* SEC_SEV_ES_WORK_AREA.SevEsEnabled:

1 43c3df78460d OvmfPkg: Reserve a page in memory for the SEV-ES usage
2 0731236fc108 OvmfPkg/PlatformPei: Reserve SEV-ES work area if S3 is supported
3 8a2732186a53 OvmfPkg/ResetVector: Add support for a 32-bit SEV check
4 13e5492bfdf3 OvmfPkg/Sec: Add #VC exception handling for Sec phase

The "SEC_SEV_ES_WORK_AREA.SevEsEnabled" field is important for the
following reason:

- in an SEV-ES guest, just learning about SEV requires exception
handling; thus, the Reset Vector sets up exception handling
*unconditionally*,

- in SEC, we deal with exception handling regardless of SEV-ES, but
*how* we do that is conditional on SEV-ES.

This means that caching the SEV-ES presence from the Reset Vector to
SEC makes a lot of sense. Given that in the Reset Vector we have
unconditional exception handling, and then in SEC we have a cached
result, we are allowed to only have conditional exception handling in
SEC.


* SEC_SEV_ES_WORK_AREA.RandomData, SEC_SEV_ES_WORK_AREA.EncryptionMask:

1 7cb96c47a94e OvmfPkg/ResetVector: Validate the encryption bit position for SEV/SEV-ES
2 bd0c1c8e225b OvmfPkg/ResetVector: Perform a simple SEV-ES sanity check
3 3b32be7e7192 OvmfPkg/ResetVector: Save the encryption mask at boot time
4 b97dc4b92ba1 OvmfPkg/MemEncryptSevLib: Add an interface to retrieve the encryption mask

The "RandomData" and "EncryptionMask" fields in the
SEC_SEV_ES_WORK_AREA structure seem justified because they implement
some serious work (which must be done as early as possible, i.e., in
the Reset Vector), *and* caching the result of that work for the rest
of the firmware saves significant complexity (and the result is
security-related even).

"SEC_SEV_ES_WORK_AREA.SevSnpEnabled" is unlike any of these three
fields.

)

Thanks
Laszlo

diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 5fbacaed5f9d..1971557b1c00 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -73,6 +73,7 @@
%define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
%define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
%define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
+ %define SEV_ES_WORK_AREA_SNP (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 1)
%define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
%define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
%define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))




Join devel@edk2.groups.io to automatically receive all group messages.