[edk2-devel] [PATCH v1 1/1] MdeModulePkg/BootMaintenanceManagerUiLib: Check array index before access

[edk2-devel] [PATCH v1 1/1] MdeModulePkg/BootMaintenanceManagerUiLib: Check array index before access

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

Many arrays are defined with a length of MAX_MENU_NUMBER in
FormGuid.h. Two of those are BootOptionOrder and DriverOptionOrder.

In UpdatePage.c, a pointer is set to either of those arrays. The
array buffer is accessed using an index whose range is checked after
the pointer to the array is dereferenced. This change moves the check
before the dereference.

In another place in the file, the ConsoleCheck pointer is also set to
an array buffer with MAX_MENU_NUMBER elements. Only an ASSERT()
currently checks the range of the array index. This change
conditionalizes the pointer dereference itself on the range of Index.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
index ca81b7f35264..b1d1e2ee44f4 100644
--- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
+++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
@@ -527,9 +527,12 @@ UpdateConsolePage (
         ((NewTerminalContext->IsStdErr != 0) && (UpdatePageId == FORM_CON_ERR_ID))
         )
     {
-      CheckFlags         |= EFI_IFR_CHECKBOX_DEFAULT;
-      ConsoleCheck[Index] = TRUE;
-    } else {
+      CheckFlags |= EFI_IFR_CHECKBOX_DEFAULT;
+
+      if (Index < MAX_MENU_NUMBER) {
+        ConsoleCheck[Index] = TRUE;
+      }
+    } else if (Index < MAX_MENU_NUMBER) {
       ConsoleCheck[Index] = FALSE;
     }
 
@@ -622,7 +625,7 @@ UpdateOrderPage (
   ASSERT (OptionsOpCodeHandle != NULL);
 
   NewMenuEntry = NULL;
-  for (OptionIndex = 0; (OptionOrder[OptionIndex] != 0 && OptionIndex < MAX_MENU_NUMBER); OptionIndex++) {
+  for (OptionIndex = 0; (OptionIndex < MAX_MENU_NUMBER && OptionOrder[OptionIndex] != 0); OptionIndex++) {
     BootOptionFound = FALSE;
     for (Index = 0; Index < OptionMenu->MenuNumber; Index++) {
       NewMenuEntry = BOpt_GetMenuEntry (OptionMenu, Index);
-- 
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108436): https://edk2.groups.io/g/devel/message/108436
Mute This Topic: https://groups.io/mt/101229613/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/BootMaintenanceManagerUiLib: Check array index before access

[Dandan Bi]

Reviewed-by: Dandan Bi <dandan.bi@intel.com>



Thanks,
Dandan

-----Original Message-----
From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com> 
Sent: Friday, September 8, 2023 10:08 AM
To: devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: [PATCH v1 1/1] MdeModulePkg/BootMaintenanceManagerUiLib: Check array index before access

From: Michael Kubacki <michael.kubacki@microsoft.com>

Many arrays are defined with a length of MAX_MENU_NUMBER in FormGuid.h. Two of those are BootOptionOrder and DriverOptionOrder.

In UpdatePage.c, a pointer is set to either of those arrays. The array buffer is accessed using an index whose range is checked after the pointer to the array is dereferenced. This change moves the check before the dereference.

In another place in the file, the ConsoleCheck pointer is also set to an array buffer with MAX_MENU_NUMBER elements. Only an ASSERT() currently checks the range of the array index. This change conditionalizes the pointer dereference itself on the range of Index.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
 MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
index ca81b7f35264..b1d1e2ee44f4 100644
--- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
+++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
@@ -527,9 +527,12 @@ UpdateConsolePage (
         ((NewTerminalContext->IsStdErr != 0) && (UpdatePageId == FORM_CON_ERR_ID))
         )
     {
-      CheckFlags         |= EFI_IFR_CHECKBOX_DEFAULT;
-      ConsoleCheck[Index] = TRUE;
-    } else {
+      CheckFlags |= EFI_IFR_CHECKBOX_DEFAULT;
+
+      if (Index < MAX_MENU_NUMBER) {
+        ConsoleCheck[Index] = TRUE;
+      }
+    } else if (Index < MAX_MENU_NUMBER) {
       ConsoleCheck[Index] = FALSE;
     }
 
@@ -622,7 +625,7 @@ UpdateOrderPage (
   ASSERT (OptionsOpCodeHandle != NULL);
 
   NewMenuEntry = NULL;
-  for (OptionIndex = 0; (OptionOrder[OptionIndex] != 0 && OptionIndex < MAX_MENU_NUMBER); OptionIndex++) {
+  for (OptionIndex = 0; (OptionIndex < MAX_MENU_NUMBER && 
+ OptionOrder[OptionIndex] != 0); OptionIndex++) {
     BootOptionFound = FALSE;
     for (Index = 0; Index < OptionMenu->MenuNumber; Index++) {
       NewMenuEntry = BOpt_GetMenuEntry (OptionMenu, Index);
--
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108441): https://edk2.groups.io/g/devel/message/108441
Mute This Topic: https://groups.io/mt/101229613/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-