* [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks
@ 2023-07-06 1:57 Michael Kubacki
2023-07-06 2:57 ` [edk2-devel] " Isaac Oram
[not found] ` <176F27E11435C093.24383@groups.io>
0 siblings, 2 replies; 3+ messages in thread
From: Michael Kubacki @ 2023-07-06 1:57 UTC (permalink / raw)
To: devel
Cc: Chasel Chiu, Nate DeSimone, Isaac Oram, Liming Gao, Eric Dong,
Ken Lautner
From: Michael Kubacki <michael.kubacki@microsoft.com>
Adds some sanity checks around the Memory Type Information data
restored from the `EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME` UEFI
variable.
This is particularly useful when the structures that the data was
saved against have changed in the latest firmware image. For example,
`EfiUnacceptedMemoryType` was added to `EFI_MEMORY_TYPE` in
edk2 commit `502c01c`. This incremented `EfiMaxMemoryType` by `1`.
That change was first released in the `edk2-stable202211` stable tag.
Firmware performing an update across those stable tags may encounter
issues depending on code implementation for handling
`EfiMaxMemoryType` as a terminating loop value. This change checks
the size and max memory type saved in the UEFI variable to determine
whether it is better to start from the defaults and rebuild the
UEFI variable data on the current boot.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Co-authored-by: Ken Lautner <klautner@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c | 32 +++++++++++++++++---
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c b/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
index d8c96b52f4b3..bc97711a02f6 100644
--- a/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
+++ b/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
@@ -164,18 +164,22 @@ BuildMemoryTypeInformation (
EFI_STATUS Status;
EFI_PEI_READ_ONLY_VARIABLE2_PPI *VariableServices;
UINTN DataSize;
+ UINTN Index;
EFI_MEMORY_TYPE_INFORMATION MemoryData[EfiMaxMemoryType + 1];
//
// Locate system configuration variable
//
- Status = PeiServicesLocatePpi(
+ Status = PeiServicesLocatePpi (
&gEfiPeiReadOnlyVariable2PpiGuid, // GUID
0, // INSTANCE
NULL, // EFI_PEI_PPI_DESCRIPTOR
(VOID **) &VariableServices // PPI
);
- ASSERT_EFI_ERROR(Status);
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ return;
+ }
DataSize = sizeof (MemoryData);
Status = VariableServices->GetVariable (
@@ -186,9 +190,29 @@ BuildMemoryTypeInformation (
&DataSize,
&MemoryData
);
- if (EFI_ERROR(Status)) {
+ if (!EFI_ERROR (Status)) {
+ if (DataSize % sizeof (EFI_MEMORY_TYPE_INFORMATION) != 0) {
+ DEBUG ((DEBUG_ERROR, "The UEFI Memory Type Information variable size is inconsistent with this build.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ } else {
+ // Loop through all except the last one and make sure it seems reasonable
+ for (Index = 0; Index < ((DataSize / sizeof (EFI_MEMORY_TYPE_INFORMATION)) - 1); Index++) {
+ if (MemoryData[Index].Type >= EfiMaxMemoryType) {
+ DEBUG ((DEBUG_ERROR, "UEFI Memory Type Information variable has an invalid memory type.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ }
+ }
+ // The last entry must be MaxMemoryType with size 0
+ if ((MemoryData[Index].Type != EfiMaxMemoryType) || (MemoryData[Index].NumberOfPages != 0)) {
+ DEBUG ((DEBUG_ERROR, "UEFI Memory Type Information variable contains an invalid last entry.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ }
+ }
+ }
+
+ if (EFI_ERROR (Status)) {
DataSize = sizeof (mDefaultMemoryTypeInformation);
- CopyMem(MemoryData, mDefaultMemoryTypeInformation, DataSize);
+ CopyMem (MemoryData, mDefaultMemoryTypeInformation, DataSize);
}
///
--
2.41.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks
2023-07-06 1:57 [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks Michael Kubacki
@ 2023-07-06 2:57 ` Isaac Oram
[not found] ` <176F27E11435C093.24383@groups.io>
1 sibling, 0 replies; 3+ messages in thread
From: Isaac Oram @ 2023-07-06 2:57 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com
Cc: Chiu, Chasel, Desimone, Nathaniel L, Gao, Liming, Dong, Eric,
Lautner, Kenneth
Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Wednesday, July 5, 2023 6:57 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Lautner, Kenneth <klautner@microsoft.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks
From: Michael Kubacki <michael.kubacki@microsoft.com>
Adds some sanity checks around the Memory Type Information data restored from the `EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME` UEFI variable.
This is particularly useful when the structures that the data was saved against have changed in the latest firmware image. For example, `EfiUnacceptedMemoryType` was added to `EFI_MEMORY_TYPE` in
edk2 commit `502c01c`. This incremented `EfiMaxMemoryType` by `1`.
That change was first released in the `edk2-stable202211` stable tag.
Firmware performing an update across those stable tags may encounter issues depending on code implementation for handling `EfiMaxMemoryType` as a terminating loop value. This change checks the size and max memory type saved in the UEFI variable to determine whether it is better to start from the defaults and rebuild the UEFI variable data on the current boot.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Co-authored-by: Ken Lautner <klautner@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c | 32 +++++++++++++++++---
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c b/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
index d8c96b52f4b3..bc97711a02f6 100644
--- a/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
+++ b/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/Platfor
+++ mInitPreMem.c
@@ -164,18 +164,22 @@ BuildMemoryTypeInformation (
EFI_STATUS Status;
EFI_PEI_READ_ONLY_VARIABLE2_PPI *VariableServices;
UINTN DataSize;
+ UINTN Index;
EFI_MEMORY_TYPE_INFORMATION MemoryData[EfiMaxMemoryType + 1];
//
// Locate system configuration variable
//
- Status = PeiServicesLocatePpi(
+ Status = PeiServicesLocatePpi (
&gEfiPeiReadOnlyVariable2PpiGuid, // GUID
0, // INSTANCE
NULL, // EFI_PEI_PPI_DESCRIPTOR
(VOID **) &VariableServices // PPI
);
- ASSERT_EFI_ERROR(Status);
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ return;
+ }
DataSize = sizeof (MemoryData);
Status = VariableServices->GetVariable ( @@ -186,9 +190,29 @@ BuildMemoryTypeInformation (
&DataSize,
&MemoryData
);
- if (EFI_ERROR(Status)) {
+ if (!EFI_ERROR (Status)) {
+ if (DataSize % sizeof (EFI_MEMORY_TYPE_INFORMATION) != 0) {
+ DEBUG ((DEBUG_ERROR, "The UEFI Memory Type Information variable size is inconsistent with this build.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ } else {
+ // Loop through all except the last one and make sure it seems reasonable
+ for (Index = 0; Index < ((DataSize / sizeof (EFI_MEMORY_TYPE_INFORMATION)) - 1); Index++) {
+ if (MemoryData[Index].Type >= EfiMaxMemoryType) {
+ DEBUG ((DEBUG_ERROR, "UEFI Memory Type Information variable has an invalid memory type.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ }
+ }
+ // The last entry must be MaxMemoryType with size 0
+ if ((MemoryData[Index].Type != EfiMaxMemoryType) || (MemoryData[Index].NumberOfPages != 0)) {
+ DEBUG ((DEBUG_ERROR, "UEFI Memory Type Information variable contains an invalid last entry.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ }
+ }
+ }
+
+ if (EFI_ERROR (Status)) {
DataSize = sizeof (mDefaultMemoryTypeInformation);
- CopyMem(MemoryData, mDefaultMemoryTypeInformation, DataSize);
+ CopyMem (MemoryData, mDefaultMemoryTypeInformation, DataSize);
}
///
--
2.41.0.windows.1
-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106670): https://edk2.groups.io/g/devel/message/106670
Mute This Topic: https://groups.io/mt/99978201/1492418
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [isaac.w.oram@intel.com]
-=-=-=-=-=-=
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks
[not found] ` <176F27E11435C093.24383@groups.io>
@ 2023-07-07 1:14 ` Isaac Oram
0 siblings, 0 replies; 3+ messages in thread
From: Isaac Oram @ 2023-07-07 1:14 UTC (permalink / raw)
To: devel@edk2.groups.io, Oram, Isaac W, mikuback@linux.microsoft.com
Cc: Chiu, Chasel, Desimone, Nathaniel L, Gao, Liming, Dong, Eric,
Lautner, Kenneth
Pushed as 87c40ac89b97eccac690762536db5376af15bb65
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Isaac Oram
Sent: Wednesday, July 5, 2023 7:57 PM
To: devel@edk2.groups.io; mikuback@linux.microsoft.com
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>; Lautner, Kenneth <klautner@microsoft.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks
Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Wednesday, July 5, 2023 6:57 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Lautner, Kenneth <klautner@microsoft.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks
From: Michael Kubacki <michael.kubacki@microsoft.com>
Adds some sanity checks around the Memory Type Information data restored from the `EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME` UEFI variable.
This is particularly useful when the structures that the data was saved against have changed in the latest firmware image. For example, `EfiUnacceptedMemoryType` was added to `EFI_MEMORY_TYPE` in
edk2 commit `502c01c`. This incremented `EfiMaxMemoryType` by `1`.
That change was first released in the `edk2-stable202211` stable tag.
Firmware performing an update across those stable tags may encounter issues depending on code implementation for handling `EfiMaxMemoryType` as a terminating loop value. This change checks the size and max memory type saved in the UEFI variable to determine whether it is better to start from the defaults and rebuild the UEFI variable data on the current boot.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Co-authored-by: Ken Lautner <klautner@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c | 32 +++++++++++++++++---
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c b/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
index d8c96b52f4b3..bc97711a02f6 100644
--- a/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
+++ b/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/Platfor
+++ mInitPreMem.c
@@ -164,18 +164,22 @@ BuildMemoryTypeInformation (
EFI_STATUS Status;
EFI_PEI_READ_ONLY_VARIABLE2_PPI *VariableServices;
UINTN DataSize;
+ UINTN Index;
EFI_MEMORY_TYPE_INFORMATION MemoryData[EfiMaxMemoryType + 1];
//
// Locate system configuration variable
//
- Status = PeiServicesLocatePpi(
+ Status = PeiServicesLocatePpi (
&gEfiPeiReadOnlyVariable2PpiGuid, // GUID
0, // INSTANCE
NULL, // EFI_PEI_PPI_DESCRIPTOR
(VOID **) &VariableServices // PPI
);
- ASSERT_EFI_ERROR(Status);
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ return;
+ }
DataSize = sizeof (MemoryData);
Status = VariableServices->GetVariable ( @@ -186,9 +190,29 @@ BuildMemoryTypeInformation (
&DataSize,
&MemoryData
);
- if (EFI_ERROR(Status)) {
+ if (!EFI_ERROR (Status)) {
+ if (DataSize % sizeof (EFI_MEMORY_TYPE_INFORMATION) != 0) {
+ DEBUG ((DEBUG_ERROR, "The UEFI Memory Type Information variable size is inconsistent with this build.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ } else {
+ // Loop through all except the last one and make sure it seems reasonable
+ for (Index = 0; Index < ((DataSize / sizeof (EFI_MEMORY_TYPE_INFORMATION)) - 1); Index++) {
+ if (MemoryData[Index].Type >= EfiMaxMemoryType) {
+ DEBUG ((DEBUG_ERROR, "UEFI Memory Type Information variable has an invalid memory type.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ }
+ }
+ // The last entry must be MaxMemoryType with size 0
+ if ((MemoryData[Index].Type != EfiMaxMemoryType) || (MemoryData[Index].NumberOfPages != 0)) {
+ DEBUG ((DEBUG_ERROR, "UEFI Memory Type Information variable contains an invalid last entry.\n"));
+ Status = EFI_COMPROMISED_DATA;
+ }
+ }
+ }
+
+ if (EFI_ERROR (Status)) {
DataSize = sizeof (mDefaultMemoryTypeInformation);
- CopyMem(MemoryData, mDefaultMemoryTypeInformation, DataSize);
+ CopyMem (MemoryData, mDefaultMemoryTypeInformation, DataSize);
}
///
--
2.41.0.windows.1
-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106670): https://edk2.groups.io/g/devel/message/106670
Mute This Topic: https://groups.io/mt/99978201/1492418
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [isaac.w.oram@intel.com] -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-07 1:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-06 1:57 [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks Michael Kubacki
2023-07-06 2:57 ` [edk2-devel] " Isaac Oram
[not found] ` <176F27E11435C093.24383@groups.io>
2023-07-07 1:14 ` Isaac Oram
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox