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".
> +
[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.
+//
+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 (
if (!DataIsIdentical) {
Status = SetLargeVariable (L"FspNvsBuffer",
&gFspNvsBufferVariableGuid, TRUE, DataSize, HobData);
+if (Status == EFI_ABORTED) {
+ //
+ // Fail to lock variable! This should not happen.
+ //
+ ASSERT_EFI_ERROR (Status);
+ //
+ // When building without ASSERT_EFI_ERROR hang, delete the varialbe
so it will not be consumed.
+ //
+ DEBUG ((DEBUG_ERROR, "Delete variable!\n"));
+ DataSize = 0;
+ Status = SetLargeVariable (L"FspNvsBuffer",
&gFspNvsBufferVariableGuid, FALSE, DataSize, HobData);
+}
ASSERT_EFI_ERROR (Status);
DEBUG ((DEBUG_INFO, "Saved size of FSP / MRC Training Data: 0x%x\n",
DataSize));
} else {
@@ -106,7 +137,7 @@ SaveMemoryConfigEntryPoint
