MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c does not initialize AccessResults in HiiConfigRoutingExportConfig()

MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c does not initialize AccessResults in HiiConfigRoutingExportConfig()

[Charles Hyde]

[-- Attachment #1.1: Type: text/plain, Size: 1187 bytes --]

In the function HiiConfigRoutingExportConfig() in MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c, the string pointer AccessResults is not initialized like the other pointers are. This variable should be set to NULL near line 5295, along with the other pointers.

In testing my company’s BIOS with uefi-sct, I found strange behavior that I traced to this variable not being initialized before calling ConfigAccess->ExtractConfig() at line 5322. If you check lines 4875 through 4888, you will find this other function does initialize its pointer variables.

Also, AccessResults is not checked for NULL before it is used at line 5357. This could result in StrStr() and GetElementsFromRequest() both being passed a NULL pointer and this section of code would be invalid.

This attached patch solves both problems, it initializes the pointer to NULL, and it checks for NULL after calling ConfigAccess->ExtractConfig(). As a result of implementing my patch, my company's BIOS passed the uefi-sct without issue. I also checked a much older BIOS that we have, and the problem exists there as well. It appears this uninitialized pointer has been around for no less than 10 years.

[-- Attachment #1.2: Type: text/html, Size: 1221 bytes --]

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 2633 bytes --]

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
index 5ae6189a28..700e80619b 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
@@ -420,14 +420,19 @@ AppendToMultiString (
   }
 
   AppendStringSize = StrSize (AppendString);
+  if (AppendStringSize <= sizeof(CHAR16))
+    return EFI_SUCCESS;
+
   MultiStringSize  = StrSize (*MultiString);
   MaxLen           = MAX_STRING_LENGTH / sizeof (CHAR16);
 
   //
   // Enlarge the buffer each time when length exceeds MAX_STRING_LENGTH.
   //
-  if ((MultiStringSize + AppendStringSize > MAX_STRING_LENGTH) ||
-      (MultiStringSize > MAX_STRING_LENGTH))
+  if ((MultiStringSize + AppendStringSize > MAX_STRING_LENGTH) /*||
+      (MultiStringSize > MAX_STRING_LENGTH)*/)  // There is no need to check the second part.
+                                                // If the first part is false, the second part will always be false.
+                                                // If the second part is true, the first part must also be true.
   {
     *MultiString = (EFI_STRING)ReallocatePool (
                                  MultiStringSize,
@@ -5292,6 +5297,7 @@ HiiConfigRoutingExportConfig (
     //
     IfrDataParsedFlag = FALSE;
     Progress          = NULL;
+    AccessResults     = NULL;
     HiiHandle         = NULL;
     DefaultResults    = NULL;
     Database          = NULL;
@@ -5326,6 +5332,17 @@ HiiConfigRoutingExportConfig (
                              &AccessResults
                              );
     if (EFI_ERROR (Status)) {
+
+      // If an error was returned, then do not believe any results in these two pointers.
+      if (Progress) {
+        FreePool (Progress);
+        Progress = NULL;
+      }
+      if (AccessResults) {
+        FreePool (AccessResults);
+        AccessResults = NULL;
+      }
+
       //
       // Update AccessResults by getting default setting from IFR when HiiPackage is registered to HiiHandle
       //
@@ -5350,6 +5367,18 @@ HiiConfigRoutingExportConfig (
     }
 
     if (!EFI_ERROR (Status)) {
+
+      // If AccessResults == NULL, there is nothing to be done.
+      if (AccessResults == NULL) {
+        if (Progress)
+          FreePool (Progress);
+
+        if (ConfigRequest != NULL)
+          FreePool (ConfigRequest);
+
+        continue;
+      }
+
       //
       // Update AccessResults by getting default setting from IFR when HiiPackage is registered to HiiHandle
       //