devel@edk2.groups.io | [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts

Home Messages Hashtags Subgroups Calendar

Date Date 1 - 3 of 3

[PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts

Michael Kubacki

Thanks Mike. Erich, I'll include the update for this in the v2 series.

Show quoted text

On 11/23/2022 8:28 PM, Michael D Kinney wrote:
Hi Erich,
One comment below.
Mike

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Wednesday, November 9, 2022 9:33 AM
To: devel@edk2.groups.io
Cc: Bi, Dandan <dandan.bi@...>; Erich McMillan <emcmillan@...>; Wang, Jian J <jian.j.wang@...>; Gao,
Liming <gaoliming@...>; Michael Kubacki <mikuback@...>; Zeng, Star <star.zeng@...>; Gao,
Zhichao <zhichao.gao@...>; Liu, Zhiguang <zhiguang.liu@...>; Kubacki, Michael <michael.kubacki@...>
Subject: [edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts

From: Erich McMillan <emcmillan@...>

Details for these CodeQL alerts can be found here:

- Pointer overflow check (cpp/pointer-overflow-check):
   - https://cwe.mitre.org/data/definitions/758.html

- Potential buffer overflow check (cpp/potential-buffer-overflow):
   - https://cwe.mitre.org/data/definitions/676.html

CodeQL alert:

   - Line 1612 in MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
     - Type: Pointer overflow check
     - Severity: Low
     - Problem: Range check relying on pointer overflow

Cc: Dandan Bi <dandan.bi@...>
Cc: Erich McMillan <emcmillan@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Liming Gao <gaoliming@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Star Zeng <star.zeng@...>
Cc: Zhichao Gao <zhichao.gao@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Co-authored-by: Michael Kubacki <michael.kubacki@...>
Signed-off-by: Erich McMillan <emcmillan@...>
---
  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 4 +++-
  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf | 1 +
  2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
index 1d43adc7662c..03eca4e6b103 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
  **/

  #include "SmbiosDxe.h"
+#include <Library/SafeIntLib.h>

  //
  // Module Global:
@@ -1594,6 +1595,7 @@ ParseAndAddExistingSmbiosTable (
    CHAR8                     *String;
    EFI_SMBIOS_HANDLE         SmbiosHandle;
    SMBIOS_STRUCTURE_POINTER  SmbiosEnd;
+  UINTN                     SafeIntResult;

    mPrivateData.Smbios.MajorVersion = MajorVersion;
    mPrivateData.Smbios.MinorVersion = MinorVersion;
@@ -1609,7 +1611,7 @@ ParseAndAddExistingSmbiosTable (
      // Make sure not to access memory beyond SmbiosEnd
      //
      if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||The line above does the same unsafe add operation.
The use of SafeUintnAdd()should be moved before the if statement
and SafeIntResult should be used twice in the if statement.
The check for EFI_ERROR from SafeUintnAdd() can be performed
before the if statement.

-        (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
+        EFI_ERROR (SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE), &SafeIntResult)))
      {
        return EFI_INVALID_PARAMETER;
      }
diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
index c03915a6921f..8b7c74694775 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
@@ -42,6 +42,7 @@ [LibraryClasses]
    DebugLib
    PcdLib
    HobLib
+  SafeIntLib

  [Protocols]
    gEfiSmbiosProtocolGuid                            ## PRODUCES
--
2.28.0.windows.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96147): https://edk2.groups.io/g/devel/message/96147
Mute This Topic: https://groups.io/mt/94918085/1643496
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@...]
-=-=-=-=-=-=

Michael D Kinney

#96592

Hi Erich,

One comment below.

Mike

Show quoted text

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Wednesday, November 9, 2022 9:33 AM
To: devel@edk2.groups.io
Cc: Bi, Dandan <dandan.bi@...>; Erich McMillan <emcmillan@...>; Wang, Jian J <jian.j.wang@...>; Gao,
Liming <gaoliming@...>; Michael Kubacki <mikuback@...>; Zeng, Star <star.zeng@...>; Gao,
Zhichao <zhichao.gao@...>; Liu, Zhiguang <zhiguang.liu@...>; Kubacki, Michael <michael.kubacki@...>
Subject: [edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts

From: Erich McMillan <emcmillan@...>

Details for these CodeQL alerts can be found here:

- Pointer overflow check (cpp/pointer-overflow-check):
  - https://cwe.mitre.org/data/definitions/758.html

- Potential buffer overflow check (cpp/potential-buffer-overflow):
  - https://cwe.mitre.org/data/definitions/676.html

CodeQL alert:

  - Line 1612 in MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
    - Type: Pointer overflow check
    - Severity: Low
    - Problem: Range check relying on pointer overflow

Cc: Dandan Bi <dandan.bi@...>
Cc: Erich McMillan <emcmillan@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Liming Gao <gaoliming@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Star Zeng <star.zeng@...>
Cc: Zhichao Gao <zhichao.gao@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Co-authored-by: Michael Kubacki <michael.kubacki@...>
Signed-off-by: Erich McMillan <emcmillan@...>
---
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 4 +++-
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
index 1d43adc7662c..03eca4e6b103 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 **/

 #include "SmbiosDxe.h"
+#include <Library/SafeIntLib.h>

 //
 // Module Global:
@@ -1594,6 +1595,7 @@ ParseAndAddExistingSmbiosTable (
   CHAR8                     *String;
   EFI_SMBIOS_HANDLE         SmbiosHandle;
   SMBIOS_STRUCTURE_POINTER  SmbiosEnd;
+  UINTN                     SafeIntResult;

   mPrivateData.Smbios.MajorVersion = MajorVersion;
   mPrivateData.Smbios.MinorVersion = MinorVersion;
@@ -1609,7 +1611,7 @@ ParseAndAddExistingSmbiosTable (
     // Make sure not to access memory beyond SmbiosEnd
     //
     if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||The line above does the same unsafe add operation.  
The use of SafeUintnAdd()should be moved before the if statement
and SafeIntResult should be used twice in the if statement.
The check for EFI_ERROR from SafeUintnAdd() can be performed
before the if statement.

-        (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
+        EFI_ERROR (SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE), &SafeIntResult)))
     {
       return EFI_INVALID_PARAMETER;
     }
diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
index c03915a6921f..8b7c74694775 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
@@ -42,6 +42,7 @@ [LibraryClasses]
   DebugLib
   PcdLib
   HobLib
+  SafeIntLib

 [Protocols]
   gEfiSmbiosProtocolGuid                            ## PRODUCES
--
2.28.0.windows.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96147): https://edk2.groups.io/g/devel/message/96147
Mute This Topic: https://groups.io/mt/94918085/1643496
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@...]
-=-=-=-=-=-=

Michael Kubacki

#96147

From: Erich McMillan <emcmillan@...>

Details for these CodeQL alerts can be found here:

- Pointer overflow check (cpp/pointer-overflow-check):
  - https://cwe.mitre.org/data/definitions/758.html

- Potential buffer overflow check (cpp/potential-buffer-overflow):
  - https://cwe.mitre.org/data/definitions/676.html

CodeQL alert:

  - Line 1612 in MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
    - Type: Pointer overflow check
    - Severity: Low
    - Problem: Range check relying on pointer overflow

Cc: Dandan Bi <dandan.bi@...>
Cc: Erich McMillan <emcmillan@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Liming Gao <gaoliming@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Star Zeng <star.zeng@...>
Cc: Zhichao Gao <zhichao.gao@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Co-authored-by: Michael Kubacki <michael.kubacki@...>
Signed-off-by: Erich McMillan <emcmillan@...>
---
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 4 +++-
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c b/MdeModulePkg/=
Universal/SmbiosDxe/SmbiosDxe.c
index 1d43adc7662c..03eca4e6b103 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
=20
 #include "SmbiosDxe.h"
+#include <Library/SafeIntLib.h>
=20
 //
 // Module Global:
@@ -1594,6 +1595,7 @@ ParseAndAddExistingSmbiosTable (
   CHAR8                     *String;
   EFI_SMBIOS_HANDLE         SmbiosHandle;
   SMBIOS_STRUCTURE_POINTER  SmbiosEnd;
+  UINTN                     SafeIntResult;
=20
   mPrivateData.Smbios.MajorVersion =3D MajorVersion;
   mPrivateData.Smbios.MinorVersion =3D MinorVersion;
@@ -1609,7 +1611,7 @@ ParseAndAddExistingSmbiosTable (
     // Make sure not to access memory beyond SmbiosEnd
     //
     if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||
-        (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
+        EFI_ERROR (SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUC=
TURE), &SafeIntResult)))
     {
       return EFI_INVALID_PARAMETER;
     }
diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf b/MdeModulePk=
g/Universal/SmbiosDxe/SmbiosDxe.inf
index c03915a6921f..8b7c74694775 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
@@ -42,6 +42,7 @@ [LibraryClasses]
   DebugLib
   PcdLib
   HobLib
+  SafeIntLib
=20
 [Protocols]
   gEfiSmbiosProtocolGuid                            ## PRODUCES
--=20
2.28.0.windows.1

1 - 3 of 3

Previous Topic Next Topic

Home Hashtags Subgroups Calendar About Features Pricing Updates Terms Help Twitter