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

[Michael D Kinney]

Hi Erich,

One comment below.

Mike

> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel at edk2.groups.io
> Cc: Bi, Dandan <dandan.bi at intel.com>; Erich McMillan <emcmillan at microsoft.com>; Wang, Jian J <jian.j.wang at intel.com>; Gao,
> Liming <gaoliming at byosoft.com.cn>; Michael Kubacki <mikuback at linux.microsoft.com>; Zeng, Star <star.zeng at intel.com>; Gao,
> Zhichao <zhichao.gao at intel.com>; Liu, Zhiguang <zhiguang.liu at intel.com>; Kubacki, Michael <michael.kubacki at microsoft.com>
> Subject: [edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
> 
> From: Erich McMillan <emcmillan at microsoft.com>
> 
> 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 at intel.com>
> Cc: Erich McMillan <emcmillan at microsoft.com>
> Cc: Jian J Wang <jian.j.wang at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Michael Kubacki <mikuback at linux.microsoft.com>
> Cc: Star Zeng <star.zeng at intel.com>
> Cc: Zhichao Gao <zhichao.gao at intel.com>
> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
> Co-authored-by: Michael Kubacki <michael.kubacki at microsoft.com>
> Signed-off-by: Erich McMillan <emcmillan at microsoft.com>
> ---
>  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 at edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney at intel.com]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96592): https://edk2.groups.io/g/devel/message/96592
Mute This Topic: https://groups.io/mt/94918085/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-