[PATCH V3 0/2] Re-design CcProbeLib

[PATCH V3 0/2] Re-design CcProbeLib

[Min Xu]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3974

CcProbeLib once was designed to probe the Confidential Computing guest
type by checking the PcdOvmfWorkArea. But this memory is allocated with
either EfiACPIMemoryNVS or EfiBootServicesData. It cannot be accessed
after ExitBootService. Please see the detailed analysis in BZ#3974.

To fix this issue, CcProbeLib is re-designed as 2 implementation:
 - SecPeiCcProbeLib
 - DxeCcProbeLib

In SecPeiCcProbeLib we check the CC guest type by reading the
PcdOvmfWorkArea. Because it is used in SEC / PEI and we don't worry about
the issues in BZ#3974.

In DxeCcProbeLib we cache the GuestType in Ovmf work area in first-call.
After that the Guest type is returned with the cached value. So that we
don't need to worry about the access to Ovmf work area after
ExitBootService.

The reason why we probe CC guest type in 2 different ways is the global
varialbe. Global variable cannot be used in SEC/PEI and CcProbe is called
very frequently.

Code: https://github.com/mxu9/edk2/tree/CcProbeLib.BZ3974.v3

v3 changes:
 - Re-design CcProbeLib to 2 implementation: SecPeiCcProbeLib and
   DxeCcProbeLib. The difference between the 2 implementation is the
   cache of the CcGuestType.

v2 changes:
 - Reserve Ovmf work-area as RT_DATA. See
   https://edk2.groups.io/g/devel/message/92599

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Yuan Yu <yuanyu@google.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>

Min M Xu (2):
  OvmfPkg: Add SecPeiCcProbeLib
  OvmfPkg: Update CcProbeLib to DxeCcProbeLib

 OvmfPkg/IntelTdx/IntelTdxX64.dsc              |  3 +-
 OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.c    | 38 +++++++++++++++++++
 .../{CcProbeLib.inf => DxeCcProbeLib.inf}     |  6 +--
 .../{CcProbeLib.c => SecPeiCcProbeLib.c}      |  0
 .../Library/CcProbeLib/SecPeiCcProbeLib.inf   | 25 ++++++++++++
 OvmfPkg/OvmfPkgX64.dsc                        |  5 ++-
 6 files changed, 72 insertions(+), 5 deletions(-)
 create mode 100644 OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.c
 rename OvmfPkg/Library/CcProbeLib/{CcProbeLib.inf => DxeCcProbeLib.inf} (70%)
 rename OvmfPkg/Library/CcProbeLib/{CcProbeLib.c => SecPeiCcProbeLib.c} (100%)
 create mode 100644 OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf

-- 
2.29.2.windows.2

[PATCH V3 1/2] OvmfPkg: Add SecPeiCcProbeLib

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3974

SecPeiCcProbeLib is designed to probe the Confidential Computing guest
type in SEC/PEI phase. The CC guest type was set by each CC guest at
the beginning of boot up and saved in PcdOvmfWorkArea.

Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.c | 31 +++++++++++++++++++
 .../Library/CcProbeLib/SecPeiCcProbeLib.inf   | 25 +++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.c
 create mode 100644 OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf

diff --git a/OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.c b/OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.c
new file mode 100644
index 000000000000..d698e5c8d7f8
--- /dev/null
+++ b/OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.c
@@ -0,0 +1,31 @@
+/** @file
+
+  CcProbeLib is used to probe the Confidential computing guest type.
+
+  Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Library/CcProbeLib.h>
+#include <WorkArea.h>
+
+/**
+  Probe the ConfidentialComputing Guest type. See defition of
+  CC_GUEST_TYPE in <ConfidentialComputingGuestAttr.h>.
+
+  @return The guest type
+
+**/
+UINT8
+EFIAPI
+CcProbe (
+  VOID
+  )
+{
+  OVMF_WORK_AREA  *WorkArea;
+
+  WorkArea = (OVMF_WORK_AREA *)FixedPcdGet32 (PcdOvmfWorkAreaBase);
+
+  return WorkArea != NULL ? WorkArea->Header.GuestType : CcGuestTypeNonEncrypted;
+}
diff --git a/OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf b/OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf
new file mode 100644
index 000000000000..f63ed71e7c28
--- /dev/null
+++ b/OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf
@@ -0,0 +1,25 @@
+## @file
+# CcProbeLib is used to probe Confidential Computing guest type.
+#
+#  Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = SecPeiCcProbeLib
+  FILE_GUID                      = 78eb7f2a-a42a-4b01-b160-5a05a0a52bac
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = CcProbeLib|SEC PEIM PEI_CORE
+
+[Sources]
+  SecPeiCcProbeLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
-- 
2.29.2.windows.2

[PATCH V3 2/2] OvmfPkg: Update CcProbeLib to DxeCcProbeLib

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3974

CcProbeLib once was designed to probe the Confidential Computing guest
type by checking the PcdOvmfWorkArea. But this memory is allocated with
either EfiACPIMemoryNVS or EfiBootServicesData. It cannot be accessed
after ExitBootService. Please see the detailed analysis in BZ#3974.

To fix this issue, CcProbeLib is redesigned as 2 implementation:
 - SecPeiCcProbeLib
 - DxeCcProbeLib

In SecPeiCcProbeLib we check the CC guest type by reading the
PcdOvmfWorkArea. Because it is used in SEC / PEI and we don't worry about
the issues in BZ#3974.

In DxeCcProbeLib we cache the GuestType in Ovmf work area in first-call.
After that the Guest type is returned with the cached value. So that we
don't need to worry about the access to Ovmf work area after
ExitBootService.

The reason why we probe CC guest type in 2 different ways is the global
varialbe. Global variable cannot be used in SEC/PEI and CcProbe is called
very frequently.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/IntelTdx/IntelTdxX64.dsc                      |  3 ++-
 .../CcProbeLib/{CcProbeLib.c => DxeCcProbeLib.c}      | 11 +++++++++--
 .../CcProbeLib/{CcProbeLib.inf => DxeCcProbeLib.inf}  |  6 +++---
 OvmfPkg/OvmfPkgX64.dsc                                |  5 ++++-
 4 files changed, 18 insertions(+), 7 deletions(-)
 rename OvmfPkg/Library/CcProbeLib/{CcProbeLib.c => DxeCcProbeLib.c} (55%)
 rename OvmfPkg/Library/CcProbeLib/{CcProbeLib.inf => DxeCcProbeLib.inf} (70%)

diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 71b1cf8e7090..1a7ecd503e50 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -140,7 +140,7 @@
   PciCapLib|OvmfPkg/Library/BasePciCapLib/BasePciCapLib.inf
   PciCapPciSegmentLib|OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf
   PciCapPciIoLib|OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf
-  CcProbeLib|OvmfPkg/Library/CcProbeLib/CcProbeLib.inf
+  CcProbeLib|OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.inf
   IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
@@ -234,6 +234,7 @@
   HobLib|EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
   PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
   PeilessStartupLib|OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
+  CcProbeLib|OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf
 
 [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
diff --git a/OvmfPkg/Library/CcProbeLib/CcProbeLib.c b/OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.c
similarity index 55%
rename from OvmfPkg/Library/CcProbeLib/CcProbeLib.c
rename to OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.c
index d698e5c8d7f8..11b2eef390cf 100644
--- a/OvmfPkg/Library/CcProbeLib/CcProbeLib.c
+++ b/OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.c
@@ -10,6 +10,9 @@
 #include <Library/CcProbeLib.h>
 #include <WorkArea.h>
 
+STATIC UINT8    mCcProbeGuestType = 0;
+STATIC BOOLEAN  mCcProbed         = FALSE;
+
 /**
   Probe the ConfidentialComputing Guest type. See defition of
   CC_GUEST_TYPE in <ConfidentialComputingGuestAttr.h>.
@@ -25,7 +28,11 @@ CcProbe (
 {
   OVMF_WORK_AREA  *WorkArea;
 
-  WorkArea = (OVMF_WORK_AREA *)FixedPcdGet32 (PcdOvmfWorkAreaBase);
+  if (!mCcProbed) {
+    WorkArea          = (OVMF_WORK_AREA *)FixedPcdGet32 (PcdOvmfWorkAreaBase);
+    mCcProbeGuestType = WorkArea != NULL ? WorkArea->Header.GuestType : CcGuestTypeNonEncrypted;
+    mCcProbed         = TRUE;
+  }
 
-  return WorkArea != NULL ? WorkArea->Header.GuestType : CcGuestTypeNonEncrypted;
+  return mCcProbeGuestType;
 }
diff --git a/OvmfPkg/Library/CcProbeLib/CcProbeLib.inf b/OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.inf
similarity index 70%
rename from OvmfPkg/Library/CcProbeLib/CcProbeLib.inf
rename to OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.inf
index 5300c9ba2644..9bdba4312755 100644
--- a/OvmfPkg/Library/CcProbeLib/CcProbeLib.inf
+++ b/OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.inf
@@ -8,14 +8,14 @@
 
 [Defines]
   INF_VERSION                    = 0x00010005
-  BASE_NAME                      = CcProbeLib
+  BASE_NAME                      = DxeCcProbeLib
   FILE_GUID                      = 05184ec9-abb0-4491-8584-e388639a7c48
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = CcProbeLib
+  LIBRARY_CLASS                  = CcProbeLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER UEFI_APPLICATION
 
 [Sources]
-  CcProbeLib.c
+  DxeCcProbeLib.c
 
 [Packages]
   MdePkg/MdePkg.dec
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 6e68f60dc90f..2741f4d988b7 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -199,7 +199,7 @@
 
 !if $(SMM_REQUIRE) == FALSE
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
-  CcProbeLib|OvmfPkg/Library/CcProbeLib/CcProbeLib.inf
+  CcProbeLib|OvmfPkg/Library/CcProbeLib/DxeCcProbeLib.inf
 !else
   CcProbeLib|MdePkg/Library/CcProbeLibNull/CcProbeLibNull.inf
 !endif
@@ -287,6 +287,7 @@
 !endif
   VmgExitLib|OvmfPkg/Library/VmgExitLib/SecVmgExitLib.inf
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
+  CcProbeLib|OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
@@ -303,6 +304,7 @@
   DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
 !endif
   PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
+  CcProbeLib|OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf
 
 [LibraryClasses.common.PEIM]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
@@ -332,6 +334,7 @@
   PlatformInitLib|OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
 
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
+  CcProbeLib|OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf
 
 [LibraryClasses.common.DXE_CORE]
   HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
-- 
2.29.2.windows.2

Re: [PATCH V3 2/2] OvmfPkg: Update CcProbeLib to DxeCcProbeLib

[Gerd Hoffmann]

  Hi,

> In DxeCcProbeLib we cache the GuestType in Ovmf work area in first-call.
> After that the Guest type is returned with the cached value. So that we
> don't need to worry about the access to Ovmf work area after
> ExitBootService.

This only works in case the first call is early enough.
Better use a CONSTRUCTOR instead.

take care,
  Gerd

Re: [edk2-devel] [PATCH V3 2/2] OvmfPkg: Update CcProbeLib to DxeCcProbeLib

[Min Xu]

On August 26, 2022 1:23 PM, Gerd Hoffmann wrote:
>   Hi,
> 
> > In DxeCcProbeLib we cache the GuestType in Ovmf work area in first-call.
> > After that the Guest type is returned with the cached value. So that
> > we don't need to worry about the access to Ovmf work area after
> > ExitBootService.
> 
> This only works in case the first call is early enough.
> Better use a CONSTRUCTOR instead.
> 
Thanks for reminder.
There is another situation that the first call of DxeCcProbeLib is earlier than its CONSTRUCTOR.
For example, in MdeModulePkg/Core/Dxe/DxeMain, BaseDebugLibSerialPortConstructor is called before DxeCcProbeLibConstructor. While CcProbe () is called in BaseDebugLibSerialPortConstructor.
So I would like to read the Cc guest type in both CONSTRUCTOR and CcProbe.

ProcessLibraryConstructorList (
  IN EFI_HANDLE        ImageHandle,
  IN EFI_SYSTEM_TABLE  *SystemTable
  )
{
  EFI_STATUS  Status;

  Status = BaseDebugLibSerialPortConstructor ();
  ASSERT_RETURN_ERROR (Status);

  Status = DxeCcProbeLibConstructor ();
  ASSERT_RETURN_ERROR (Status);
  ...
  ...
}

Thanks
Min

Re: [edk2-devel] [PATCH V3 2/2] OvmfPkg: Update CcProbeLib to DxeCcProbeLib

[Gerd Hoffmann]

On Fri, Aug 26, 2022 at 08:21:49AM +0000, Xu, Min M wrote:
> On August 26, 2022 1:23 PM, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > In DxeCcProbeLib we cache the GuestType in Ovmf work area in first-call.
> > > After that the Guest type is returned with the cached value. So that
> > > we don't need to worry about the access to Ovmf work area after
> > > ExitBootService.
> > 
> > This only works in case the first call is early enough.
> > Better use a CONSTRUCTOR instead.
> > 
> Thanks for reminder.
> There is another situation that the first call of DxeCcProbeLib is earlier than its CONSTRUCTOR.
> For example, in MdeModulePkg/Core/Dxe/DxeMain, BaseDebugLibSerialPortConstructor is called before DxeCcProbeLibConstructor. While CcProbe () is called in BaseDebugLibSerialPortConstructor.
> So I would like to read the Cc guest type in both CONSTRUCTOR and CcProbe.

Makes sense (and please add a comment explaining this corner case).

take care,
  Gerd