[edk2-devel] [PATCH v2 0/5] BZ 4219: MdeModulePkg/Core/Dxe: Fix issues

[edk2-devel] [PATCH v2 0/5] BZ 4219: MdeModulePkg/Core/Dxe: Fix issues

[Ranbir Singh]

v1 -> v2:
  - Rebased
  - Commit message update wrt Fix UNUSED_VALUE Coverity issues

Ranbir Singh (5):
  MdeModulePkg/Core/Dxe: Fix FORWARD_NULL Coverity issues
  MdeModulePkg/Core/Dxe: Fix MISSING_BREAK Coverity issue
  MdeModulePkg/Core/Dxe: Fix DEADCODE Coverity issue
  MdeModulePkg/Core/Dxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issues
  MdeModulePkg/Core/Dxe: Fix UNUSED_VALUE Coverity issues

 MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c |  2 ++
 MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 15 +++++++++
 MdeModulePkg/Core/Dxe/Event/Event.c           |  4 +--
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c               | 19 +++++++++++
 MdeModulePkg/Core/Dxe/Image/Image.c           |  3 ++
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c         | 35 +++++++++-----------
 6 files changed, 57 insertions(+), 21 deletions(-)

-- 
2.34.1



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

[edk2-devel] [PATCH v2 1/5] MdeModulePkg/Core/Dxe: Fix FORWARD_NULL Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The functions CoreConvertSpace and CoreAllocateSpace in

    MdeModulePkg/Core/Dxe/Gcd/Gcd.c has

    ASSERT (FALSE); at lines 755 and 1155 which gets hit when

Operation neither include GCD_MEMORY_SPACE_OPERATION nor include
GCD_IO_SPACE_OPERATION but this comes into play only in DEBUG mode.
In Release mode, the code continues to proceed in this undesirable
case with Map variable still set to NULL and hence dereferencing
"Map" will lead to CRASH.

It is safer to add a debug message in this scenario and return from
the function with EFI_INVALID_PARAMETER; The existing ASSERT may be
retained or may be deleted whatever is deemed more appropriate.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4219

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 792cd2e0af23..39fa2adf9366 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -752,7 +752,9 @@ CoreConvertSpace (
     CoreAcquireGcdIoLock ();
     Map = &mGcdIoSpaceMap;
   } else {
+    DEBUG ((DEBUG_GCD, "  Status = %r\n", EFI_INVALID_PARAMETER));
     ASSERT (FALSE);
+    return EFI_INVALID_PARAMETER;
   }
 
   //
@@ -1152,7 +1154,9 @@ CoreAllocateSpace (
     CoreAcquireGcdIoLock ();
     Map = &mGcdIoSpaceMap;
   } else {
+    DEBUG ((DEBUG_GCD, "  Status = %r\n", EFI_INVALID_PARAMETER));
     ASSERT (FALSE);
+    return EFI_INVALID_PARAMETER;
   }
 
   Found     = FALSE;
-- 
2.34.1



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

[edk2-devel] [PATCH v2 2/5] MdeModulePkg/Core/Dxe: Fix MISSING_BREAK Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function CoreIsSchedulable has switch-case code in which

       case EFI_DEP_BEFORE:
       case EFI_DEP_AFTER:

has a comment that

// For a well-formed Dependency Expression, the code should never get here.

It also has an ASSERT (FALSE); but this is applicable only in DEBUG
mode. Seemingly, for RELEASE mode, code should not be allowed to fall
through to case EFI_DEP_SOR: which is below the above two.

Hence, add return FALSE at the end.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4219

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c b/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
index acbf68b700fb..9799ec9ca097 100644
--- a/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
+++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
@@ -265,6 +265,8 @@ CoreIsSchedulable (
         //
         DEBUG ((DEBUG_DISPATCH, "  RESULT = FALSE (Unexpected BEFORE or AFTER opcode)\n"));
         ASSERT (FALSE);
+        return FALSE;
+
       case EFI_DEP_SOR:
         //
         // These opcodes can only appear once as the first opcode.  If it is found
-- 
2.34.1



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

[edk2-devel] [PATCH v2 3/5] MdeModulePkg/Core/Dxe: Fix DEADCODE Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

In the function PromoteGuardedFreePages(), the value of AvailablePages
cannot be ZERO at the condition check point

  if (AvailablePages != 0) {

as the code can come out of the while loop above only when AvailablePages
is non-ZERO.

Hence, remove the redundant condition check and the return FALSE;
DEADCODE statement at the end of the function.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4219

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 35 +++++++++-----------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 0c0ca61872b4..016791ee002b 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -1568,28 +1568,25 @@ PromoteGuardedFreePages (
     }
   }
 
-  if (AvailablePages != 0) {
-    DEBUG ((DEBUG_INFO, "Promoted pages: %lX (%lx)\r\n", Start, (UINT64)AvailablePages));
-    ClearGuardedMemoryBits (Start, AvailablePages);
+  DEBUG ((DEBUG_INFO, "Promoted pages: %lX (%lx)\r\n", Start, (UINT64)AvailablePages));
+  ClearGuardedMemoryBits (Start, AvailablePages);
 
-    if (gCpu != NULL) {
-      //
-      // Set flag to make sure allocating memory without GUARD for page table
-      // operation; otherwise infinite loops could be caused.
-      //
-      mOnGuarding = TRUE;
-      Status      = gCpu->SetMemoryAttributes (gCpu, Start, EFI_PAGES_TO_SIZE (AvailablePages), 0);
-      ASSERT_EFI_ERROR (Status);
-      mOnGuarding = FALSE;
-    }
-
-    mLastPromotedPage = Start;
-    *StartAddress     = Start;
-    *EndAddress       = Start + EFI_PAGES_TO_SIZE (AvailablePages) - 1;
-    return TRUE;
+  if (gCpu != NULL) {
+    //
+    // Set flag to make sure allocating memory without GUARD for page table
+    // operation; otherwise infinite loops could be caused.
+    //
+    mOnGuarding = TRUE;
+    Status      = gCpu->SetMemoryAttributes (gCpu, Start, EFI_PAGES_TO_SIZE (AvailablePages), 0);
+    ASSERT_EFI_ERROR (Status);
+    mOnGuarding = FALSE;
   }
 
-  return FALSE;
+  mLastPromotedPage = Start;
+  *StartAddress     = Start;
+  *EndAddress       = Start + EFI_PAGES_TO_SIZE (AvailablePages) - 1;
+
+  return TRUE;
 }
 
 /**
-- 
2.34.1



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

[edk2-devel] [PATCH v2 4/5] MdeModulePkg/Core/Dxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

"1 << Priority" / "1 << Event->NotifyTpl" are potentially overflowing
expressions with type "int" (32 bits, signed) evaluated using 32-bit
arithmetic, and then used in a context that expects an expression of
type "UINTN" (64 bits, unsigned).

To avoid overflow, cast "1" to type "UINTN" before << operation.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4219

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Core/Dxe/Event/Event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c b/MdeModulePkg/Core/Dxe/Event/Event.c
index dc82abb02130..6cf93f7f562d 100644
--- a/MdeModulePkg/Core/Dxe/Event/Event.c
+++ b/MdeModulePkg/Core/Dxe/Event/Event.c
@@ -191,7 +191,7 @@ CoreDispatchEventNotifies (
     CoreAcquireEventLock ();
   }
 
-  gEventPending &= ~(UINTN)(1 << Priority);
+  gEventPending &= ~(UINTN)((UINTN)1 << Priority);
   CoreReleaseEventLock ();
 }
 
@@ -225,7 +225,7 @@ CoreNotifyEvent (
   //
 
   InsertTailList (&gEventQueue[Event->NotifyTpl], &Event->NotifyLink);
-  gEventPending |= (UINTN)(1 << Event->NotifyTpl);
+  gEventPending |= (UINTN)((UINTN)1 << Event->NotifyTpl);
 }
 
 /**
-- 
2.34.1



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

[edk2-devel] [PATCH v2 5/5] MdeModulePkg/Core/Dxe: Fix UNUSED_VALUE Coverity issues

[Ranbir Singh]

The return value after calls to functions CoreProcessFvImageFile,
CoreStartImage,  CoreGetDepexSectionAndPreProccess,
CoreInternalAddMemorySpace, CoreAddIoSpace, CoreAllocateMemorySpace
and CoreCloseProtocol is stored in Status, but it is not made of any
use and later Status gets overridden.

An option assuming this no Status check is deliberate, would be to
remove the return value storage in Status. Otherwise, simply add
Status check and appropriate debug messages (the patch does this).

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4219

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 15 +++++++++++++++
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c               | 15 +++++++++++++++
 MdeModulePkg/Core/Dxe/Image/Image.c           |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
index cf9d55687766..f53a2513457a 100644
--- a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
@@ -506,6 +506,10 @@ CoreDispatcher (
         // Produce a firmware volume block protocol for FvImage so it gets dispatched from.
         //
         Status = CoreProcessFvImageFile (DriverEntry->Fv, DriverEntry->FvHandle, &DriverEntry->FileName);
+        if (EFI_ERROR (Status)) {
+          DEBUG ((DEBUG_DISPATCH, "Failed to produce a FVB protocol for Fv %p FvHandle %p and FileName %g.\n",
+            DriverEntry->Fv, DriverEntry->FvHandle, &DriverEntry->FileName));
+        }
       } else {
         REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
           EFI_PROGRESS_CODE,
@@ -516,6 +520,10 @@ CoreDispatcher (
         ASSERT (DriverEntry->ImageHandle != NULL);
 
         Status = CoreStartImage (DriverEntry->ImageHandle, NULL, NULL);
+        if (EFI_ERROR (Status)) {
+          DEBUG ((DEBUG_DISPATCH, "Failed to transfer control to a loaded image's entry point for ImageHandle %p.\n",
+            DriverEntry->ImageHandle));
+        }
 
         REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
           EFI_PROGRESS_CODE,
@@ -549,6 +557,13 @@ CoreDispatcher (
         // If Section Extraction Protocol did not let the Depex be read before retry the read
         //
         Status = CoreGetDepexSectionAndPreProccess (DriverEntry);
+        if (EFI_ERROR (Status)) {
+          if (Status == EFI_PROTOCOL_ERROR) {
+            DEBUG ((DEBUG_DISPATCH, "Section extraction protocol failure for DriverEntry %p.\n", DriverEntry));
+          } else {
+            DEBUG ((DEBUG_DISPATCH, "No Depex, assume UEFI 2.0 driver model for DriverEntry %p.\n", DriverEntry));
+          }
+        }
       }
 
       if (DriverEntry->Dependent) {
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 39fa2adf9366..384fee600d85 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -2638,6 +2638,9 @@ CoreInitializeGcdServices (
                    ResourceHob->ResourceLength,
                    Capabilities
                    );
+        if (EFI_ERROR (Status)) {
+          DEBUG ((DEBUG_GCD, "Failed to add a segment of memory to GCD map, Status = %r\n", Status));
+        }
       }
 
       if (GcdIoType != EfiGcdIoTypeNonExistent) {
@@ -2646,6 +2649,9 @@ CoreInitializeGcdServices (
                    ResourceHob->PhysicalStart,
                    ResourceHob->ResourceLength
                    );
+        if (EFI_ERROR (Status)) {
+          DEBUG ((DEBUG_GCD, "Failed to add reserved I/O or I/O resources, Status = %r\n", Status));
+        }
       }
     }
   }
@@ -2668,6 +2674,9 @@ CoreInitializeGcdServices (
                gDxeCoreImageHandle,
                NULL
                );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", Status));
+    }
   }
 
   //
@@ -2715,6 +2724,9 @@ CoreInitializeGcdServices (
                             gDxeCoreImageHandle,
                             NULL
                             );
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", Status));
+      }
     }
   }
 
@@ -2763,6 +2775,9 @@ CoreInitializeGcdServices (
                    gDxeCoreImageHandle,
                    NULL
                    );
+        if (EFI_ERROR (Status)) {
+          DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", Status));
+        }
       }
     }
   }
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 9dbfb2a1fad2..769e2d379051 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1010,6 +1010,9 @@ CoreUnloadAndCloseImage (
                              Image->Handle,
                              OpenInfo[OpenInfoIndex].ControllerHandle
                              );
+                  if (EFI_ERROR (Status)) {
+                    DEBUG ((DEBUG_WARN, "Failed to close protocol on Handle %p\n", HandleBuffer[HandleIndex]));
+                  }
                 }
               }
 
-- 
2.34.1



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