Re: [edk2-devel] [edk2-platforms: PATCH v4] MinPlatformPkg/SaveMemoryConfig: Variable may not be locked.
Fix pushed: https://github.com/tianocore/edk2-platforms/commit/cbc8e420ac4505e9c51aa0d4f049026691024ca5 Thanks, Chasel > -Original Message- > From: devel@edk2.groups.io On Behalf Of Chiu, Chasel > Sent: Friday, February 18, 2022 7:56 AM > To: Desimone, Nathaniel L ; > devel@edk2.groups.io > Cc: Gao, Liming ; Dong, Eric > ; Oram, Isaac W > Subject: Re: [edk2-devel] [edk2-platforms: PATCH v4] > MinPlatformPkg/SaveMemoryConfig: Variable may not be locked. > > > Thanks Nate, Isaac for good catch! I will correct them before pushing. > > > -Original Message- > > From: Desimone, Nathaniel L > > Sent: Friday, February 18, 2022 7:22 AM > > To: devel@edk2.groups.io; Chiu, Chasel > > Cc: Gao, Liming ; Dong, Eric > > ; Oram, Isaac W > > Subject: RE: [edk2-devel] [edk2-platforms: PATCH v4] > > MinPlatformPkg/SaveMemoryConfig: Variable may not be locked. > > > > Hi Chasel, > > > > There are a couple of typos and minor code style issues pointed out below. > > Please fix those during the push... no need to send another patch. > > > > Reviewed-by: Nate DeSimone > > > > Thanks, > > Nate > > > > > -Original Message- > > > From: devel@edk2.groups.io On Behalf Of Chiu, > > > Chasel > > > Sent: Wednesday, February 16, 2022 10:21 PM > > > To: devel@edk2.groups.io > > > Cc: Chiu, Chasel ; Desimone, Nathaniel L > > > ; Gao, Liming > > > ; Dong, Eric ; Oram, > > > Isaac W > > > Subject: [edk2-devel] [edk2-platforms: PATCH v4] > > > MinPlatformPkg/SaveMemoryConfig: Variable may not be locked. > > > > > > From: "Chiu, Chasel" > > > > > > 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. > > > > > > Failing to lock variable could be security vulnerability, so the > > > LargeVariableWriteLib functions will return EFI_ABORTED if locking > > > was failed and SaveMemoryConfig driver will delete variable to > > > prevent from using unlocked variable. > > > > > > 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 > > > Cc: Liming Gao > > > Cc: Eric Dong > > > Signed-off-by: Chasel Chiu > > > Reviewed-by: Isaac Oram ---Patch V4: > > > 1.Removed CpuDeadLoop and delete variable if Lock failed in > > SaveMemoryConfig. 2. LargeVariableWrite will not delete variable if > > Lock failed, let caller to do it. > > > > > > Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryC > > onfig.c| 37 ++--- > > > > > > Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWri > > teLib.c | 142 > > > + > > > + > > > > > > > > Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRunti > > meVariableWriteLib.c | 61 > > + > > > > > > Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryC > > onfig.inf | 3 ++- > > > > > > Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib. > > > h > > | 26 -- > > > > > > Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRunti > > meVariableWriteLib.inf | 8 +--- > > > 6 files changed, 248 insertions(+), 29 deletions(-) > > > > > > diff --git > > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor > > y > > > Config.c > > > > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor > > y > > > Config.c > > > index 820585f676..fb51edd5cb 100644 > > > --- > > > > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor > > y > > > Co
Re: [edk2-devel] [edk2-platforms: PATCH v4] MinPlatformPkg/SaveMemoryConfig: Variable may not be locked.
Thanks Nate, Isaac for good catch! I will correct them before pushing. > -Original Message- > From: Desimone, Nathaniel L > Sent: Friday, February 18, 2022 7:22 AM > To: devel@edk2.groups.io; Chiu, Chasel > Cc: Gao, Liming ; Dong, Eric > ; Oram, Isaac W > Subject: RE: [edk2-devel] [edk2-platforms: PATCH v4] > MinPlatformPkg/SaveMemoryConfig: Variable may not be locked. > > Hi Chasel, > > There are a couple of typos and minor code style issues pointed out below. > Please fix those during the push... no need to send another patch. > > Reviewed-by: Nate DeSimone > > Thanks, > Nate > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of Chiu, > > Chasel > > Sent: Wednesday, February 16, 2022 10:21 PM > > To: devel@edk2.groups.io > > Cc: Chiu, Chasel ; Desimone, Nathaniel L > > ; Gao, Liming > > ; Dong, Eric ; Oram, > > Isaac W > > Subject: [edk2-devel] [edk2-platforms: PATCH v4] > > MinPlatformPkg/SaveMemoryConfig: Variable may not be locked. > > > > From: "Chiu, Chasel" > > > > 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. > > > > Failing to lock variable could be security vulnerability, so the > > LargeVariableWriteLib functions will return EFI_ABORTED if locking was > > failed and SaveMemoryConfig driver will delete variable to prevent > > from using unlocked variable. > > > > 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 > > Cc: Liming Gao > > Cc: Eric Dong > > Signed-off-by: Chasel Chiu > > Reviewed-by: Isaac Oram ---Patch V4: > > 1.Removed CpuDeadLoop and delete variable if Lock failed in > SaveMemoryConfig. 2. LargeVariableWrite will not delete variable if Lock > failed, > let caller to do it. > > > Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryC > onfig.c| 37 ++--- > > > Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWri > teLib.c | 142 > + > + > > > > Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRunti > meVariableWriteLib.c | 61 > + > > > Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryC > onfig.inf | 3 ++- > > Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib.h > | 26 -- > > > Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRunti > meVariableWriteLib.inf | 8 +--- > > 6 files changed, 248 insertions(+), 29 deletions(-) > > > > diff --git > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor > y > > Config.c > > > b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor > y > > Config.c > > index 820585f676..fb51edd5cb 100644 > > --- > > > a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor > y > > Config.c > > +++ > b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMe > > +++ moryConfig.c > > @@ -2,13 +2,14 @@ > >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. > > +Copyright (c) 2017 - 2022, Intel Corporation. All rights > > +reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -18,6 +19,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include #include > > #include > > > > +#include > > #include > > > > /** > > @@ -86,6 +88,23 @@ SaveMemoryConfigEntryPoint ( > > Status = GetLargeVariable (L"FspNvsBuffer", > &gFspNvsBuff
Re: [edk2-devel] [edk2-platforms: PATCH v4] MinPlatformPkg/SaveMemoryConfig: Variable may not be locked.
Hi Chasel,
There are a couple of typos and minor code style issues pointed out below.
Please fix those during the push... no need to send another patch.
Reviewed-by: Nate DeSimone
Thanks,
Nate
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Chiu,
> Chasel
> Sent: Wednesday, February 16, 2022 10:21 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Desimone, Nathaniel L
> ; Gao, Liming
> ; Dong, Eric ; Oram,
> Isaac W
> Subject: [edk2-devel] [edk2-platforms: PATCH v4]
> MinPlatformPkg/SaveMemoryConfig: Variable may not be locked.
>
> From: "Chiu, Chasel"
>
> 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.
>
> Failing to lock variable could be security vulnerability, so the
> LargeVariableWriteLib functions will return EFI_ABORTED if locking
> was failed and SaveMemoryConfig driver will delete variable to
> prevent from using unlocked variable.
>
> 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
> Cc: Liming Gao
> Cc: Eric Dong
> Signed-off-by: Chasel Chiu
> Reviewed-by: Isaac Oram
> ---Patch V4: 1.Removed CpuDeadLoop and delete variable if Lock failed in
> SaveMemoryConfig. 2. LargeVariableWrite will not delete variable if Lock
> failed, let caller to do it.
> Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
>| 37 ++---
>
> Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
> | 142
> ++
>
> Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.c
>| 61 +
>
> Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
> | 3 ++-
> Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableWriteLib.h
>| 26 --
>
> Platform/Intel/MinPlatformPkg/Library/DxeRuntimeVariableWriteLib/DxeRuntimeVariableWriteLib.inf
> | 8 +---
> 6 files changed, 248 insertions(+), 29 deletions(-)
>
> diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
>
> b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
> index 820585f676..fb51edd5cb 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
> @@ -2,13 +2,14 @@
>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.
> +Copyright (c) 2017 - 2022, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -18,6 +19,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include
> #include
> #include
> +#include
> #include
>
> /**
> @@ -86,6 +88,23 @@ 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);
> + if (EFI_ERROR (Status)) {
> +//
> +// Fail to lock variable is security vulnerability and
> should not happen.
> +//
> +ASSERT_EFI_ERROR (Status);
> +//
> +// When building without ASSERT_EFI_ERROR hang, delete the
> varialbe so it will not be consumed.
Typo here, "varialbe" should be "variable".
> +//
> +DEBUG ((DEBUG_ERROR, "Delete variable!\n"));
> +DataSize = 0;
> +Status = SetLargeVariable (L"FspNvsBuffer",
> &gFspNvsBufferVariableGuid, FALSE, DataSize, HobData);
> +ASSERT_EFI_ERROR (Status);
> + }
> }
> FreePool (VariableData);
>}
> @@ -95,6 +114,18 @@ SaveMemoryConfigEntryPoint (
