Re: [edk2-devel] [PATCH v7 00/12] Enable New CodeQL Queries

["Oliver Smith-Denny" <osd@smith-denny.com>]

With comments and for the patchset:

Reviewed-by: Oliver Smith-Denny <osd@smith-denny.com>

Thanks!

On 3/24/2023 3:30 PM, Michael Kubacki wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Adds queries for the following:
> 
> 1. cpp/conditionallyuninitializedvariable
> 2. cpp/pointer-overflow-check
> 3. cpp/overrunning-write
> 4. cpp/overrunning-write-with-float
> 5. cpp/very-likely-overrunning-write
> 
> These check for vulnerabilities with the following CWEs:
> 
>    - https://cwe.mitre.org/data/definitions/120.html
>    - https://cwe.mitre.org/data/definitions/457.html
>    - https://cwe.mitre.org/data/definitions/676.html
>    - https://cwe.mitre.org/data/definitions/758.html
>    - https://cwe.mitre.org/data/definitions/787.html
>    - https://cwe.mitre.org/data/definitions/805.html
> 
> The first part of this patch series contains fixes for CodeQL alerts
> across various packages that are produced by the new queries being
> enabled.
> 
> The second part updates the CodeQL queries.
> 
> Note: The changes are currently in the following pull request
> https://github.com/tianocore/edk2/pull/4133
> 
> v7 series changes:
> 
> 1. Added R-b tag to UefiCpuPkg patch
> 2. Merged Rebecca's patch https://edk2.groups.io/g/devel/message/101819
>     into [PATCH v7 02/12]
> 
> v6 series changes:
> 
> 1. Also change "1u" to "1" in:
>     - UefiCpuPkg/CpuMpPei/CpuPaging.c
>     - UefiCpuPkg/CpuMpPei/CpuMpPei.c
> 
> v5 series changes:
> 
> 1. Changed "1u" to "1" in UefiCpuPkg/CpuMpPei/CpuBist.c
> 2. Added Rb tags from v4 series
> 
> v4 series changes:
> 
> 1. Simplify conditional logic in Patch 1 per Michael Brown's
>     suggestion.
> 
> v3 series changes:
> 
> 1. Rebased series onto 93a21b4 (current edk2/master)
> 
> 2. Added v2 Rb tags
> 
> V2 series changes:
> 
> 1. MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
>     - Applied SafeUintnAdd() to both variables in the comparison
>       in ParseAndAddExistingSmbiosTable()
> 
>      Addresses feedback from: Mike Kinney
> 
> 2. CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
>     - Changes:
> 
>       if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
> 
>       To:
> 
>       if (((Inf & 0x80) == 0x00) && (Asn1Tag != V_ASN1_SEQUENCE)) {
> 
>      Addresses feedback from: Mike Kinney
> 
> 3. MdePkg/Library/BaseLib/String.c
>     - Removes: #include <Uefi/UefiBaseType.h>
>     - Changes conditional style in changes to if statement from
>       ternary for changes made throughout the file
>     - Updates commit message to describe change in return value
> 
>     Addresses feedback from: Mike Kinney
> 
> 4. NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
>     - Changes:
> 
>       if (!EFI_ERROR (Status) && (Data > HTTP_URI_PORT_MAX_NUM)) {
>         Status = EFI_INVALID_PARAMETER;
>         goto ON_EXIT;
>       }
> 
>       To:
> 
>       if (EFI_ERROR (Status) || (Data > HTTP_URI_PORT_MAX_NUM)) {
>         Status = EFI_INVALID_PARAMETER;
>         goto ON_EXIT;
>       }
> 
>     Addresses feedback from: Mike Kinney
> 
> 5. ShellPkg/Application/Shell/Shell.c
>     - Initializes CalleeStatus to EFI_SUCCESS in DoStartupScript()
>     - Restores original if statement logic in DoStartupScript()
> 
>     Addresses feedback from: Zhichao Gao
> 
> 6. ShellPkg/Application/Shell/ShellProtocol.c
>     - Adds additional check for return value from
>       PARSE_HANDLE_DATABASE_UEFI_DRIVERS() in EfiShellGetDeviceName()
> 
>     Addresses feedback from: Zhichao Gao
> 
> 7. Includes up-to-date R-b tags
> 
> ---
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Michael Brown <mcb30@ipxe.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Xiaoyu Lu <xiaoyu1.lu@intel.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Erich McMillan (1):
>    MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
> 
> Michael Kubacki (11):
>    BaseTools/PatchCheck.py: Add PCCTS to tab exemption list
>    BaseTools/VfrCompile: Fix potential buffer overwrites
>    CryptoPkg: Fix conditionally uninitialized variable
>    MdeModulePkg: Fix conditionally uninitialized variables
>    MdePkg: Fix conditionally uninitialized variables
>    NetworkPkg: Fix conditionally uninitialized variables
>    PcAtChipsetPkg: Fix conditionally uninitialized variables
>    ShellPkg: Fix conditionally uninitialized variables
>    UefiCpuPkg: Fix conditionally uninitialized variables
>    .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries
>    .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries
> 
>   BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c               | 10 ++--
>   BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c              |  4 +-
>   CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c                 | 21 ++++---
>   MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c                        |  5 +-
>   MdeModulePkg/Bus/Pci/UhciDxe/Uhci.c                           | 24 +++++---
>   MdeModulePkg/Core/Dxe/Mem/Page.c                              | 17 +++---
>   MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootOption.c | 25 ++++----
>   MdeModulePkg/Library/FileExplorerLib/FileExplorer.c           |  5 +-
>   MdeModulePkg/Universal/BdsDxe/BdsEntry.c                      | 33 ++++++-----
>   MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c      | 11 ++--
>   MdeModulePkg/Universal/HiiDatabaseDxe/Font.c                  | 14 +++--
>   MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c                  |  8 +--
>   MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c         |  2 +-
>   MdePkg/Library/BaseLib/String.c                               | 40 ++++++++++---
>   NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c                    |  2 +-
>   NetworkPkg/TcpDxe/TcpInput.c                                  |  3 +
>   PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c            |  9 ++-
>   ShellPkg/Application/Shell/Shell.c                            |  1 +
>   ShellPkg/Application/Shell/ShellProtocol.c                    | 60 ++++++++++----------
>   ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c    | 56 +++++++++---------
>   ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c            | 18 +++---
>   ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c   |  9 ++-
>   ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c        | 14 +++--
>   ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c     | 17 ++++--
>   ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c        | 21 +++----
>   UefiCpuPkg/CpuMpPei/CpuBist.c                                 |  8 ++-
>   UefiCpuPkg/CpuMpPei/CpuMpPei.c                                |  8 ++-
>   UefiCpuPkg/CpuMpPei/CpuPaging.c                               |  9 ++-
>   .github/codeql/edk2.qls                                       | 10 ++++
>   BaseTools/Scripts/PatchCheck.py                               |  5 +-
>   30 files changed, 286 insertions(+), 183 deletions(-)
>