Re: [edk2-platforms: PATCH] MinPlatformPkg/SaveMemoryConfig: Variable may not be locked.

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Chasel,

Please see feedback inline below.

Thanks,
Nate

> -----Original Message-----
> From: Chiu, Chasel <chasel.chiu@intel.com>
> Sent: Tuesday, February 8, 2022 8:20 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2-platforms: PATCH] MinPlatformPkg/SaveMemoryConfig:
> Variable may not be locked.
> 
> 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, also switch to VariablePolicyProtocol for
> locking variables.
> 
> This patch also updated 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>
> ---
>  Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c                    | 12 +++++++++---
>  Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.c   | 62 ++++++++++++++++++++++++++++++++++++++++++++++----------------
>  Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.inf |  8 +++++---
>  3 files changed, 60 insertions(+), 22 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
> index 820585f676..0ae5511fed 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 = VarLibVariableRequestToLock (L"FspNvsBuffer",  &gFspNvsBufferVariableGuid);
> +              ASSERT_EFI_ERROR (Status);

Great catch on finding this bug! Unfortunately, this implementation introduces a different bug 😊.

If the data being written is greater than the maximum size of a UEFI variable, then LargeVariableLib will split it across multiple variables ("FspNvsBuffer1", "FspNvsBuffer2", and so on). Accordingly, this code will only work if the size of the FSP NVS data is small enough to fit inside a single UEFI variable. To fix this bug, I believe you will need to add a new function to the LargeVariableLib that allows one to lock the variable without having to write to it.

>              }
>              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/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/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