[edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Brijesh Singh posted 28 patches 1 month, 2 weeks ago

[edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Posted by Brijesh Singh 1 month, 2 weeks ago
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
secrets page.

When SEV-SNP is enabled, the secrets page contains the VM platform
communication keys. The guest BIOS and OS can use this key to communicate
with the SEV firmware to get attesation report. See the SEV-SNP firmware
spec for more details for the content of the secrets page.

When SEV and SEV-ES is enabled, the secrets page contains the information
provided by the guest owner after the attestation. See the SEV
LAUNCH_SECRET command for more details.

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/AmdSev/SecretPei/SecretPei.c   | 16 +++++++++++++++-
 OvmfPkg/AmdSev/SecretPei/SecretPei.inf |  1 +
 OvmfPkg/OvmfPkgX64.dsc                 |  2 ++
 OvmfPkg/OvmfPkgX64.fdf                 |  5 +++++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
index ad491515dd..92836c562c 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -7,6 +7,7 @@
 #include <PiPei.h>
 #include <Library/HobLib.h>
 #include <Library/PcdLib.h>
+#include <Library/MemEncryptSevLib.h>
 
 EFI_STATUS
 EFIAPI
@@ -15,10 +16,23 @@ InitializeSecretPei (
   IN CONST EFI_PEI_SERVICES     **PeiServices
   )
 {
+  UINTN   Type;
+
+  //
+  // The secret page should be mapped encrypted by the guest OS and must not
+  // be treated as a system RAM. Mark it as ACPI NVS so that guest OS maps it
+  // encrypted.
+  //
+  if (MemEncryptSevSnpIsEnabled ()) {
+    Type = EfiACPIMemoryNVS;
+  } else {
+    Type = EfiBootServicesData;
+  }
+
   BuildMemoryAllocationHob (
     PcdGet32 (PcdSevLaunchSecretBase),
     PcdGet32 (PcdSevLaunchSecretSize),
-    EfiBootServicesData
+    Type
     );
 
   return EFI_SUCCESS;
diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
index 08be156c4b..9265f8adee 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
@@ -26,6 +26,7 @@
   HobLib
   PeimEntryPoint
   PcdLib
+  MemEncryptSevLib
 
 [FixedPcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index a7d747f6b4..593c0e69f6 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -716,6 +716,7 @@
   OvmfPkg/SmmAccess/SmmAccessPei.inf
 !endif
   UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
 
 !if $(TPM_ENABLE) == TRUE
   OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -965,6 +966,7 @@
   OvmfPkg/PlatformDxe/Platform.inf
   OvmfPkg/AmdSevDxe/AmdSevDxe.inf
   OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
 
 !if $(SMM_REQUIRE) == TRUE
   OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index d519f85328..b04175f77c 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -88,6 +88,9 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE
 0x00C000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
 
+0x00D000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
 0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
@@ -178,6 +181,7 @@ INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
 INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
 INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
 !endif
+INF  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
 
 ################################################################################
 
@@ -313,6 +317,7 @@ INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
 INF  ShellPkg/Application/Shell/Shell.inf
 
 INF MdeModulePkg/Logo/LogoDxe.inf
+INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
 
 #
 # Network modules
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74638): https://edk2.groups.io/g/devel/message/74638
Mute This Topic: https://groups.io/mt/82479058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Posted by Dov Murik 1 month, 2 weeks ago
[+cc: Tobin]

Hi Brijesh,

On 30/04/2021 14:51, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
> secrets page.
> 
> When SEV-SNP is enabled, the secrets page contains the VM platform
> communication keys. The guest BIOS and OS can use this key to communicate
> with the SEV firmware to get attesation report. See the SEV-SNP firmware
> spec for more details for the content of the secrets page.
> 
> When SEV and SEV-ES is enabled, the secrets page contains the information
> provided by the guest owner after the attestation. See the SEV
> LAUNCH_SECRET command for more details.
> 
> 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/AmdSev/SecretPei/SecretPei.c   | 16 +++++++++++++++-
>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf |  1 +
>  OvmfPkg/OvmfPkgX64.dsc                 |  2 ++
>  OvmfPkg/OvmfPkgX64.fdf                 |  5 +++++
>  4 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
> index ad491515dd..92836c562c 100644
> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
> @@ -7,6 +7,7 @@
>  #include <PiPei.h>
>  #include <Library/HobLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/MemEncryptSevLib.h>
> 
>  EFI_STATUS
>  EFIAPI
> @@ -15,10 +16,23 @@ InitializeSecretPei (
>    IN CONST EFI_PEI_SERVICES     **PeiServices
>    )
>  {
> +  UINTN   Type;
> +
> +  //
> +  // The secret page should be mapped encrypted by the guest OS and must not
> +  // be treated as a system RAM. Mark it as ACPI NVS so that guest OS maps it
> +  // encrypted.
> +  //
> +  if (MemEncryptSevSnpIsEnabled ()) {
> +    Type = EfiACPIMemoryNVS;
> +  } else {
> +    Type = EfiBootServicesData;
> +  }
> +

Would it make sense to always use EfiACPIMemoryNVS for the injected secret area, even for regular SEV (non-SNP)?

-Dov



>    BuildMemoryAllocationHob (
>      PcdGet32 (PcdSevLaunchSecretBase),
>      PcdGet32 (PcdSevLaunchSecretSize),
> -    EfiBootServicesData
> +    Type
>      );
> 
>    return EFI_SUCCESS;
> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
> index 08be156c4b..9265f8adee 100644
> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
> @@ -26,6 +26,7 @@
>    HobLib
>    PeimEntryPoint
>    PcdLib
> +  MemEncryptSevLib
> 
>  [FixedPcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index a7d747f6b4..593c0e69f6 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -716,6 +716,7 @@
>    OvmfPkg/SmmAccess/SmmAccessPei.inf
>  !endif
>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> +  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
> 
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -965,6 +966,7 @@
>    OvmfPkg/PlatformDxe/Platform.inf
>    OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>    OvmfPkg/IoMmuDxe/IoMmuDxe.inf
> +  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> 
>  !if $(SMM_REQUIRE) == TRUE
>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index d519f85328..b04175f77c 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -88,6 +88,9 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE
>  0x00C000|0x001000
>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
> 
> +0x00D000|0x001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
> +
>  0x010000|0x010000
>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
> 
> @@ -178,6 +181,7 @@ INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> +INF  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
> 
>  ################################################################################
> 
> @@ -313,6 +317,7 @@ INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>  INF  ShellPkg/Application/Shell/Shell.inf
> 
>  INF MdeModulePkg/Logo/LogoDxe.inf
> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> 
>  #
>  # Network modules
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74759): https://edk2.groups.io/g/devel/message/74759
Mute This Topic: https://groups.io/mt/82479058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Posted by Brijesh Singh 1 month, 2 weeks ago
On 5/5/21 1:42 AM, Dov Murik wrote:
> [+cc: Tobin]
>
> Hi Brijesh,
>
> On 30/04/2021 14:51, Brijesh Singh wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C93168c94eb6d44ed08e608d90f910426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557937779907471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLpmk3G%2BmXcZrzXxCmO3M9EDPiLRnP1IUmPqRQNbBuU%3D&amp;reserved=0
>>
>> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
>> secrets page.
>>
>> When SEV-SNP is enabled, the secrets page contains the VM platform
>> communication keys. The guest BIOS and OS can use this key to communicate
>> with the SEV firmware to get attesation report. See the SEV-SNP firmware
>> spec for more details for the content of the secrets page.
>>
>> When SEV and SEV-ES is enabled, the secrets page contains the information
>> provided by the guest owner after the attestation. See the SEV
>> LAUNCH_SECRET command for more details.
>>
>> 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/AmdSev/SecretPei/SecretPei.c   | 16 +++++++++++++++-
>>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf |  1 +
>>  OvmfPkg/OvmfPkgX64.dsc                 |  2 ++
>>  OvmfPkg/OvmfPkgX64.fdf                 |  5 +++++
>>  4 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>> index ad491515dd..92836c562c 100644
>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>> @@ -7,6 +7,7 @@
>>  #include <PiPei.h>
>>  #include <Library/HobLib.h>
>>  #include <Library/PcdLib.h>
>> +#include <Library/MemEncryptSevLib.h>
>>
>>  EFI_STATUS
>>  EFIAPI
>> @@ -15,10 +16,23 @@ InitializeSecretPei (
>>    IN CONST EFI_PEI_SERVICES     **PeiServices
>>    )
>>  {
>> +  UINTN   Type;
>> +
>> +  //
>> +  // The secret page should be mapped encrypted by the guest OS and must not
>> +  // be treated as a system RAM. Mark it as ACPI NVS so that guest OS maps it
>> +  // encrypted.
>> +  //
>> +  if (MemEncryptSevSnpIsEnabled ()) {
>> +    Type = EfiACPIMemoryNVS;
>> +  } else {
>> +    Type = EfiBootServicesData;
>> +  }
>> +
> Would it make sense to always use EfiACPIMemoryNVS for the injected secret area, even for regular SEV (non-SNP)?

Ideally yes. Maybe James had some reasons for choosing the
EfiBootServicesData. If I had to guess, it was mainly because there no
guest kernel support which consumes the SEV secrets page. Since the
memory is not marked ACPI NVS, so it can be used as a system RAM after
the ExitBootServices is called in the kernel.

I am fine with using ACPI NVS for both SEV and SEV-SNP. I was not able
to build and run AmdSev package in my setup, can you submit a prepatch
to change the memory type and verify that it works ?

>
> -Dov
>
>
>
>>    BuildMemoryAllocationHob (
>>      PcdGet32 (PcdSevLaunchSecretBase),
>>      PcdGet32 (PcdSevLaunchSecretSize),
>> -    EfiBootServicesData
>> +    Type
>>      );
>>
>>    return EFI_SUCCESS;
>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>> index 08be156c4b..9265f8adee 100644
>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>> @@ -26,6 +26,7 @@
>>    HobLib
>>    PeimEntryPoint
>>    PcdLib
>> +  MemEncryptSevLib
>>
>>  [FixedPcd]
>>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index a7d747f6b4..593c0e69f6 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -716,6 +716,7 @@
>>    OvmfPkg/SmmAccess/SmmAccessPei.inf
>>  !endif
>>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>> +  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>
>>  !if $(TPM_ENABLE) == TRUE
>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> @@ -965,6 +966,7 @@
>>    OvmfPkg/PlatformDxe/Platform.inf
>>    OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>    OvmfPkg/IoMmuDxe/IoMmuDxe.inf
>> +  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>
>>  !if $(SMM_REQUIRE) == TRUE
>>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index d519f85328..b04175f77c 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -88,6 +88,9 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE
>>  0x00C000|0x001000
>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
>>
>> +0x00D000|0x001000
>> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
>> +
>>  0x010000|0x010000
>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>
>> @@ -178,6 +181,7 @@ INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>  !endif
>> +INF  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>
>>  ################################################################################
>>
>> @@ -313,6 +317,7 @@ INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>>  INF  ShellPkg/Application/Shell/Shell.inf
>>
>>  INF MdeModulePkg/Logo/LogoDxe.inf
>> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>
>>  #
>>  # Network modules
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74763): https://edk2.groups.io/g/devel/message/74763
Mute This Topic: https://groups.io/mt/82479058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Posted by Laszlo Ersek 1 month, 2 weeks ago
On 05/05/21 15:11, Brijesh Singh wrote:
> 
> On 5/5/21 1:42 AM, Dov Murik wrote:
>> [+cc: Tobin]
>>
>> Hi Brijesh,
>>
>> On 30/04/2021 14:51, Brijesh Singh wrote:
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C93168c94eb6d44ed08e608d90f910426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557937779907471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLpmk3G%2BmXcZrzXxCmO3M9EDPiLRnP1IUmPqRQNbBuU%3D&amp;reserved=0
>>>
>>> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
>>> secrets page.
>>>
>>> When SEV-SNP is enabled, the secrets page contains the VM platform
>>> communication keys. The guest BIOS and OS can use this key to communicate
>>> with the SEV firmware to get attesation report. See the SEV-SNP firmware
>>> spec for more details for the content of the secrets page.
>>>
>>> When SEV and SEV-ES is enabled, the secrets page contains the information
>>> provided by the guest owner after the attestation. See the SEV
>>> LAUNCH_SECRET command for more details.
>>>
>>> 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/AmdSev/SecretPei/SecretPei.c   | 16 +++++++++++++++-
>>>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf |  1 +
>>>  OvmfPkg/OvmfPkgX64.dsc                 |  2 ++
>>>  OvmfPkg/OvmfPkgX64.fdf                 |  5 +++++
>>>  4 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>> index ad491515dd..92836c562c 100644
>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>> @@ -7,6 +7,7 @@
>>>  #include <PiPei.h>
>>>  #include <Library/HobLib.h>
>>>  #include <Library/PcdLib.h>
>>> +#include <Library/MemEncryptSevLib.h>
>>>
>>>  EFI_STATUS
>>>  EFIAPI
>>> @@ -15,10 +16,23 @@ InitializeSecretPei (
>>>    IN CONST EFI_PEI_SERVICES     **PeiServices
>>>    )
>>>  {
>>> +  UINTN   Type;
>>> +
>>> +  //
>>> +  // The secret page should be mapped encrypted by the guest OS and must not
>>> +  // be treated as a system RAM. Mark it as ACPI NVS so that guest OS maps it
>>> +  // encrypted.
>>> +  //
>>> +  if (MemEncryptSevSnpIsEnabled ()) {
>>> +    Type = EfiACPIMemoryNVS;
>>> +  } else {
>>> +    Type = EfiBootServicesData;
>>> +  }
>>> +
>> Would it make sense to always use EfiACPIMemoryNVS for the injected secret area, even for regular SEV (non-SNP)?
> 
> Ideally yes. Maybe James had some reasons for choosing the
> EfiBootServicesData. If I had to guess, it was mainly because there no
> guest kernel support which consumes the SEV secrets page.

git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
reserve the Sev Secret area", 2020-12-14).

Commit bff2811c6d99 makes it clear that the area in question lives in MEMFD.

We're populating the area in the PEI phase. We don't want anything in
DXE to overwrite it.

Once the bootloader (and/or perhaps the kernel's EFI stub) fetched the
secret from that particular location, there is no need to prevent later
parts of the OS (the actual kernel) from repurposing that area. That's
why EfiBootServicesData was used.

> Since the
> memory is not marked ACPI NVS, so it can be used as a system RAM after
> the ExitBootServices is called in the kernel.

Yes.

I don't think AcpiNVS would be a good fit. Linux saves and restores
AcpiNVS areas upon S3 suspend/resume. Regardless of whether S3 works, or
will work, in SEV* guests, if we don't want the guest kernel to touch
that area *at all*, Reserved is a better type.

Please refer to "Table 7-6 Memory Type Usage after ExitBootServices()"
in the UEFI spec (v2.9).

> 
> I am fine with using ACPI NVS for both SEV and SEV-SNP. I was not able
> to build and run AmdSev package in my setup, can you submit a prepatch
> to change the memory type and verify that it works ?

NB: I've not yet reached this patch in my own review of the series, so
I'm likely missing some context. I do have a thought -- under SEV-SNP,
the secrets page apparently needs different (stronger) protection from
the host as under plain SEV. I don't think that hiding the different
protection requirements behind a single common memory type is helpful.
Not to mention the wasted memory in the plain SEV case -- it's not a lot
of memory, mind you, but the principle matters.

So ATM I would like to keep this patch in the SEV-SNP series, and to
preserve the different memory types between SEV and SEV-SNP.

Thanks
Laszlo




> 
>>
>> -Dov
>>
>>
>>
>>>    BuildMemoryAllocationHob (
>>>      PcdGet32 (PcdSevLaunchSecretBase),
>>>      PcdGet32 (PcdSevLaunchSecretSize),
>>> -    EfiBootServicesData
>>> +    Type
>>>      );
>>>
>>>    return EFI_SUCCESS;
>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>> index 08be156c4b..9265f8adee 100644
>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>> @@ -26,6 +26,7 @@
>>>    HobLib
>>>    PeimEntryPoint
>>>    PcdLib
>>> +  MemEncryptSevLib
>>>
>>>  [FixedPcd]
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index a7d747f6b4..593c0e69f6 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -716,6 +716,7 @@
>>>    OvmfPkg/SmmAccess/SmmAccessPei.inf
>>>  !endif
>>>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>>> +  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>
>>>  !if $(TPM_ENABLE) == TRUE
>>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>> @@ -965,6 +966,7 @@
>>>    OvmfPkg/PlatformDxe/Platform.inf
>>>    OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>>    OvmfPkg/IoMmuDxe/IoMmuDxe.inf
>>> +  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>>
>>>  !if $(SMM_REQUIRE) == TRUE
>>>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>> index d519f85328..b04175f77c 100644
>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>> @@ -88,6 +88,9 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE
>>>  0x00C000|0x001000
>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
>>>
>>> +0x00D000|0x001000
>>> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
>>> +
>>>  0x010000|0x010000
>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>
>>> @@ -178,6 +181,7 @@ INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>>  !endif
>>> +INF  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>
>>>  ################################################################################
>>>
>>> @@ -313,6 +317,7 @@ INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>>>  INF  ShellPkg/Application/Shell/Shell.inf
>>>
>>>  INF MdeModulePkg/Logo/LogoDxe.inf
>>> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>>
>>>  #
>>>  # Network modules
>>>
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74773): https://edk2.groups.io/g/devel/message/74773
Mute This Topic: https://groups.io/mt/82479058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Posted by James Bottomley 1 month, 1 week ago
On Wed, 2021-05-05 at 21:33 +0200, Laszlo Ersek wrote:
> On 05/05/21 15:11, Brijesh Singh wrote:
> > On 5/5/21 1:42 AM, Dov Murik wrote:
[...]
> > > Would it make sense to always use EfiACPIMemoryNVS for the
> > > injected secret area, even for regular SEV (non-SNP)?
> > 
> > Ideally yes. Maybe James had some reasons for choosing the
> > EfiBootServicesData. If I had to guess, it was mainly because there
> > no guest kernel support which consumes the SEV secrets page.
> 
> git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
> reserve the Sev Secret area", 2020-12-14).
> 
> Commit bff2811c6d99 makes it clear that the area in question lives in
> MEMFD.
> 
> We're populating the area in the PEI phase. We don't want anything in
> DXE to overwrite it.
> 
> Once the bootloader (and/or perhaps the kernel's EFI stub) fetched
> the secret from that particular location, there is no need to prevent
> later parts of the OS (the actual kernel) from repurposing that area.
> That's why EfiBootServicesData was used.

That's right: originally the design was not to have the boot secrets
survive boot because they should already be copied into their correct,
and presumably protected, locations by the time exit boot services
comes.  The grub code actually shreds the secret in the page once it
consumes it, so the area for a simple disk secret should be empty
anyway.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74800): https://edk2.groups.io/g/devel/message/74800
Mute This Topic: https://groups.io/mt/82479058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Posted by Dov Murik 1 month, 1 week ago

On 05/05/2021 22:33, Laszlo Ersek wrote:
> On 05/05/21 15:11, Brijesh Singh wrote:
>>
>> On 5/5/21 1:42 AM, Dov Murik wrote:
>>> [+cc: Tobin]
>>>
>>> Hi Brijesh,
>>>
>>> On 30/04/2021 14:51, Brijesh Singh wrote:
>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C93168c94eb6d44ed08e608d90f910426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557937779907471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLpmk3G%2BmXcZrzXxCmO3M9EDPiLRnP1IUmPqRQNbBuU%3D&amp;reserved=0
>>>>
>>>> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
>>>> secrets page.
>>>>
>>>> When SEV-SNP is enabled, the secrets page contains the VM platform
>>>> communication keys. The guest BIOS and OS can use this key to communicate
>>>> with the SEV firmware to get attesation report. See the SEV-SNP firmware
>>>> spec for more details for the content of the secrets page.
>>>>
>>>> When SEV and SEV-ES is enabled, the secrets page contains the information
>>>> provided by the guest owner after the attestation. See the SEV
>>>> LAUNCH_SECRET command for more details.
>>>>
>>>> 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/AmdSev/SecretPei/SecretPei.c   | 16 +++++++++++++++-
>>>>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf |  1 +
>>>>  OvmfPkg/OvmfPkgX64.dsc                 |  2 ++
>>>>  OvmfPkg/OvmfPkgX64.fdf                 |  5 +++++
>>>>  4 files changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>>> index ad491515dd..92836c562c 100644
>>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>>> @@ -7,6 +7,7 @@
>>>>  #include <PiPei.h>
>>>>  #include <Library/HobLib.h>
>>>>  #include <Library/PcdLib.h>
>>>> +#include <Library/MemEncryptSevLib.h>
>>>>
>>>>  EFI_STATUS
>>>>  EFIAPI
>>>> @@ -15,10 +16,23 @@ InitializeSecretPei (
>>>>    IN CONST EFI_PEI_SERVICES     **PeiServices
>>>>    )
>>>>  {
>>>> +  UINTN   Type;
>>>> +
>>>> +  //
>>>> +  // The secret page should be mapped encrypted by the guest OS and must not
>>>> +  // be treated as a system RAM. Mark it as ACPI NVS so that guest OS maps it
>>>> +  // encrypted.
>>>> +  //
>>>> +  if (MemEncryptSevSnpIsEnabled ()) {
>>>> +    Type = EfiACPIMemoryNVS;
>>>> +  } else {
>>>> +    Type = EfiBootServicesData;
>>>> +  }
>>>> +
>>> Would it make sense to always use EfiACPIMemoryNVS for the injected secret area, even for regular SEV (non-SNP)?
>>
>> Ideally yes. Maybe James had some reasons for choosing the
>> EfiBootServicesData. If I had to guess, it was mainly because there no
>> guest kernel support which consumes the SEV secrets page.
> 
> git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
> reserve the Sev Secret area", 2020-12-14).
> 
> Commit bff2811c6d99 makes it clear that the area in question lives in MEMFD.
> 
> We're populating the area in the PEI phase. We don't want anything in
> DXE to overwrite it.
> 
> Once the bootloader (and/or perhaps the kernel's EFI stub) fetched the
> secret from that particular location, there is no need to prevent later
> parts of the OS (the actual kernel) from repurposing that area. That's
> why EfiBootServicesData was used.
> 

The first use of the secret area was to hold the guest luks disk passphrase; this is used in the grub-inside-OVMF (AmdSev package), and there was no need to keep that page around for the guest kernel.

The reason I'm raising this whole point is that we're working now on guest-kernel support for reading secrets from that injected page (for plain SEV).  We considered either (a) modifying the secrets page memory type to reserved here, or (b) add code to the kernel EFI stub that would copy this page somewhere else for kernel's later use (which seems more work and not sure what's the advantage).

Option (b) seems harder and more fragile, and I'm not sure if there are any advantages (though I'm definitely not an expert in that area).




>> Since the
>> memory is not marked ACPI NVS, so it can be used as a system RAM after
>> the ExitBootServices is called in the kernel.
> 
> Yes.
> 
> I don't think AcpiNVS would be a good fit. Linux saves and restores
> AcpiNVS areas upon S3 suspend/resume. Regardless of whether S3 works, or
> will work, in SEV* guests, if we don't want the guest kernel to touch
> that area *at all*, Reserved is a better type.

Thanks for this clarification.

> 
> Please refer to "Table 7-6 Memory Type Usage after ExitBootServices()"
> in the UEFI spec (v2.9).
> 
>>
>> I am fine with using ACPI NVS for both SEV and SEV-SNP. I was not able
>> to build and run AmdSev package in my setup, can you submit a prepatch
>> to change the memory type and verify that it works ?
> 
> NB: I've not yet reached this patch in my own review of the series, so
> I'm likely missing some context. I do have a thought -- under SEV-SNP,
> the secrets page apparently needs different (stronger) protection from
> the host as under plain SEV. I don't think that hiding the different
> protection requirements behind a single common memory type is helpful.
> Not to mention the wasted memory in the plain SEV case -- it's not a lot
> of memory, mind you, but the principle matters.
>

Like I said above, we have plans to have this small amount of memory available also to the guest OS; so maybe that shouldn't be the driving force in the decision here.

-Dov

 

> So ATM I would like to keep this patch in the SEV-SNP series, and to
> preserve the different memory types between SEV and SEV-SNP.
> 
> Thanks
> Laszlo
> 
> 
> 
> 
>>
>>>
>>> -Dov
>>>
>>>
>>>
>>>>    BuildMemoryAllocationHob (
>>>>      PcdGet32 (PcdSevLaunchSecretBase),
>>>>      PcdGet32 (PcdSevLaunchSecretSize),
>>>> -    EfiBootServicesData
>>>> +    Type
>>>>      );
>>>>
>>>>    return EFI_SUCCESS;
>>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>> index 08be156c4b..9265f8adee 100644
>>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>> @@ -26,6 +26,7 @@
>>>>    HobLib
>>>>    PeimEntryPoint
>>>>    PcdLib
>>>> +  MemEncryptSevLib
>>>>
>>>>  [FixedPcd]
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>> index a7d747f6b4..593c0e69f6 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>> @@ -716,6 +716,7 @@
>>>>    OvmfPkg/SmmAccess/SmmAccessPei.inf
>>>>  !endif
>>>>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>>>> +  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>>
>>>>  !if $(TPM_ENABLE) == TRUE
>>>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>>> @@ -965,6 +966,7 @@
>>>>    OvmfPkg/PlatformDxe/Platform.inf
>>>>    OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>>>    OvmfPkg/IoMmuDxe/IoMmuDxe.inf
>>>> +  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>>>
>>>>  !if $(SMM_REQUIRE) == TRUE
>>>>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>>> index d519f85328..b04175f77c 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>>> @@ -88,6 +88,9 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE
>>>>  0x00C000|0x001000
>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
>>>>
>>>> +0x00D000|0x001000
>>>> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
>>>> +
>>>>  0x010000|0x010000
>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>>
>>>> @@ -178,6 +181,7 @@ INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>>>  !endif
>>>> +INF  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>>
>>>>  ################################################################################
>>>>
>>>> @@ -313,6 +317,7 @@ INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>>>>  INF  ShellPkg/Application/Shell/Shell.inf
>>>>
>>>>  INF MdeModulePkg/Logo/LogoDxe.inf
>>>> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>>>
>>>>  #
>>>>  # Network modules
>>>>
>>
>>
>> 
>>
>>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74793): https://edk2.groups.io/g/devel/message/74793
Mute This Topic: https://groups.io/mt/82479058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Posted by James Bottomley 1 month, 1 week ago
On Thu, 2021-05-06 at 13:57 +0300, Dov Murik wrote:
> 
> On 05/05/2021 22:33, Laszlo Ersek wrote:
> > On 05/05/21 15:11, Brijesh Singh wrote:
> > > On 5/5/21 1:42 AM, Dov Murik wrote:
[...]
> > > > Would it make sense to always use EfiACPIMemoryNVS for the
> > > > injected secret area, even for regular SEV (non-SNP)?
> > > 
> > > Ideally yes. Maybe James had some reasons for choosing the
> > > EfiBootServicesData. If I had to guess, it was mainly because
> > > there no guest kernel support which consumes the SEV secrets
> > > page.
> > 
> > git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
> > reserve the Sev Secret area", 2020-12-14).
> > 
> > Commit bff2811c6d99 makes it clear that the area in question lives
> > in MEMFD.
> > 
> > We're populating the area in the PEI phase. We don't want anything
> > in DXE to overwrite it.
> > 
> > Once the bootloader (and/or perhaps the kernel's EFI stub) fetched
> > the secret from that particular location, there is no need to
> > prevent later parts of the OS (the actual kernel) from repurposing
> > that area. That's why EfiBootServicesData was used.
> > 
> 
> The first use of the secret area was to hold the guest luks disk
> passphrase; this is used in the grub-inside-OVMF (AmdSev package),
> and there was no need to keep that page around for the guest kernel.
> 
> The reason I'm raising this whole point is that we're working now on
> guest-kernel support for reading secrets from that injected page (for
> plain SEV).  We considered either (a) modifying the secrets page
> memory type to reserved here, or (b) add code to the kernel EFI stub
> that would copy this page somewhere else for kernel's later use
> (which seems more work and not sure what's the advantage).

It mirrors the TPM boot log behaviour: the log is stored in boot time
only memory, so the kernel EFI stub has to copy it out.  The reason the
TPM boot log behaves this way is that if the kernel didn't want to
collect it, it still gets freed.  I imagine a similar rationale exists
for the boot secrets: if the kernel isn't interested in them for some
reason, we don't want them to persist.

> Option (b) seems harder and more fragile, and I'm not sure if there
> are any advantages (though I'm definitely not an expert in that
> area).

I think forcing the kernel to consume the secret before exit boot
services is still a good idea because 

   1. If the kernel can't consume the secret it gets freed
   2. Not all secrets are consumed by the kernel, so it can pick the ones
      it wants out and discard the rest
   3. If the kernel is using a secret protection mechanism, that may not
      work for the memfd page, so relocation of the secret might be a more
      secure mechanism.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74801): https://edk2.groups.io/g/devel/message/74801
Mute This Topic: https://groups.io/mt/82479058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH RFC v2 11/28] OvmfPkg: Reserve Secrets page in MEMFD

Posted by Laszlo Ersek 1 month, 1 week ago
Hi Dov,

On 05/06/21 12:57, Dov Murik wrote:
> 
> 
> On 05/05/2021 22:33, Laszlo Ersek wrote:
>> On 05/05/21 15:11, Brijesh Singh wrote:
>>>
>>> On 5/5/21 1:42 AM, Dov Murik wrote:
>>>> [+cc: Tobin]
>>>>
>>>> Hi Brijesh,
>>>>
>>>> On 30/04/2021 14:51, Brijesh Singh wrote:
>>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C93168c94eb6d44ed08e608d90f910426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557937779907471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nLpmk3G%2BmXcZrzXxCmO3M9EDPiLRnP1IUmPqRQNbBuU%3D&amp;reserved=0
>>>>>
>>>>> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
>>>>> secrets page.
>>>>>
>>>>> When SEV-SNP is enabled, the secrets page contains the VM platform
>>>>> communication keys. The guest BIOS and OS can use this key to communicate
>>>>> with the SEV firmware to get attesation report. See the SEV-SNP firmware
>>>>> spec for more details for the content of the secrets page.
>>>>>
>>>>> When SEV and SEV-ES is enabled, the secrets page contains the information
>>>>> provided by the guest owner after the attestation. See the SEV
>>>>> LAUNCH_SECRET command for more details.
>>>>>
>>>>> 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/AmdSev/SecretPei/SecretPei.c   | 16 +++++++++++++++-
>>>>>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf |  1 +
>>>>>  OvmfPkg/OvmfPkgX64.dsc                 |  2 ++
>>>>>  OvmfPkg/OvmfPkgX64.fdf                 |  5 +++++
>>>>>  4 files changed, 23 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>>>> index ad491515dd..92836c562c 100644
>>>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>>>> @@ -7,6 +7,7 @@
>>>>>  #include <PiPei.h>
>>>>>  #include <Library/HobLib.h>
>>>>>  #include <Library/PcdLib.h>
>>>>> +#include <Library/MemEncryptSevLib.h>
>>>>>
>>>>>  EFI_STATUS
>>>>>  EFIAPI
>>>>> @@ -15,10 +16,23 @@ InitializeSecretPei (
>>>>>    IN CONST EFI_PEI_SERVICES     **PeiServices
>>>>>    )
>>>>>  {
>>>>> +  UINTN   Type;
>>>>> +
>>>>> +  //
>>>>> +  // The secret page should be mapped encrypted by the guest OS and must not
>>>>> +  // be treated as a system RAM. Mark it as ACPI NVS so that guest OS maps it
>>>>> +  // encrypted.
>>>>> +  //
>>>>> +  if (MemEncryptSevSnpIsEnabled ()) {
>>>>> +    Type = EfiACPIMemoryNVS;
>>>>> +  } else {
>>>>> +    Type = EfiBootServicesData;
>>>>> +  }
>>>>> +
>>>> Would it make sense to always use EfiACPIMemoryNVS for the injected secret area, even for regular SEV (non-SNP)?
>>>
>>> Ideally yes. Maybe James had some reasons for choosing the
>>> EfiBootServicesData. If I had to guess, it was mainly because there no
>>> guest kernel support which consumes the SEV secrets page.
>>
>> git-blame fingers commit bff2811c6d99 ("OvmfPkg/AmdSev: assign and
>> reserve the Sev Secret area", 2020-12-14).
>>
>> Commit bff2811c6d99 makes it clear that the area in question lives in MEMFD.
>>
>> We're populating the area in the PEI phase. We don't want anything in
>> DXE to overwrite it.
>>
>> Once the bootloader (and/or perhaps the kernel's EFI stub) fetched the
>> secret from that particular location, there is no need to prevent later
>> parts of the OS (the actual kernel) from repurposing that area. That's
>> why EfiBootServicesData was used.
>>
> 
> The first use of the secret area was to hold the guest luks disk passphrase; this is used in the grub-inside-OVMF (AmdSev package), and there was no need to keep that page around for the guest kernel.
> 
> The reason I'm raising this whole point is that we're working now on guest-kernel support for reading secrets from that injected page (for plain SEV).  We considered either (a) modifying the secrets page memory type to reserved here, or (b) add code to the kernel EFI stub that would copy this page somewhere else for kernel's later use (which seems more work and not sure what's the advantage).
> 
> Option (b) seems harder and more fragile, and I'm not sure if there are any advantages (though I'm definitely not an expert in that area).
> 
> 
> 
> 
>>> Since the
>>> memory is not marked ACPI NVS, so it can be used as a system RAM after
>>> the ExitBootServices is called in the kernel.
>>
>> Yes.
>>
>> I don't think AcpiNVS would be a good fit. Linux saves and restores
>> AcpiNVS areas upon S3 suspend/resume. Regardless of whether S3 works, or
>> will work, in SEV* guests, if we don't want the guest kernel to touch
>> that area *at all*, Reserved is a better type.
> 
> Thanks for this clarification.
> 
>>
>> Please refer to "Table 7-6 Memory Type Usage after ExitBootServices()"
>> in the UEFI spec (v2.9).
>>
>>>
>>> I am fine with using ACPI NVS for both SEV and SEV-SNP. I was not able
>>> to build and run AmdSev package in my setup, can you submit a prepatch
>>> to change the memory type and verify that it works ?
>>
>> NB: I've not yet reached this patch in my own review of the series, so
>> I'm likely missing some context. I do have a thought -- under SEV-SNP,
>> the secrets page apparently needs different (stronger) protection from
>> the host as under plain SEV. I don't think that hiding the different
>> protection requirements behind a single common memory type is helpful.
>> Not to mention the wasted memory in the plain SEV case -- it's not a lot
>> of memory, mind you, but the principle matters.
>>
> 
> Like I said above, we have plans to have this small amount of memory available also to the guest OS; so maybe that shouldn't be the driving force in the decision here.

What you describe (= runtime guest OS access) seems to justify changing
the memory type of this allocation. However, that update doesn't look
tied to SEV-SNP -- it's basically a "change of use case" (change of
purpose) under plain SEV too. I think that deserves a separate
(stand-alone) patch; maybe even a separate TianoCore BZ ticket.

Thanks
Laszlo



> 
> -Dov
> 
>  
> 
>> So ATM I would like to keep this patch in the SEV-SNP series, and to
>> preserve the different memory types between SEV and SEV-SNP.
>>
>> Thanks
>> Laszlo
>>
>>
>>
>>
>>>
>>>>
>>>> -Dov
>>>>
>>>>
>>>>
>>>>>    BuildMemoryAllocationHob (
>>>>>      PcdGet32 (PcdSevLaunchSecretBase),
>>>>>      PcdGet32 (PcdSevLaunchSecretSize),
>>>>> -    EfiBootServicesData
>>>>> +    Type
>>>>>      );
>>>>>
>>>>>    return EFI_SUCCESS;
>>>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>>> index 08be156c4b..9265f8adee 100644
>>>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>>> @@ -26,6 +26,7 @@
>>>>>    HobLib
>>>>>    PeimEntryPoint
>>>>>    PcdLib
>>>>> +  MemEncryptSevLib
>>>>>
>>>>>  [FixedPcd]
>>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
>>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>>> index a7d747f6b4..593c0e69f6 100644
>>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>>> @@ -716,6 +716,7 @@
>>>>>    OvmfPkg/SmmAccess/SmmAccessPei.inf
>>>>>  !endif
>>>>>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>>>>> +  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>>>
>>>>>  !if $(TPM_ENABLE) == TRUE
>>>>>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>>>> @@ -965,6 +966,7 @@
>>>>>    OvmfPkg/PlatformDxe/Platform.inf
>>>>>    OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>>>>    OvmfPkg/IoMmuDxe/IoMmuDxe.inf
>>>>> +  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>>>>
>>>>>  !if $(SMM_REQUIRE) == TRUE
>>>>>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>>>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>>>> index d519f85328..b04175f77c 100644
>>>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>>>> @@ -88,6 +88,9 @@ gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevE
>>>>>  0x00C000|0x001000
>>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
>>>>>
>>>>> +0x00D000|0x001000
>>>>> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
>>>>> +
>>>>>  0x010000|0x010000
>>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>>>
>>>>> @@ -178,6 +181,7 @@ INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>>>>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>>>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>>>>  !endif
>>>>> +INF  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>>>
>>>>>  ################################################################################
>>>>>
>>>>> @@ -313,6 +317,7 @@ INF  OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>>>>>  INF  ShellPkg/Application/Shell/Shell.inf
>>>>>
>>>>>  INF MdeModulePkg/Logo/LogoDxe.inf
>>>>> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>>>>
>>>>>  #
>>>>>  # Network modules
>>>>>
>>>
>>>
>>> 
>>>
>>>
>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74797): https://edk2.groups.io/g/devel/message/74797
Mute This Topic: https://groups.io/mt/82479058/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-