[edk2] [PATCH 0/5] Resolve issues brought by r17732-r17739

[Hao Wu]

Commit r17732 to r17739 used safe string functions to replace the old ones
in IntelFrameworkModulePkg. However, these changes broght the following
two issues:

1. AllocateCopyPool (AllocationSize, Buffer)

Some usage of AllocateCopyPool() will read contents out of the scope
of 'Buffer'. Potential risk when 'Buffer' is allocated at the boundary of
memory region.

2. Some replacement of StrnCpy/StrnCat with StrCpyS/StrCatS functions

These changes will cause ASSERT when buffer overflow occurs, the
patches will use StrnCpyS/StrnCatS instead to resolve this issue.

Hao Wu (5):
IntelFrameworkModulePkg GenericBdsLib: Resolve issue brought by r17733
IntelFrameworkModulePkg BdsDxe: Resolve issue brought by r17735
IntelFrameworkModulePkg BootMaint: Resolve issue brought by r17736
IntelFrameworkModulePkg BootMngr: Resolve issue brought by r17737
IntelFrameworkModulePkg DeviceMngr: Resolve issue brought by r17738

.../Library/GenericBdsLib/BdsMisc.c | 20 ++++++++++++--------
.../Universal/BdsDxe/BootMaint/BootOption.c | 3 ++-
.../Universal/BdsDxe/BootMngr/BootManager.c | 3 ++-
.../Universal/BdsDxe/DeviceMngr/DeviceManager.c | 3 ++-
.../Universal/BdsDxe/MemoryTest.c | 14 ++++++++++++--
5 files changed, 30 insertions(+), 13 deletions(-)

[Hao Wu]

HelpString = AllocateCopyPool (HelpSize, L"Device Path : ");

The above using of AllocateCopyPool() will read contents out of the scope
of the constant string. Potential risk for the constant string allocated
at the boundary of memory region.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <***@intel.com>
Reviewed-by: Qiu Shumin <***@intel.com>
Reviewed-by: Jeff Fan <***@intel.com>
---
IntelFrameworkModulePkg/Universal/BdsDxe/BootMngr/BootManager.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BootMngr/BootManager.c b/IntelFrameworkModulePkg/Universal/BdsDxe/BootMngr/BootManager.c
index 978959d..6efd783 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BootMngr/BootManager.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BootMngr/BootManager.c
@@ -319,8 +319,9 @@ CallBootManager (

TempStr = DevicePathToStr (Option->DevicePath);
HelpSize = StrSize (TempStr) + StrSize (L"Device Path : ");
- HelpString = AllocateCopyPool (HelpSize, L"Device Path : ");
+ HelpString = AllocateZeroPool (HelpSize);
ASSERT (HelpString != NULL);
+ StrCatS (HelpString, HelpSize / sizeof (CHAR16), L"Device Path : ");
StrCatS (HelpString, HelpSize / sizeof (CHAR16), TempStr);

HelpToken = HiiSetString (HiiHandle, 0, HelpString, NULL);

[Hao Wu]

String = AllocateCopyPool (BufferLen, L"MAC:");

The above using of AllocateCopyPool() will read contents out of the scope
of the constant string. Potential risk for the constant string allocated
at the boundary of memory region.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <***@intel.com>
Reviewed-by: Qiu Shumin <***@intel.com>
Reviewed-by: Jeff Fan <***@intel.com>
---
IntelFrameworkModulePkg/Universal/BdsDxe/DeviceMngr/DeviceManager.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/DeviceMngr/DeviceManager.c b/IntelFrameworkModulePkg/Universal/BdsDxe/DeviceMngr/DeviceManager.c
index 5da0d47..af2b18a 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/DeviceMngr/DeviceManager.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/DeviceMngr/DeviceManager.c
@@ -374,12 +374,13 @@ GetMacAddressString(
// The size is the Number size + ":" size + Vlan size(\XXXX) + End
//
BufferLen = (4 + 2 * HwAddressSize + (HwAddressSize - 1) + 5 + 1) * sizeof (CHAR16);
- String = AllocateCopyPool (BufferLen, L"MAC:");
+ String = AllocateZeroPool (BufferLen);
if (String == NULL) {
return FALSE;
}

*PBuffer = String;
+ StrCpyS (String, BufferLen / sizeof (CHAR16), L"MAC:");
String += 4;

//

[Hao Wu]

StrCatS (StrPercent, sizeof (StrPercent) / sizeof (CHAR16), TmpStr);

The above using of StrCatS will cause ASSERT if TmpStr is longer than
StrPercent. Therefore, StrnCatS is used here to resolve the issue.

Similar scenario is for:
StrCatS (StrTotalMemory, StrTotalMemorySize / sizeof (CHAR16), TmpStr);

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <***@intel.com>
Reviewed-by: Qiu Shumin <***@intel.com>
Reviewed-by: Jeff Fan <***@intel.com>
---
IntelFrameworkModulePkg/Universal/BdsDxe/MemoryTest.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/MemoryTest.c b/IntelFrameworkModulePkg/Universal/BdsDxe/MemoryTest.c
index eef840b..fedb151 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/MemoryTest.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/MemoryTest.c
@@ -324,7 +324,12 @@ BdsMemoryTest (
//
// TmpStr size is 64, StrPercent is reserved to 16.
//
- StrCatS (StrPercent, sizeof (StrPercent) / sizeof (CHAR16), TmpStr);
+ StrnCatS (
+ StrPercent,
+ sizeof (StrPercent) / sizeof (CHAR16),
+ TmpStr,
+ sizeof (StrPercent) / sizeof (CHAR16) - StrLen (StrPercent) - 1
+ );
PrintXY (10, 10, NULL, NULL, StrPercent);
FreePool (TmpStr);
}
@@ -389,7 +394,12 @@ Done:

TmpStr = GetStringById (STRING_TOKEN (STR_MEM_TEST_COMPLETED));
if (TmpStr != NULL) {
- StrCatS (StrTotalMemory, StrTotalMemorySize / sizeof (CHAR16), TmpStr);
+ StrnCatS (
+ StrTotalMemory,
+ StrTotalMemorySize / sizeof (CHAR16),
+ TmpStr,
+ StrTotalMemorySize / sizeof (CHAR16) - StrLen (StrTotalMemory) - 1
+ );
FreePool (TmpStr);
}

[Hao Wu]

StringBuffer1 = AllocateCopyPool (
MAX_STRING_LEN * sizeof (CHAR16),
L"Configuration changed. Reset to apply it Now."
);

The above using of AllocateCopyPool() will read contents out of the scope
of the constant string. Potential risk for the constant string allocated
at the boundary of memory region.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <***@intel.com>
Reviewed-by: Qiu Shumin <***@intel.com>
Reviewed-by: Jeff Fan <***@intel.com>
---
.../Library/GenericBdsLib/BdsMisc.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c
index b5be631..24c1998 100644
--- a/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c
+++ b/IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c
@@ -1127,16 +1127,20 @@ SetupResetReminder (
if (IsResetReminderFeatureEnable ()) {
if (IsResetRequired ()) {

- StringBuffer1 = AllocateCopyPool (
- MAX_STRING_LEN * sizeof (CHAR16),
- L"Configuration changed. Reset to apply it Now."
- );
+ StringBuffer1 = AllocateZeroPool (MAX_STRING_LEN * sizeof (CHAR16));
ASSERT (StringBuffer1 != NULL);
- StringBuffer2 = AllocateCopyPool (
- MAX_STRING_LEN * sizeof (CHAR16),
- L"Press ENTER to reset"
- );
+ StringBuffer2 = AllocateZeroPool (MAX_STRING_LEN * sizeof (CHAR16));
ASSERT (StringBuffer2 != NULL);
+ StrCpyS (
+ StringBuffer1,
+ MAX_STRING_LEN,
+ L"Configuration changed. Reset to apply it Now."
+ );
+ StrCpyS (
+ StringBuffer2,
+ MAX_STRING_LEN,
+ L"Press ENTER to reset"
+ );
//
// Popup a menu to notice user
//

[Hao Wu]

Str = AllocateCopyPool (MaxLen * sizeof (CHAR16), Str1);

The above using of AllocateCopyPool() will read contents out of the scope
of Str1. Potential risk for Str1 allocated at the boundary of memory
region.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <***@intel.com>
Reviewed-by: Qiu Shumin <***@intel.com>
Reviewed-by: Jeff Fan <***@intel.com>
---
IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootOption.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootOption.c b/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootOption.c
index 1519315..56bcfab 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootOption.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootOption.c
@@ -1096,12 +1096,13 @@ BOpt_AppendFileName (
Size1 = StrSize (Str1);
Size2 = StrSize (Str2);
MaxLen = (Size1 + Size2 + sizeof (CHAR16)) / sizeof (CHAR16);
- Str = AllocateCopyPool (MaxLen * sizeof (CHAR16), Str1);
+ Str = AllocateZeroPool (MaxLen * sizeof (CHAR16));
ASSERT (Str != NULL);

TmpStr = AllocateZeroPool (MaxLen * sizeof (CHAR16));
ASSERT (TmpStr != NULL);

+ StrCatS (Str, MaxLen, Str1);
if (!((*Str == '\\') && (*(Str + 1) == 0))) {
StrCatS (Str, MaxLen, L"\\");
}