public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-platforms: PATCH v2] MinPlatformPkg/SaveMemoryConfig: Variable may not be locked.
@ 2022-02-10  6:59 Chiu, Chasel
  2022-02-10 18:01 ` [edk2-devel] " Oram, Isaac W
  0 siblings, 1 reply; 2+ messages in thread
From: Chiu, Chasel @ 2022-02-10  6:59 UTC (permalink / raw)
  To: devel; +Cc: Chasel Chiu, Nate DeSimone, Liming Gao, Eric Dong

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3829

Fixed the bug that existing variable will not be locked when it is
identical with hob data by creating LockLargeVariable function, also
switched to VariablePolicyProtocol for locking variables.

This patch also modified SaveMemoryConfig driver to be unloaded after
execution because it does not produce any service protocol. To achieve
this goal the DxeRuntimeVariableWriteLib should close registered
ExitBootService events in its DESTRUCTOR.

Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
---

V2 : Created LockLargeVariable function to support locking multiple variable case.

 Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c                    | 12 +++++++++---
 Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c              | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.c   | 62 ++++++++++++++++++++++++++++++++++++++++++++++----------------
 Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib.h                           | 23 ++++++++++++++++++++++-
 Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.inf |  8 +++++---
 5 files changed, 177 insertions(+), 25 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
index 820585f676..6e521bdce6 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
@@ -2,7 +2,7 @@
   This is the driver that locates the MemoryConfigurationData HOB, if it
   exists, and saves the data to nvRAM.
 
-Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2017 - 2022, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -18,6 +18,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/BaseMemoryLib.h>
 #include <Library/LargeVariableReadLib.h>
 #include <Library/LargeVariableWriteLib.h>
+#include <Library/VariableWriteLib.h>
 #include <Guid/FspNonVolatileStorageHob2.h>
 
 /**
@@ -86,6 +87,11 @@ SaveMemoryConfigEntryPoint (
             Status = GetLargeVariable (L"FspNvsBuffer", &gFspNvsBufferVariableGuid, &BufferSize, VariableData);
             if (!EFI_ERROR (Status) && (BufferSize == DataSize) && (0 == CompareMem (HobData, VariableData, DataSize))) {
               DataIsIdentical = TRUE;
+              //
+              // No need to update Variable, only lock it.
+              //
+              Status = LockLargeVariable (L"FspNvsBuffer",  &gFspNvsBufferVariableGuid);
+              ASSERT_EFI_ERROR (Status);
             }
             FreePool (VariableData);
           }
@@ -106,7 +112,7 @@ SaveMemoryConfigEntryPoint (
   }
 
   //
-  // This driver cannot be unloaded because DxeRuntimeVariableWriteLib constructor will register ExitBootServices callback.
+  // This driver does not produce any protocol services, so always unload it.
   //
-  return EFI_SUCCESS;
+  return EFI_REQUEST_UNLOAD_IMAGE;
 }
diff --git a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
index e4b97ef1df..5bee2d6751 100644
--- a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
+++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
@@ -10,7 +10,7 @@
   integer number will be added to the end of the variable name. This number
   will be incremented for each variable as needed to store the entire data set.
 
-  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2021 - 2022, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -412,7 +412,7 @@ SetLargeVariable (
     // all data is saved.
     //
     if (LockVariable) {
-      for (Index = 0; Index < VariablesSaved; Index++) {
+      for (Index = 0; Index <= VariablesSaved; Index++) {
         ZeroMem (TempVariableName, MAX_VARIABLE_NAME_SIZE);
         UnicodeSPrint (TempVariableName, MAX_VARIABLE_NAME_SIZE, L"%s%d", VariableName, Index);
 
@@ -448,3 +448,96 @@ Done:
   DEBUG ((DEBUG_ERROR, "SetLargeVariable: Status = %r\n", Status));
   return Status;
 }
+
+/**
+  Locks the existing large variable.
+
+  @param[in]  VariableName       A Null-terminated string that is the name of the vendor's variable.
+                                 Each VariableName is unique for each VendorGuid. VariableName must
+                                 contain 1 or more characters. If VariableName is an empty string,
+                                 then EFI_INVALID_PARAMETER is returned.
+  @param[in]  VendorGuid         A unique identifier for the vendor.
+  @retval EFI_SUCCESS            The firmware has successfully locked the variable.
+  @retval EFI_INVALID_PARAMETER  An invalid combination of variable name and GUID was supplied
+  @retval EFI_UNSUPPORTED        The service for locking variable is not ready.
+  @retval EFI_NOT_FOUND          The targeting variable for locking is not present.
+
+**/
+EFI_STATUS
+EFIAPI
+LockLargeVariable (
+  IN  CHAR16                       *VariableName,
+  IN  EFI_GUID                     *VendorGuid
+  )
+{
+  CHAR16        TempVariableName[MAX_VARIABLE_NAME_SIZE];
+  UINT64        VariableSize;
+  EFI_STATUS    Status;
+  UINTN         Index;
+
+  //
+  // Check input parameters.
+  //
+  if (VariableName == NULL || VariableName[0] == 0 || VendorGuid == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (!VarLibIsVariableRequestToLockSupported ()) {
+    return EFI_UNSUPPORTED;
+  }
+
+  VariableSize = 0;
+  Index = 0;
+  ZeroMem (TempVariableName, MAX_VARIABLE_NAME_SIZE);
+  UnicodeSPrint (TempVariableName, MAX_VARIABLE_NAME_SIZE, L"%s%d", VariableName, Index);
+  Status = VarLibGetVariable (TempVariableName, VendorGuid, NULL, &VariableSize, NULL);
+  if (Status == EFI_BUFFER_TOO_SMALL) {
+    //
+    // Lock multiple variables.
+    //
+
+    //
+    // Lock first variable and continue to rest of the variables.
+    //
+    DEBUG ((DEBUG_INFO, "Locking %s, Guid = %g\n", TempVariableName, VendorGuid));
+    Status = VarLibVariableRequestToLock (TempVariableName, VendorGuid);
+    for (Index = 1; Index < MAX_VARIABLE_SPLIT; Index++) {
+      ZeroMem (TempVariableName, MAX_VARIABLE_NAME_SIZE);
+      UnicodeSPrint (TempVariableName, MAX_VARIABLE_NAME_SIZE, L"%s%d", VariableName, Index);
+
+      VariableSize = 0;
+      Status = VarLibGetVariable (TempVariableName, VendorGuid, NULL, &VariableSize, NULL);
+      if (Status == EFI_BUFFER_TOO_SMALL) {
+        DEBUG ((DEBUG_INFO, "Locking %s, Guid = %g\n", TempVariableName, VendorGuid));
+        Status = VarLibVariableRequestToLock (TempVariableName, VendorGuid);
+        ASSERT_EFI_ERROR (Status);
+      } else if (Status == EFI_NOT_FOUND) {
+        //
+        // No more variables need to lock.
+        //
+        return EFI_SUCCESS;
+      }
+    }   // End of for loop
+  } else if (Status == EFI_NOT_FOUND) {
+    //
+    // Check if it is single variable scenario.
+    //
+    VariableSize = 0;
+    Status = VarLibGetVariable (VariableName, VendorGuid, NULL, &VariableSize, NULL);
+    if (Status == EFI_BUFFER_TOO_SMALL) {
+      //
+      // Lock single variable.
+      //
+      DEBUG ((DEBUG_INFO, "Locking %s, Guid = %g\n", VariableName, VendorGuid));
+      Status = VarLibVariableRequestToLock (VariableName, VendorGuid);
+      ASSERT_EFI_ERROR (Status);
+      return EFI_SUCCESS;
+    }
+  }
+
+  //
+  // Here probably means variable not present.
+  //
+  return Status;
+
+}
diff --git a/Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.c b/Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.c
index 9ed59f8827..e7d0c5ec34 100644
--- a/Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.c
+++ b/Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.c
@@ -10,7 +10,7 @@
   Using this library allows code to be written in a generic manner that can be
   used in DXE or SMM without modification.
 
-  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2021 - 2022, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -18,14 +18,16 @@
 #include <Uefi.h>
 
 #include <Guid/EventGroup.h>
-#include <Protocol/VariableLock.h>
+#include <Library/VariablePolicyHelperLib.h>
 
 #include <Library/UefiLib.h>
 #include <Library/DebugLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiRuntimeServicesTableLib.h>
 
-STATIC EDKII_VARIABLE_LOCK_PROTOCOL  *mVariableWriteLibVariableLock = NULL;
+STATIC EDKII_VARIABLE_POLICY_PROTOCOL  *mVariableWriteLibVariablePolicy = NULL;
+EFI_EVENT                              mExitBootServiceEvent;
+EFI_EVENT                              mLegacyBootEvent;
 
 /**
   Sets the value of a variable.
@@ -144,7 +146,7 @@ VarLibIsVariableRequestToLockSupported (
   VOID
   )
 {
-  if (mVariableWriteLibVariableLock != NULL) {
+  if (mVariableWriteLibVariablePolicy != NULL) {
     return TRUE;
   } else {
     return FALSE;
@@ -178,16 +180,46 @@ VarLibVariableRequestToLock (
 {
   EFI_STATUS    Status = EFI_UNSUPPORTED;
 
-  if (mVariableWriteLibVariableLock != NULL) {
-    Status = mVariableWriteLibVariableLock->RequestToLock (
-                                              mVariableWriteLibVariableLock,
-                                              VariableName,
-                                              VendorGuid
-                                              );
+  if (mVariableWriteLibVariablePolicy != NULL) {
+    Status = RegisterBasicVariablePolicy (
+               mVariableWriteLibVariablePolicy,
+               (CONST EFI_GUID*) VendorGuid,
+               (CONST CHAR16 *) VariableName,
+               VARIABLE_POLICY_NO_MIN_SIZE,
+               VARIABLE_POLICY_NO_MAX_SIZE,
+               VARIABLE_POLICY_NO_MUST_ATTR,
+               VARIABLE_POLICY_NO_CANT_ATTR,
+               VARIABLE_POLICY_TYPE_LOCK_NOW
+               );
+    ASSERT_EFI_ERROR (Status);
   }
   return Status;
 }
 
+/**
+  Close events when driver unloaded.
+
+  @param[in] ImageHandle  A handle for the image that is initializing this driver
+  @param[in] SystemTable  A pointer to the EFI system table
+
+  @retval    EFI_SUCCESS  The initialization finished successfully.
+**/
+EFI_STATUS
+EFIAPI
+DxeRuntimeVariableWriteLibDestructor (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  if (mExitBootServiceEvent != 0) {
+    gBS->CloseEvent (mExitBootServiceEvent);
+  }
+  if (mLegacyBootEvent != 0) {
+    gBS->CloseEvent (mLegacyBootEvent);
+  }
+  return EFI_SUCCESS;
+}
+
 /**
   Exit Boot Services Event notification handler.
 
@@ -202,7 +234,7 @@ DxeRuntimeVariableWriteLibOnExitBootServices (
   IN  VOID                         *Context
   )
 {
-  mVariableWriteLibVariableLock = NULL;
+  mVariableWriteLibVariablePolicy = NULL;
 }
 
 /**
@@ -227,13 +259,11 @@ DxeRuntimeVariableWriteLibConstructor (
   )
 {
   EFI_STATUS    Status;
-  EFI_EVENT     ExitBootServiceEvent;
-  EFI_EVENT     LegacyBootEvent;
 
   //
   // Locate VariableLockProtocol.
   //
-  Status = gBS->LocateProtocol (&gEdkiiVariableLockProtocolGuid, NULL, (VOID **)&mVariableWriteLibVariableLock);
+  Status = gBS->LocateProtocol (&gEdkiiVariablePolicyProtocolGuid, NULL, (VOID **)&mVariableWriteLibVariablePolicy);
   ASSERT_EFI_ERROR (Status);
 
   //
@@ -245,7 +275,7 @@ DxeRuntimeVariableWriteLibConstructor (
              DxeRuntimeVariableWriteLibOnExitBootServices,
              NULL,
              &gEfiEventExitBootServicesGuid,
-             &ExitBootServiceEvent
+             &mExitBootServiceEvent
              );
   ASSERT_EFI_ERROR (Status);
 
@@ -257,7 +287,7 @@ DxeRuntimeVariableWriteLibConstructor (
              TPL_NOTIFY,
              DxeRuntimeVariableWriteLibOnExitBootServices,
              NULL,
-             &LegacyBootEvent
+             &mLegacyBootEvent
              );
   ASSERT_EFI_ERROR (Status);
 
diff --git a/Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib.h b/Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib.h
index c847d7f152..83b5e78506 100644
--- a/Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib.h
+++ b/Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib.h
@@ -16,7 +16,7 @@
   is possible, adjusting the value of PcdMaxVariableSize may provide a simpler
   solution to this problem.
 
-  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2021 - 2022, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -66,4 +66,25 @@ SetLargeVariable (
   IN  VOID                         *Data
   );
 
+/**
+  Locks the existing large variable.
+
+  @param[in]  VariableName       A Null-terminated string that is the name of the vendor's variable.
+                                 Each VariableName is unique for each VendorGuid. VariableName must
+                                 contain 1 or more characters. If VariableName is an empty string,
+                                 then EFI_INVALID_PARAMETER is returned.
+  @param[in]  VendorGuid         A unique identifier for the vendor.
+  @retval EFI_SUCCESS            The firmware has successfully locked the variable.
+  @retval EFI_INVALID_PARAMETER  An invalid combination of variable name and GUID was supplied
+  @retval EFI_UNSUPPORTED        The service for locking variable is not ready.
+  @retval EFI_NOT_FOUND          The targeting variable for locking is not present.
+
+**/
+EFI_STATUS
+EFIAPI
+LockLargeVariable (
+  IN  CHAR16                       *VariableName,
+  IN  EFI_GUID                     *VendorGuid
+  );
+
 #endif  // _LARGE_VARIABLE_WRITE_LIB_H_
diff --git a/Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.inf b/Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.inf
index 704a8ac7cc..f83090c847 100644
--- a/Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.inf
+++ b/Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.inf
@@ -10,7 +10,7 @@
 # Using this library allows code to be written in a generic manner that can be
 # used in DXE or SMM without modification.
 #
-# Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2021 - 2022, Intel Corporation. All rights reserved.<BR>
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -24,6 +24,7 @@
   MODULE_TYPE                    = DXE_RUNTIME_DRIVER
   LIBRARY_CLASS                  = VariableWriteLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_APPLICATION UEFI_DRIVER
   CONSTRUCTOR                    = DxeRuntimeVariableWriteLibConstructor
+  DESTRUCTOR                     = DxeRuntimeVariableWriteLibDestructor
 
 [Packages]
   MdePkg/MdePkg.dec
@@ -37,13 +38,14 @@
   UefiLib
   UefiBootServicesTableLib
   UefiRuntimeServicesTableLib
+  VariablePolicyHelperLib
 
 [Guids]
   gEfiEventExitBootServicesGuid       ## CONSUMES ## Event
 
 [Protocols]
   gEfiVariableWriteArchProtocolGuid   ## CONSUMES
-  gEdkiiVariableLockProtocolGuid      ## CONSUMES
+  gEdkiiVariablePolicyProtocolGuid      ## CONSUMES
 
 [Depex]
-  gEfiVariableWriteArchProtocolGuid AND gEdkiiVariableLockProtocolGuid
+  gEfiVariableWriteArchProtocolGuid AND gEdkiiVariablePolicyProtocolGuid
-- 
2.28.0.windows.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-02-10 18:01 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-10  6:59 [edk2-platforms: PATCH v2] MinPlatformPkg/SaveMemoryConfig: Variable may not be locked Chiu, Chasel
2022-02-10 18:01 ` [edk2-devel] " Oram, Isaac W

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox