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

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

Hi Laszlo,


On 6/7/21 6:54 AM, Laszlo Ersek wrote:
> Hi Brijesh,
>
> On 05/27/21 01:11, Brijesh Singh wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C107d760e06824b1b6d6d08d929aaffd3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586636747299845%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KQ42j9fOtsJfCh2iziap1v%2BBqC%2FHfR%2BR9ZbO3JHAJaM%3D&reserved=0
>>
>> The GHCB Version 2 introduces advertisement of features that are supported
>> by the hypervisor. The features value is saved in the SevEs workarea. Save
>> the value in the PCD for the later use.
>>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  OvmfPkg/PlatformPei/PlatformPei.inf        |   1 +
>>  OvmfPkg/Include/Library/MemEncryptSevLib.h |   2 +
>>  OvmfPkg/PlatformPei/AmdSev.c               |  26 +++++
>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 122 +++++++++++++++++++++
>>  OvmfPkg/ResetVector/ResetVector.nasmb      |   1 +
>>  5 files changed, 152 insertions(+)
> (1) Please split this patch: the PlatformPei changes should be in the second patch.

Noted.


>
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index bc1dcac48343..3256ccfe88d8 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -111,6 +111,7 @@ [Pcd]
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
>>    gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled
>> +  gUefiCpuPkgTokenSpaceGuid.PcdGhcbHypervisorFeatures
>>  
>>  [FixedPcd]
>>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> index 24507de55c5d..dd1c97d4a9a3 100644
>> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> @@ -55,6 +55,8 @@ typedef struct _SEC_SEV_ES_WORK_AREA {
>>    UINT64   RandomData;
>>  
>>    UINT64   EncryptionMask;
>> +
>> +  UINT64   HypervisorFeatures;
>>  } SEC_SEV_ES_WORK_AREA;
>>  
>>  //
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index 67b78fd5fa36..81e40e0889aa 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -43,6 +43,27 @@ AmdSevSnpInitialize (
>>    ASSERT_RETURN_ERROR (PcdStatus);
>>  }
>>  
>> +/**
>> +
>> +  Function to set the PcdHypervisorFeatures.
>> +**/
>> +STATIC
>> +VOID
>> +AmdSevHypervisorFeatures (
>> +  VOID
>> +  )
>> +{
>> +  SEC_SEV_ES_WORK_AREA  *SevEsWorkArea;
>> +  RETURN_STATUS         PcdStatus;
>> +
>> +  SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
>> +
>> +  PcdStatus = PcdSet64S (PcdGhcbHypervisorFeatures, SevEsWorkArea->HypervisorFeatures);
>> +  ASSERT_RETURN_ERROR (PcdStatus);
>> +
>> +  DEBUG ((DEBUG_INFO, "GHCB Hypervisor Features=0x%Lx\n", SevEsWorkArea->HypervisorFeatures));
> (2) Overlong line.
>
> Please avoid basic mistakes like this, even in an RFC series.

I will split it, but all the checkpatch and CI stuff passed on my branch
before the submission.


>
>
>> +}
>> +
>>  /**
>>  
>>    Initialize SEV-ES support if running as an SEV-ES guest.
>> @@ -73,6 +94,11 @@ AmdSevEsInitialize (
>>    PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
>>    ASSERT_RETURN_ERROR (PcdStatus);
>>  
>> +  //
>> +  // Set the hypervisor features PCD.
>> +  //
>> +  AmdSevHypervisorFeatures ();
>> +
>>    //
>>    // Allocate GHCB and per-CPU variable pages.
>>    //   Since the pages must survive across the UEFI to OS transition
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index 6838cdeec9c3..75e63d2a0561 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -62,6 +62,16 @@ BITS    32
>>  %define GHCB_CPUID_REGISTER_SHIFT  30
>>  %define CPUID_INSN_LEN              2
>>  
>> +; GHCB SEV Information MSR protocol
>> +%define GHCB_SEV_INFORMATION_REQUEST    2
>> +%define GHCB_SEV_INFORMATION_RESPONSE   1
> (3) These macro names do not match the ones in "MdePkg/Include/Register/Amd/Fam17Msr.h" (GHCB_INFO_SEV_INFO_GET, GHCB_INFO_SEV_INFO, respectively).
>
> They don't *have* to match, technically speaking, but one goal of using macros for magic numbers is so we can grep the source for them. The macros just below (for values 128 and 129) do match the header file.

Noted, I will match them.


>
>> +
>> +; GHCB Hypervisor features MSR protocol
>> +%define GHCB_HYPERVISOR_FEATURES_REQUEST    128
>> +%define GHCB_HYPERVISOR_FEATURES_RESPONSE   129
>> +
>> +; GHCB request to terminate protocol values
>> +%define GHCB_GENERAL_TERMINATE_REQUEST    255
> (4) Not only does this macro name not match the one in the header file (which is GHCB_INFO_TERMINATE_REQUEST), even the value is wrong. The header file has
>
> #define GHCB_INFO_TERMINATE_REQUEST                 256
>
> and I checked the GHCBv2 spec too; there is no operation defined with opcode 255.
>
Ah good catch. Thanks


>>  
>>  ; Check if Secure Encrypted Virtualization (SEV) features are enabled.
>>  ;
>> @@ -86,6 +96,13 @@ CheckSevFeatures:
>>      ; will set it to 1.
>>      mov       byte[SEV_ES_WORK_AREA_SNP], 0
>>  
>> +    ; Set the Hypervisor features field in the workarea to zero to communicate
>> +    ; to the hypervisor features to the SEC phase. The hypervisor feature is
>> +    ; filled during the call to CheckHypervisorFeatures.
>> +    mov     eax, 0
>> +    mov     dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES], eax
>> +    mov     dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES + 4], eax
>> +
>>      ;
>>      ; Set up exception handlers to check for SEV-ES
>>      ;   Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
>> @@ -225,6 +242,106 @@ IsSevEsEnabled:
>>  SevEsDisabled:
>>      OneTimeCallRet IsSevEsEnabled
>>  
>> +; The version 2 of GHCB specification added the support to query the hypervisor features.
>> +; If the GHCB version is >=2 then read the hypervisor features.
>> +;
>> +; Modified:  EAX, EBX, ECX, EDX
>> +;
>> +CheckHypervisorFeatures:
> (5) Arguably this label name should contain "Sev".

Noted.


>
>> +    ; Get the SEV Information
>> +    ; Setup GHCB MSR
>> +    ;   GHCB_MSR[11:0]  = SEV information request
>> +    ;
>> +    mov     edx, 0
>> +    mov     eax, GHCB_SEV_INFORMATION_REQUEST
>> +    mov     ecx, 0xc0010130
>> +    wrmsr
>> +
>> +    ;
>> +    ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
>> +    ; mode, so work around this by temporarily switching to 64-bit mode.
>> +    ;
>> +BITS    64
>> +    rep     vmmcall
>> +BITS    32
>> +
>> +    ;
>> +    ; SEV Information Response GHCB MSR
>> +    ;   GHCB_MSR[63:48] = Maximum protocol version
>> +    ;   GHCB_MSR[47:32] = Minimum protocol version
>> +    ;   GHCB_MSR[11:0]  = SEV information response
>> +    ;
>> +    mov     ecx, 0xc0010130
>> +    rdmsr
>> +    and     eax, 0xfff
>> +    cmp     eax, GHCB_SEV_INFORMATION_RESPONSE
>> +    jnz     TerminateSevGuestLaunch
> (6) Before modifying the ResetVector module like this, please insert a refactoring patch as follows:
>
> - A new SEV-specific assembly include file should be introduced. The majority of the "OvmfPkg/ResetVector/Ia32/PageTables64.asm" file now deals with SEV aspects, but the file-top comment still says "Sets the CR3 register for 64-bit paging". It's high time that we move SEV stuff to a file with a name that references SEV.
>
> - We now have five (5) invocations of the GHCB MSR protocol in this file, and every one of them open-codes the same setup, the same 0xc0010130 MSR constant, the same retval check logic with possible guest termination, the same "rep vmmcall" workaround for the 32-bit limitation of NASM, and so on. this file is now borderline unreadable. At the minimum, please introduce a function-like NASM macro with two arguments (= the request & response opcodes), and extract as much as possible.
>
Okay, I will try moving all the SEV specific stuff outside this file.


>
>> +    shr     edx, 16
>> +    cmp     edx, 2
>> +    jl      CheckHypervisorFeaturesDone
>> +
>> +    ; Get the hypervisor features
>> +    ; Setup GHCB MSR
>> +    ;   GHCB_MSR[11:0]  = Hypervisor features request
>> +    ;
>> +    mov     edx, 0
>> +    mov     eax, GHCB_HYPERVISOR_FEATURES_REQUEST
>> +    mov     ecx, 0xc0010130
>> +    wrmsr
>> +
>> +    ;
>> +    ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
>> +    ; mode, so work around this by temporarily switching to 64-bit mode.
>> +    ;
>> +BITS    64
>> +    rep     vmmcall
>> +BITS    32
>> +
>> +    ;
>> +    ; Hypervisor features reponse
>> +    ;   GHCB_MSR[63:12] = Features bitmap
>> +    ;   GHCB_MSR[11:0]  = Hypervisor features response
>> +    ;
>> +    mov     ecx, 0xc0010130
>> +    rdmsr
>> +    mov     ebx, eax
>> +    and     eax, 0xfff
>> +    cmp     eax, GHCB_HYPERVISOR_FEATURES_RESPONSE
>> +    jnz     TerminateSevGuestLaunch
>> +
>> +    shr     ebx, 12
>> +    mov     dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES], ebx
>> +    mov     dword[SEV_ES_WORK_AREA_HYPERVISOR_FEATURES + 4], edx
> (7) According to the spec, the FEATURES bitmap is a contiguous bitmap of 52 bits. The way the EDX:EAX qword is shifted right by 12 bits above is incorrect. The EAX half is shifted OK (through EBX), but the EDX half is not shifted down by 12 bits at all, it is simply stored to the most significant dword of the "HypervisorFeatures" field. This basically inserts a 12 bit wide gap in the FEATURES bitmap.

Noted, I will fix it.


>
>> +
>> +    jmp     CheckHypervisorFeaturesDone
>> +TerminateSevGuestLaunch:
>> +    ;
>> +    ; Setup GHCB MSR
>> +    ;   GHCB_MSR[23:16] = 0
>> +    ;   GHCB_MSR[15:12] = 0
>> +    ;   GHCB_MSR[11:0]  = Terminate Request
>> +    ;
>> +    mov     edx, 0
>> +    mov     eax, GHCB_GENERAL_TERMINATE_REQUEST
> (8) The "MdePkg/Include/Register/Amd/Fam17Msr.h" header file introduces GHCB_TERMINATE_GHCB, GHCB_TERMINATE_GHCB_GENERAL, GHCB_TERMINATE_GHCB_PROTOCOL. Can we use some of those here (with a separate, but matching, NASM macro of course)? See SevEsProtocolFailure() in "OvmfPkg/Sec/SecMain.c".

Sure, while moving SEV bits in a separate file I will try to define a
matching macros for it.


>
>
>> +    mov     ecx, 0xc0010130
>> +    wrmsr
>> +
>> +    ;
>> +    ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
>> +    ; mode, so work around this by temporarily switching to 64-bit mode.
>> +    ;
>> +BITS    64
>> +    rep     vmmcall
>> +BITS    32
>> +
>> +TerminateSevGuestLaunchHlt:
>> +    cli
>> +    hlt
>> +    jmp     TerminateSevGuestLaunchHlt
>> +
>> +CheckHypervisorFeaturesDone:
>> +    OneTimeCallRet CheckHypervisorFeatures
>> +
>>  ;
>>  ; Modified:  EAX, EBX, ECX, EDX
>>  ;
>> @@ -328,6 +445,11 @@ clearGhcbMemoryLoop:
>>      mov     dword[ecx * 4 + GHCB_BASE - 4], eax
>>      loop    clearGhcbMemoryLoop
>>  
>> +    ;
>> +    ; It is SEV-ES guest, query the Hypervisor features
>> +    ;
>> +    OneTimeCall   CheckHypervisorFeatures
>> +
>>  SetCr3:
>>      ;
>>      ; Set CR3 now that the paging structures are available
>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>> index 1971557b1c00..5beba3ecb290 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>> @@ -76,6 +76,7 @@
>>    %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_WORK_AREA_HYPERVISOR_FEATURES (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 24)
>>    %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
>>  %include "Ia32/Flat32ToFlat64.asm"
>>  %include "Ia32/PageTables64.asm"
>>
> (9) And, I'm arriving at the following assertion only now unfortunately, after spending about 4 hours on reviewing this patch, and the history of SEC_SEV_ES_WORK_AREA.
>
> I assert that the "SEC_SEV_ES_WORK_AREA.HypervisorFeatures" field should not exist. The only read site is here, in "OvmfPkg/PlatformPei".

I choose to cache as much as I could during the reset vector time so
that we avoid the need for VmgExit later. But now I understand that you
prefer to query those information when they are needed and not use the
SevEsWorkArea as a cache for other information.

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.


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

Thanks

Brijesh

>
> 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.
>
> And so the OvmfPkg/ResetVector hunks should be dropped from this patch.
>
> Thanks,
> Laszlo
>