* [PATCH v1 00/12] Enable New CodeQL Queries
@ 2022-11-09 17:32 Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts Michael Kubacki
` (11 more replies)
0 siblings, 12 replies; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel
Cc: Bob Feng, Dandan Bi, Eric Dong, Erich McMillan, Guomin Jiang,
Jian J Wang, Jiaxin Wu, Jiewen Yao, Liming Gao, Maciej Rabeda,
Michael D Kinney, Michael Kubacki, Rahul Kumar, Ray Ni,
Sean Brogan, Siyuan Fu, Star Zeng, Xiaoyu Lu, Yuwei Chen,
Zhichao Gao, Zhiguang Liu
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.
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 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 | 4 +-
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 2 +-
MdePkg/Library/BaseLib/String.c | 20 ++++---
NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c | 2 +-
NetworkPkg/TcpDxe/TcpInput.c | 3 ++
PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 9 ++--
ShellPkg/Application/Shell/Shell.c | 2 +-
ShellPkg/Application/Shell/ShellProtocol.c | 4 +-
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 | 4 +-
MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf | 1 +
31 files changed, 238 insertions(+), 152 deletions(-)
--
2.28.0.windows.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 1:28 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 02/12] BaseTools/PatchCheck.py: Add PCCTS to tab exemption list Michael Kubacki
` (10 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel
Cc: Dandan Bi, Erich McMillan, Jian J Wang, Liming Gao,
Michael Kubacki, Star Zeng, Zhichao Gao, Zhiguang Liu,
Michael Kubacki
From: Erich McMillan <emcmillan@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@intel.com>
Cc: Erich McMillan <emcmillan@microsoft.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Co-authored-by: Michael Kubacki <michael.kubacki@microsoft.com>
Signed-off-by: Erich McMillan <emcmillan@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) ||
- (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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 02/12] BaseTools/PatchCheck.py: Add PCCTS to tab exemption list
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 1:30 ` Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites Michael Kubacki
` (9 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel; +Cc: Bob Feng, Liming Gao, Michael D Kinney, Sean Brogan, Yuwei Chen
From: Michael Kubacki <michael.kubacki@microsoft.com>
Purdue Compiler Construction Tool Set (PCCTS) source code was copied/
pasted into BaseTools/Source/C/VfrCompile/Pccts/.
The code contains tab characters instead of spaces.
PatchCheck.py gives an error on modifications to files that
contain tabs.
This change adds that directory to the pre-existing list of
directories in which tab checks are ignored in PatchCheck.py.
The goal of my upcoming change there is not to mix tabs and spaces
but to fix a bug while preserving its current formatting characters.
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
BaseTools/Scripts/PatchCheck.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 475b3a8c27d9..a02fdb470ee2 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -384,7 +384,9 @@ class GitDiffCheck:
self.force_crlf = False
self.force_notabs = False
if os.path.basename(self.filename) == 'GNUmakefile' or \
- os.path.basename(self.filename) == 'Makefile':
+ os.path.basename(self.filename) == 'Makefile' or \
+ self.filename.startswith(
+ 'BaseTools/Source/C/VfrCompile/Pccts/'):
self.force_notabs = False
elif len(line.rstrip()) != 0:
self.format_error("didn't find diff command")
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 02/12] BaseTools/PatchCheck.py: Add PCCTS to tab exemption list Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 1:32 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable Michael Kubacki
` (8 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel; +Cc: Bob Feng, Liming Gao, Michael D Kinney, Sean Brogan, Yuwei Chen
From: Michael Kubacki <michael.kubacki@microsoft.com>
While more portable methods exist to handle these cases, this change
does not attempt to do more than fix the immediate problem and
follow the conventions already established in this code.
`snprintf()` is introduced as the minimum improvement apart from
making the buffers larger.
Fixes the following CodeQL alerts:
1. Failure on line 2339 in
BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
- Type: Potentially overrunning write
- Severity: Critical
- Problem: This 'call to sprintf' operation requires 17 bytes but
the destination is only 16 bytes.
2. Failure on line 2341 in
BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
- Type: Potentially overrunning write
- Severity: Critical
- Problem: This 'call to sprintf' operation requires 17 bytes but
the destination is only 16 bytes.
3. Failure on line 1309 in
BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
- Type: Potentially overrunning write
- Severity: Critical
- Problem: This 'call to sprintf' operation requires 25 bytes but
the destination is only 20 bytes.
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Yuwei Chen <yuwei.chen@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c | 10 +++++-----
BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c b/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
index 8e41239f4751..33d9cac4c7de 100644
--- a/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
+++ b/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
@@ -2331,14 +2331,14 @@ TokNode *p;
set_nameErrSet = bufErrSet; /* MR23 */
}
else { /* wild card */
- static char buf[sizeof("zzerr")+10];
- static char bufErrSet[sizeof("zzerr")+10];
+ static char buf[sizeof("zzerr")+11];
+ static char bufErrSet[sizeof("zzerr")+11];
int n = DefErrSet( &b, 0, NULL );
int nErrSet = DefErrSetWithSuffix(0, &bErrSet, 1, NULL, "_set");
- if ( GenCC ) sprintf(buf, "err%d", n);
- else sprintf(buf, "zzerr%d", n);
+ if ( GenCC ) snprintf(buf, 11, "err%d", n);
+ else snprintf(buf, 11, "zzerr%d", n);
if ( GenCC ) sprintf(bufErrSet, "err%d", nErrSet);
- else sprintf(bufErrSet, "zzerr%d", nErrSet);
+ else snprintf(bufErrSet, 11, "zzerr%d", nErrSet);
set_name = buf;
set_nameErrSet = bufErrSet;
}
diff --git a/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c b/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
index 051ee4ec5d28..488b4b90461c 100644
--- a/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
+++ b/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
@@ -1295,7 +1295,7 @@ int token;
#endif
{
int j;
- static char imag_name[20];
+ static char imag_name[25];
/* look in all lexclasses for the token */
if ( TokenString(token) != NULL ) return TokenString(token);
@@ -1306,7 +1306,7 @@ int token;
}
if (1) {
- sprintf(imag_name,"UnknownToken#%d",token); /* MR13 */
+ snprintf(imag_name, 25, "UnknownToken#%d", token); /* MR13 */
return imag_name; /* MR13 */
}
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (2 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 1:37 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 05/12] MdeModulePkg: Fix conditionally uninitialized variables Michael Kubacki
` (7 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel
Cc: Erich McMillan, Guomin Jiang, Jian J Wang, Jiewen Yao,
Michael Kubacki, Xiaoyu Lu
From: Michael Kubacki <michael.kubacki@microsoft.com>
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
Checks the return value from `ASN1_get_object()` to verify values
set by the function are valid.
Note that the function returns literal `0x80`:
`return (0x80);`
That is used to check the return value is as the case in other areas
of the code.
Cc: Erich McMillan <emcmillan@microsoft.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Xiaoyu Lu <xiaoyu1.lu@intel.com>
Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c | 21 +++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
index 2333157e0d17..f867656e888c 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
@@ -807,6 +807,7 @@ X509GetTBSCert (
UINT32 Asn1Tag;
UINT32 ObjClass;
UINTN Length;
+ UINTN Inf;
//
// Check input parameters.
@@ -836,9 +837,9 @@ X509GetTBSCert (
//
Temp = Cert;
Length = 0;
- ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjClass, (long)CertSize);
+ Inf = ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjClass, (long)CertSize);
- if (Asn1Tag != V_ASN1_SEQUENCE) {
+ if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
return FALSE;
}
@@ -848,7 +849,7 @@ X509GetTBSCert (
//
// Verify the parsed TBSCertificate is one correct SEQUENCE data.
//
- if (Asn1Tag != V_ASN1_SEQUENCE) {
+ if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
return FALSE;
}
@@ -1888,18 +1889,20 @@ Asn1GetTag (
IN UINT32 Tag
)
{
- UINT8 *PtrOld;
- INT32 ObjTag;
- INT32 ObjCls;
- long ObjLength;
+ UINT8 *PtrOld;
+ INT32 ObjTag;
+ INT32 ObjCls;
+ long ObjLength;
+ UINT32 Inf;
//
// Save Ptr position
//
PtrOld = *Ptr;
- ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
- if ((ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
+ Inf = ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
+ if (!(Inf & 0x80) &&
+ (ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
(ObjCls == (INT32)(Tag & CRYPTO_ASN1_TAG_CLASS_MASK)))
{
*Length = (UINTN)ObjLength;
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 05/12] MdeModulePkg: Fix conditionally uninitialized variables
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (3 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 06/12] MdePkg: " Michael Kubacki
` (6 subsequent siblings)
11 siblings, 0 replies; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel
Cc: Dandan Bi, Eric Dong, Erich McMillan, Guomin Jiang, Jian J Wang,
Liming Gao, Michael Kubacki, Ray Ni, Zhichao Gao
From: Michael Kubacki <michael.kubacki@microsoft.com>
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
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: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
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/Variable/RuntimeDxe/Variable.c | 2 +-
9 files changed, 80 insertions(+), 56 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
index 843815d0cb18..14bed5472958 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
@@ -1407,6 +1407,7 @@ SupportPaletteSnoopAttributes (
IN EFI_PCI_IO_PROTOCOL_ATTRIBUTE_OPERATION Operation
)
{
+ EFI_STATUS Status;
PCI_IO_DEVICE *Temp;
UINT16 VGACommand;
@@ -1444,13 +1445,13 @@ SupportPaletteSnoopAttributes (
// Check if they are on the same bus
//
if (Temp->Parent == PciIoDevice->Parent) {
- PCI_READ_COMMAND_REGISTER (Temp, &VGACommand);
+ Status = PCI_READ_COMMAND_REGISTER (Temp, &VGACommand);
//
// If they are on the same bus, either one can
// be set to snoop, the other set to decode
//
- if ((VGACommand & EFI_PCI_COMMAND_VGA_PALETTE_SNOOP) != 0) {
+ if (!EFI_ERROR (Status) && ((VGACommand & EFI_PCI_COMMAND_VGA_PALETTE_SNOOP) != 0)) {
//
// VGA has set to snoop, so GFX can be only set to disable snoop
//
diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/Uhci.c b/MdeModulePkg/Bus/Pci/UhciDxe/Uhci.c
index 48741085e507..496ffbd5c4cc 100644
--- a/MdeModulePkg/Bus/Pci/UhciDxe/Uhci.c
+++ b/MdeModulePkg/Bus/Pci/UhciDxe/Uhci.c
@@ -730,10 +730,12 @@ Uhci2ControlTransfer (
Uhc->PciIo->Flush (Uhc->PciIo);
- *TransferResult = QhResult.Result;
+ if (!EFI_ERROR (Status)) {
+ *TransferResult = QhResult.Result;
- if (DataLength != NULL) {
- *DataLength = QhResult.Complete;
+ if (DataLength != NULL) {
+ *DataLength = QhResult.Complete;
+ }
}
UhciDestoryTds (Uhc, TDs);
@@ -884,9 +886,11 @@ Uhci2BulkTransfer (
Uhc->PciIo->Flush (Uhc->PciIo);
- *TransferResult = QhResult.Result;
- *DataToggle = QhResult.NextToggle;
- *DataLength = QhResult.Complete;
+ if (!EFI_ERROR (Status)) {
+ *TransferResult = QhResult.Result;
+ *DataToggle = QhResult.NextToggle;
+ *DataLength = QhResult.Complete;
+ }
UhciDestoryTds (Uhc, TDs);
Uhc->PciIo->Unmap (Uhc->PciIo, DataMap);
@@ -1210,9 +1214,11 @@ Uhci2SyncInterruptTransfer (
UhciUnlinkTdFromQh (Uhc->SyncIntQh, TDs);
Uhc->PciIo->Flush (Uhc->PciIo);
- *TransferResult = QhResult.Result;
- *DataToggle = QhResult.NextToggle;
- *DataLength = QhResult.Complete;
+ if (!EFI_ERROR (Status)) {
+ *TransferResult = QhResult.Result;
+ *DataToggle = QhResult.NextToggle;
+ *DataLength = QhResult.Complete;
+ }
UhciDestoryTds (Uhc, TDs);
Uhc->PciIo->Unmap (Uhc->PciIo, DataMap);
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 160289c1f9ec..2eb07b56b420 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -449,14 +449,15 @@ PromoteMemoryResource (
//
Promoted = PromoteGuardedFreePages (&StartAddress, &EndAddress);
if (Promoted) {
- CoreGetMemorySpaceDescriptor (StartAddress, &Descriptor);
- CoreAddRange (
- EfiConventionalMemory,
- StartAddress,
- EndAddress,
- Descriptor.Capabilities & ~(EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED |
- EFI_MEMORY_TESTED | EFI_MEMORY_RUNTIME)
- );
+ if (!EFI_ERROR (CoreGetMemorySpaceDescriptor (StartAddress, &Descriptor))) {
+ CoreAddRange (
+ EfiConventionalMemory,
+ StartAddress,
+ EndAddress,
+ Descriptor.Capabilities & ~(EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED |
+ EFI_MEMORY_TESTED | EFI_MEMORY_RUNTIME)
+ );
+ }
}
}
diff --git a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootOption.c b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootOption.c
index cdaa2db15365..e22aaf3039f1 100644
--- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootOption.c
+++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootOption.c
@@ -909,23 +909,28 @@ BootFromFile (
IN EFI_DEVICE_PATH_PROTOCOL *FilePath
)
{
+ EFI_STATUS Status;
EFI_BOOT_MANAGER_LOAD_OPTION BootOption;
CHAR16 *FileName;
+ Status = EFI_NOT_STARTED;
FileName = NULL;
FileName = ExtractFileNameFromDevicePath (FilePath);
if (FileName != NULL) {
- EfiBootManagerInitializeLoadOption (
- &BootOption,
- 0,
- LoadOptionTypeBoot,
- LOAD_OPTION_ACTIVE,
- FileName,
- FilePath,
- NULL,
- 0
- );
+ Status = EfiBootManagerInitializeLoadOption (
+ &BootOption,
+ 0,
+ LoadOptionTypeBoot,
+ LOAD_OPTION_ACTIVE,
+ FileName,
+ FilePath,
+ NULL,
+ 0
+ );
+ }
+
+ if (!EFI_ERROR (Status)) {
//
// Since current no boot from removable media directly is allowed */
//
diff --git a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c
index ef949267fcc2..804a03d868f2 100644
--- a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c
+++ b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c
@@ -1075,7 +1075,10 @@ LibCreateNewFile (
NewHandle = NULL;
FullFileName = NULL;
- LibGetFileHandleFromDevicePath (gFileExplorerPrivate.RetDevicePath, &FileHandle, &ParentName, &DeviceHandle);
+ if (EFI_ERROR (LibGetFileHandleFromDevicePath (gFileExplorerPrivate.RetDevicePath, &FileHandle, &ParentName, &DeviceHandle))) {
+ return EFI_DEVICE_ERROR;
+ }
+
FullFileName = LibAppendFileName (ParentName, FileName);
if (FullFileName == NULL) {
return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 766dde3aaeeb..72de8d3211b7 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -691,6 +691,7 @@ BdsEntry (
EFI_DEVICE_PATH_PROTOCOL *FilePath;
EFI_STATUS BootManagerMenuStatus;
EFI_BOOT_MANAGER_LOAD_OPTION PlatformDefaultBootOption;
+ BOOLEAN PlatformDefaultBootOptionValid;
HotkeyTriggered = NULL;
Status = EFI_SUCCESS;
@@ -809,24 +810,24 @@ BdsEntry (
CpuDeadLoop ();
}
- Status = EfiBootManagerInitializeLoadOption (
- &PlatformDefaultBootOption,
- LoadOptionNumberUnassigned,
- LoadOptionTypePlatformRecovery,
- LOAD_OPTION_ACTIVE,
- L"Default PlatformRecovery",
- FilePath,
- NULL,
- 0
- );
- ASSERT_EFI_ERROR (Status);
+ PlatformDefaultBootOptionValid = EfiBootManagerInitializeLoadOption (
+ &PlatformDefaultBootOption,
+ LoadOptionNumberUnassigned,
+ LoadOptionTypePlatformRecovery,
+ LOAD_OPTION_ACTIVE,
+ L"Default PlatformRecovery",
+ FilePath,
+ NULL,
+ 0
+ ) == EFI_SUCCESS;
+ ASSERT (PlatformDefaultBootOptionValid == TRUE);
//
// System firmware must include a PlatformRecovery#### variable specifying
// a short-form File Path Media Device Path containing the platform default
// file path for removable media if the platform supports Platform Recovery.
//
- if (PcdGetBool (PcdPlatformRecoverySupport)) {
+ if (PlatformDefaultBootOptionValid && PcdGetBool (PcdPlatformRecoverySupport)) {
LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
if (EfiBootManagerFindLoadOption (&PlatformDefaultBootOption, LoadOptions, LoadOptionCount) == -1) {
for (Index = 0; Index < LoadOptionCount; Index++) {
@@ -1104,15 +1105,17 @@ BdsEntry (
LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
ProcessLoadOptions (LoadOptions, LoadOptionCount);
EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
- } else {
+ } else if (PlatformDefaultBootOptionValid) {
//
// When platform recovery is not enabled, still boot to platform default file path.
//
- EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
+ PlatformDefaultBootOptionValid = EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption) == EFI_SUCCESS;
}
}
- EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption);
+ if (PlatformDefaultBootOptionValid) {
+ EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption);
+ }
DEBUG ((DEBUG_ERROR, "[Bds] Unable to boot!\n"));
PlatformBootManagerUnableToBoot ();
diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
index dca3c1df07ba..0d4cfa4cf06f 100644
--- a/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
+++ b/MdeModulePkg/Universal/DisplayEngineDxe/ProcessOptions.c
@@ -944,13 +944,14 @@ PrintMismatchMenuInfo (
UINTN FormsetBufferSize;
Question = MenuOption->ThisTag;
- HiiGetFormSetFromHiiHandle (gFormData->HiiHandle, &FormsetBuffer, &FormsetBufferSize);
- FormSetTitleStr = GetToken (FormsetBuffer->FormSetTitle, gFormData->HiiHandle);
- FormTitleStr = GetToken (gFormData->FormTitle, gFormData->HiiHandle);
+ if (!EFI_ERROR (HiiGetFormSetFromHiiHandle (gFormData->HiiHandle, &FormsetBuffer, &FormsetBufferSize))) {
+ FormSetTitleStr = GetToken (FormsetBuffer->FormSetTitle, gFormData->HiiHandle);
+ FormTitleStr = GetToken (gFormData->FormTitle, gFormData->HiiHandle);
- DEBUG ((DEBUG_ERROR, "\n[%a]: Mismatch Formset : Formset Guid = %g, FormSet title = %s\n", gEfiCallerBaseName, &gFormData->FormSetGuid, FormSetTitleStr));
- DEBUG ((DEBUG_ERROR, "[%a]: Mismatch Form : FormId = %d, Form title = %s.\n", gEfiCallerBaseName, gFormData->FormId, FormTitleStr));
+ DEBUG ((DEBUG_ERROR, "\n[%a]: Mismatch Formset : Formset Guid = %g, FormSet title = %s\n", gEfiCallerBaseName, &gFormData->FormSetGuid, FormSetTitleStr));
+ DEBUG ((DEBUG_ERROR, "[%a]: Mismatch Form : FormId = %d, Form title = %s.\n", gEfiCallerBaseName, gFormData->FormId, FormTitleStr));
+ }
if (Question->OpCode->OpCode == EFI_IFR_ORDERED_LIST_OP) {
QuestionName = GetToken (((EFI_IFR_ORDERED_LIST *)MenuOption->ThisTag->OpCode)->Question.Header.Prompt, gFormData->HiiHandle);
diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c
index 399f90feb783..8a0b12f72fbe 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Font.c
@@ -1745,6 +1745,7 @@ HiiStringToImage (
Attributes = (UINT8 *)AllocateZeroPool (StrLength * sizeof (UINT8));
ASSERT (Attributes != NULL);
+ FontInfo = NULL;
RowInfo = NULL;
Status = EFI_SUCCESS;
StringIn2 = NULL;
@@ -1787,11 +1788,14 @@ HiiStringToImage (
Background = ((EFI_FONT_DISPLAY_INFO *)StringInfo)->BackgroundColor;
} else if (Status == EFI_SUCCESS) {
FontInfo = &StringInfoOut->FontInfo;
- IsFontInfoExisted (Private, FontInfo, NULL, NULL, &GlobalFont);
- Height = GlobalFont->FontPackage->Height;
- BaseLine = GlobalFont->FontPackage->BaseLine;
- Foreground = StringInfoOut->ForegroundColor;
- Background = StringInfoOut->BackgroundColor;
+ if (IsFontInfoExisted (Private, FontInfo, NULL, NULL, &GlobalFont)) {
+ Height = GlobalFont->FontPackage->Height;
+ BaseLine = GlobalFont->FontPackage->BaseLine;
+ Foreground = StringInfoOut->ForegroundColor;
+ Background = StringInfoOut->BackgroundColor;
+ } else {
+ goto Exit;
+ }
} else {
goto Exit;
}
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 6c1a3440ac8c..b64fcbdc7281 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2453,7 +2453,7 @@ VariableServiceGetVariable (
AcquireLockOnlyAtBootTime (&mVariableModuleGlobal->VariableGlobal.VariableServicesLock);
Status = FindVariable (VariableName, VendorGuid, &Variable, &mVariableModuleGlobal->VariableGlobal, FALSE);
- if ((Variable.CurrPtr == NULL) || EFI_ERROR (Status)) {
+ if (EFI_ERROR (Status) || (Variable.CurrPtr == NULL)) {
goto Done;
}
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (4 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 05/12] MdeModulePkg: Fix conditionally uninitialized variables Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 1:53 ` Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 07/12] NetworkPkg: " Michael Kubacki
` (5 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel
Cc: Erich McMillan, Liming Gao, Michael D Kinney, Michael Kubacki,
Zhiguang Liu
From: Michael Kubacki <michael.kubacki@microsoft.com>
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
Cc: Erich McMillan <emcmillan@microsoft.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
MdePkg/Library/BaseLib/String.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
index 98e6d31463e0..0ff0454b9d98 100644
--- a/MdePkg/Library/BaseLib/String.c
+++ b/MdePkg/Library/BaseLib/String.c
@@ -6,6 +6,7 @@
**/
+#include <Uefi/UefiBaseType.h>
#include "BaseLibInternals.h"
/**
@@ -408,7 +409,8 @@ StrDecimalToUintn (
{
UINTN Result;
- StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result);
+ Result = !EFI_ERROR (StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINTN;
+
return Result;
}
@@ -454,7 +456,8 @@ StrDecimalToUint64 (
{
UINT64 Result;
- StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result);
+ Result = !EFI_ERROR (StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINT64;
+
return Result;
}
@@ -501,7 +504,8 @@ StrHexToUintn (
{
UINTN Result;
- StrHexToUintnS (String, (CHAR16 **)NULL, &Result);
+ Result = !EFI_ERROR (StrHexToUintnS (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINTN;
+
return Result;
}
@@ -548,7 +552,7 @@ StrHexToUint64 (
{
UINT64 Result;
- StrHexToUint64S (String, (CHAR16 **)NULL, &Result);
+ Result = !EFI_ERROR (StrHexToUint64S (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINT64;
return Result;
}
@@ -989,7 +993,7 @@ AsciiStrDecimalToUintn (
{
UINTN Result;
- AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result);
+ Result = !EFI_ERROR (AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINTN;
return Result;
}
@@ -1031,7 +1035,7 @@ AsciiStrDecimalToUint64 (
{
UINT64 Result;
- AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result);
+ Result = !EFI_ERROR (AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINT64;
return Result;
}
@@ -1077,7 +1081,7 @@ AsciiStrHexToUintn (
{
UINTN Result;
- AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result);
+ Result = !EFI_ERROR (AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINTN;
return Result;
}
@@ -1123,7 +1127,7 @@ AsciiStrHexToUint64 (
{
UINT64 Result;
- AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result);
+ Result = !EFI_ERROR (AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINT64;
return Result;
}
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 07/12] NetworkPkg: Fix conditionally uninitialized variables
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (5 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 06/12] MdePkg: " Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 1:59 ` Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 08/12] PcAtChipsetPkg: " Michael Kubacki
` (4 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel
Cc: Erich McMillan, Jiaxin Wu, Maciej Rabeda, Michael D Kinney,
Michael Kubacki, Siyuan Fu
From: Michael Kubacki <michael.kubacki@microsoft.com>
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
Cc: Erich McMillan <emcmillan@microsoft.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c | 2 +-
NetworkPkg/TcpDxe/TcpInput.c | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
index 6a5d78629bb3..71c98abc820e 100644
--- a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -753,7 +753,7 @@ HttpUrlGetPort (
Status = AsciiStrDecimalToUintnS (Url + Parser->FieldData[HTTP_URI_FIELD_PORT].Offset, (CHAR8 **)NULL, &Data);
- if (Data > HTTP_URI_PORT_MAX_NUM) {
+ if (!EFI_ERROR (Status) && (Data > HTTP_URI_PORT_MAX_NUM)) {
Status = EFI_INVALID_PARAMETER;
goto ON_EXIT;
}
diff --git a/NetworkPkg/TcpDxe/TcpInput.c b/NetworkPkg/TcpDxe/TcpInput.c
index fb1aa827f8ba..7b329be64dfe 100644
--- a/NetworkPkg/TcpDxe/TcpInput.c
+++ b/NetworkPkg/TcpDxe/TcpInput.c
@@ -1570,6 +1570,9 @@ TcpIcmpInput (
BOOLEAN IcmpErrIsHard;
BOOLEAN IcmpErrNotify;
+ IcmpErrIsHard = FALSE;
+ IcmpErrNotify = FALSE;
+
if (Nbuf->TotalSize < sizeof (TCP_HEAD)) {
goto CLEAN_EXIT;
}
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 08/12] PcAtChipsetPkg: Fix conditionally uninitialized variables
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (6 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 07/12] NetworkPkg: " Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 2:00 ` Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 09/12] ShellPkg: " Michael Kubacki
` (3 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel; +Cc: Erich McMillan, Michael D Kinney, Michael Kubacki, Ray Ni
From: Michael Kubacki <michael.kubacki@microsoft.com>
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
Cc: Erich McMillan <emcmillan@microsoft.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
index 9242a2e82600..57ea3153aa6b 100644
--- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
+++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
@@ -344,7 +344,7 @@ PcRtcInit (
// so we can use them to get and set wakeup time.
//
Status = PcRtcGetWakeupTime (&Enabled, &Pending, &Time, Global);
- if ((Enabled) || (!EFI_ERROR (Status))) {
+ if ((!EFI_ERROR (Status)) || (Enabled)) {
return EFI_SUCCESS;
}
@@ -836,8 +836,11 @@ PcRtcSetWakeupTime (
//
// Just support set alarm time within 24 hours
//
- PcRtcGetTime (&RtcTime, &Capabilities, Global);
- Status = RtcTimeFieldsValid (&RtcTime);
+ Status = PcRtcGetTime (&RtcTime, &Capabilities, Global);
+ if (!EFI_ERROR (Status)) {
+ Status = RtcTimeFieldsValid (&RtcTime);
+ }
+
if (EFI_ERROR (Status)) {
return EFI_DEVICE_ERROR;
}
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (7 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 08/12] PcAtChipsetPkg: " Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 2:19 ` Gao, Zhichao
2022-11-09 17:32 ` [PATCH v1 10/12] UefiCpuPkg: " Michael Kubacki
` (2 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel
Cc: Erich McMillan, Michael D Kinney, Michael Kubacki, Ray Ni,
Zhichao Gao
From: Michael Kubacki <michael.kubacki@microsoft.com>
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
Cc: Erich McMillan <emcmillan@microsoft.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
ShellPkg/Application/Shell/Shell.c | 2 +-
ShellPkg/Application/Shell/ShellProtocol.c | 4 +-
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 ++++----
8 files changed, 78 insertions(+), 63 deletions(-)
diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
index df00adfdfa5b..86db2f4ebb6e 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1324,7 +1324,7 @@ DoStartupScript (
}
Status = RunShellCommand (FileStringPath, &CalleeStatus);
- if (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE) {
+ if (!EFI_ERROR (Status) && (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE)) {
ShellCommandRegisterExit (gEfiShellProtocol->BatchIsActive (), (UINT64)CalleeStatus);
}
diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c
index 509eb60e40f4..9183da284fff 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -729,8 +729,8 @@ EfiShellGetDeviceName (
//
// Now check the parent controller using this as the child.
//
- if (DeviceNameToReturn == NULL) {
- PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle, &ParentControllerCount, &ParentControllerBuffer);
+ Status = PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle, &ParentControllerCount, &ParentControllerBuffer);
+ if ((DeviceNameToReturn == NULL) && !EFI_ERROR (Status)) {
for (LoopVar = 0; LoopVar < ParentControllerCount; LoopVar++) {
PARSE_HANDLE_DATABASE_UEFI_DRIVERS (ParentControllerBuffer[LoopVar], &ParentDriverCount, &ParentDriverBuffer);
for (HandleCount = 0; HandleCount < ParentDriverCount; HandleCount++) {
diff --git a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
index 36cf46fb2c38..4549cbde9b9a 100644
--- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
+++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
@@ -1399,10 +1399,11 @@ ShellCommandCreateInitialMappingsAndPaths (
CHAR16 *MapName;
SHELL_MAP_LIST *MapListItem;
- SplitCurDir = NULL;
- MapName = NULL;
- MapListItem = NULL;
- HandleList = NULL;
+ ConsistMappingTable = NULL;
+ SplitCurDir = NULL;
+ MapName = NULL;
+ MapListItem = NULL;
+ HandleList = NULL;
//
// Reset the static members back to zero
@@ -1458,32 +1459,35 @@ ShellCommandCreateInitialMappingsAndPaths (
//
PerformQuickSort (DevicePathList, Count, sizeof (EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);
- ShellCommandConsistMappingInitialize (&ConsistMappingTable);
- //
- // Assign new Mappings to all...
- //
- for (Count = 0; HandleList[Count] != NULL; Count++) {
+ if (!EFI_ERROR (ShellCommandConsistMappingInitialize (&ConsistMappingTable))) {
//
- // Get default name first
+ // Assign new Mappings to all...
//
- NewDefaultName = ShellCommandCreateNewMappingName (MappingTypeFileSystem);
- ASSERT (NewDefaultName != NULL);
- Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName, DevicePathList[Count], 0, TRUE);
- ASSERT_EFI_ERROR (Status);
- FreePool (NewDefaultName);
-
- //
- // Now do consistent name
- //
- NewConsistName = ShellCommandConsistMappingGenMappingName (DevicePathList[Count], ConsistMappingTable);
- if (NewConsistName != NULL) {
- Status = ShellCommandAddMapItemAndUpdatePath (NewConsistName, DevicePathList[Count], 0, FALSE);
+ for (Count = 0; HandleList[Count] != NULL; Count++) {
+ //
+ // Get default name first
+ //
+ NewDefaultName = ShellCommandCreateNewMappingName (MappingTypeFileSystem);
+ ASSERT (NewDefaultName != NULL);
+ Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName, DevicePathList[Count], 0, TRUE);
ASSERT_EFI_ERROR (Status);
- FreePool (NewConsistName);
+ FreePool (NewDefaultName);
+
+ //
+ // Now do consistent name
+ //
+ NewConsistName = ShellCommandConsistMappingGenMappingName (DevicePathList[Count], ConsistMappingTable);
+ if (NewConsistName != NULL) {
+ Status = ShellCommandAddMapItemAndUpdatePath (NewConsistName, DevicePathList[Count], 0, FALSE);
+ ASSERT_EFI_ERROR (Status);
+ FreePool (NewConsistName);
+ }
}
}
- ShellCommandConsistMappingUnInitialize (ConsistMappingTable);
+ if (ConsistMappingTable != NULL) {
+ ShellCommandConsistMappingUnInitialize (ConsistMappingTable);
+ }
SHELL_FREE_NON_NULL (HandleList);
SHELL_FREE_NON_NULL (DevicePathList);
@@ -1626,12 +1630,12 @@ ShellCommandUpdateMapping (
//
PerformQuickSort (DevicePathList, Count, sizeof (EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);
- ShellCommandConsistMappingInitialize (&ConsistMappingTable);
+ Status = ShellCommandConsistMappingInitialize (&ConsistMappingTable);
//
// Assign new Mappings to remainders
//
- for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL && !EFI_ERROR (Status); Count++) {
+ for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL; Count++) {
//
// Skip ones that already have
//
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
index 97a4b57a932f..5329b559ba46 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
@@ -158,7 +158,10 @@ ShellCommandRunDblk (
ShellStatus = SHELL_INVALID_PARAMETER;
}
- ShellConvertStringToUint64 (LbaString, &Lba, TRUE, FALSE);
+ if (EFI_ERROR (ShellConvertStringToUint64 (LbaString, &Lba, TRUE, FALSE))) {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV), gShellDebug1HiiHandle, L"dblk", LbaString);
+ ShellStatus = SHELL_INVALID_PARAMETER;
+ }
}
if (BlockCountString == NULL) {
@@ -169,12 +172,13 @@ ShellCommandRunDblk (
ShellStatus = SHELL_INVALID_PARAMETER;
}
- ShellConvertStringToUint64 (BlockCountString, &BlockCount, TRUE, FALSE);
- if (BlockCount > 0x10) {
- BlockCount = 0x10;
- } else if (BlockCount == 0) {
- ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV), gShellDebug1HiiHandle, L"dblk", BlockCountString);
- ShellStatus = SHELL_INVALID_PARAMETER;
+ if (!EFI_ERROR (ShellConvertStringToUint64 (BlockCountString, &BlockCount, TRUE, FALSE))) {
+ if (BlockCount > 0x10) {
+ BlockCount = 0x10;
+ } else if (BlockCount == 0) {
+ ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV), gShellDebug1HiiHandle, L"dblk", BlockCountString);
+ ShellStatus = SHELL_INVALID_PARAMETER;
+ }
}
}
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
index 8bf23a2076a1..72f8c087cb69 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
@@ -112,10 +112,13 @@ ShellCommandRunEfiDecompress (
if (ShellStatus == SHELL_SUCCESS) {
Status = FileHandleGetSize (InFileHandle, &Temp64Bit);
- ASSERT (Temp64Bit <= (UINT32)(-1));
- InSize = (UINTN)Temp64Bit;
ASSERT_EFI_ERROR (Status);
- InBuffer = AllocateZeroPool (InSize);
+ if (!EFI_ERROR (Status)) {
+ ASSERT (Temp64Bit <= (UINT32)(-1));
+ InSize = (UINTN)Temp64Bit;
+ InBuffer = AllocateZeroPool (InSize);
+ }
+
if (InBuffer == NULL) {
Status = EFI_OUT_OF_RESOURCES;
} else {
diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
index d7a133c0c5b4..870c5b0d1da7 100644
--- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
+++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
@@ -508,9 +508,10 @@ ShellCommandRunConnect (
Count = ShellCommandLineGetCount (Package);
if (Param1 != NULL) {
- Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE, FALSE);
- Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
- if (EFI_ERROR (Status)) {
+ Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE, FALSE);
+ if (!EFI_ERROR (Status)) {
+ Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE), gShellDriver1HiiHandle, L"connect", Param1);
ShellStatus = SHELL_INVALID_PARAMETER;
}
@@ -519,9 +520,10 @@ ShellCommandRunConnect (
}
if (Param2 != NULL) {
- Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE, FALSE);
- Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
- if (EFI_ERROR (Status)) {
+ Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE, FALSE);
+ if (!EFI_ERROR (Status)) {
+ Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE), gShellDriver1HiiHandle, L"connect", Param2);
ShellStatus = SHELL_INVALID_PARAMETER;
}
diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
index 009ae5282b27..fd49d1f7ceb4 100644
--- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
+++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
@@ -160,12 +160,17 @@ ShellCommandRunDisconnect (
Param1 = ShellCommandLineGetRawValue (Package, 1);
Param2 = ShellCommandLineGetRawValue (Package, 2);
Param3 = ShellCommandLineGetRawValue (Package, 3);
- ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE);
- Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate1) : NULL;
- ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE);
- Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate2) : NULL;
- ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE);
- Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate3) : NULL;
+ if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE))) {
+ Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate1) : NULL;
+ }
+
+ if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE))) {
+ Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate2) : NULL;
+ }
+
+ if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE))) {
+ Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate3) : NULL;
+ }
if ((Param1 != NULL) && (Handle1 == NULL)) {
ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE), gShellDriver1HiiHandle, L"disconnect", Param1);
diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
index c645c9fd6882..8f70d6b6af39 100644
--- a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
+++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
@@ -438,25 +438,22 @@ ShellCommandRunDrvDiag (
ControllerHandleStr = ShellCommandLineGetRawValue (Package, 2);
ChildHandleStr = ShellCommandLineGetRawValue (Package, 3);
- if (DriverHandleStr == NULL) {
- Handle1 = NULL;
- } else {
- ShellConvertStringToUint64 (DriverHandleStr, &Intermediate, TRUE, FALSE);
+ if ((DriverHandleStr != NULL) && ShellConvertStringToUint64 (DriverHandleStr, &Intermediate, TRUE, FALSE)) {
Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
+ Handle1 = NULL;
}
- if (ControllerHandleStr == NULL) {
- Handle2 = NULL;
- } else {
- ShellConvertStringToUint64 (ControllerHandleStr, &Intermediate, TRUE, FALSE);
+ if ((ControllerHandleStr != NULL) && ShellConvertStringToUint64 (ControllerHandleStr, &Intermediate, TRUE, FALSE)) {
Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
+ Handle2 = NULL;
}
- if (ChildHandleStr == NULL) {
- Handle3 = NULL;
- } else {
- ShellConvertStringToUint64 (ChildHandleStr, &Intermediate, TRUE, FALSE);
+ if ((ChildHandleStr != NULL) && ShellConvertStringToUint64 (ChildHandleStr, &Intermediate, TRUE, FALSE)) {
Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
+ } else {
+ Handle3 = NULL;
}
Status = DoDiagnostics (
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (8 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 09/12] ShellPkg: " Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 2:04 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 11/12] .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 12/12] .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries Michael Kubacki
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Erich McMillan, Michael D Kinney, Michael Kubacki,
Rahul Kumar, Ray Ni
From: Michael Kubacki <michael.kubacki@microsoft.com>
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
Cc: Eric Dong <eric.dong@intel.com>
Cc: Erich McMillan <emcmillan@microsoft.com>
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>
Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
UefiCpuPkg/CpuMpPei/CpuBist.c | 8 +++++++-
UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++++++-
UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++-
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c
index 7dc93cd784d4..122808139b87 100644
--- a/UefiCpuPkg/CpuMpPei/CpuBist.c
+++ b/UefiCpuPkg/CpuMpPei/CpuBist.c
@@ -175,7 +175,13 @@ CollectBistDataFromPpi (
EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2;
EFI_SEC_PLATFORM_INFORMATION_CPU *CpuInstanceInHob;
- MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
+ Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
+ ASSERT_EFI_ERROR (Status);
+
+ if (EFI_ERROR (Status)) {
+ NumberOfProcessors = 1u;
+ NumberOfEnabledProcessors = 1u;
+ }
BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) +
sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors;
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index e7f1fe9f426c..a84304273168 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers (
return;
}
- MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
+ Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
+ ASSERT_EFI_ERROR (Status);
+
+ if (EFI_ERROR (Status)) {
+ NumberOfProcessors = 1u;
+ }
+
SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
ASSERT (SwitchStackData != NULL);
ZeroMem (SwitchStackData, NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
index 135422225340..1322fcb77f28 100644
--- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
+++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
@@ -538,6 +538,7 @@ SetupStackGuardPage (
UINTN NumberOfProcessors;
UINTN Bsp;
UINTN Index;
+ EFI_STATUS Status;
//
// One extra page at the bottom of the stack is needed for Guard page.
@@ -547,7 +548,13 @@ SetupStackGuardPage (
ASSERT (FALSE);
}
- MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
+ Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
+ ASSERT_EFI_ERROR (Status);
+
+ if (EFI_ERROR (Status)) {
+ NumberOfProcessors = 1u;
+ }
+
MpInitLibWhoAmI (&Bsp);
for (Index = 0; Index < NumberOfProcessors; ++Index) {
StackBase = 0;
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 11/12] .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (9 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 10/12] UefiCpuPkg: " Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 2:05 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 12/12] .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries Michael Kubacki
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel; +Cc: Sean Brogan, Michael Kubacki, Michael D Kinney
From: Michael Kubacki <michael.kubacki@microsoft.com>
The previous commits fixed issues with these queries across various
packages. Now that those are resolved, enable the queries in the
edk2 query set so regressions can be found in the future.
Enables:
1. cpp/conditionallyuninitializedvariable
- CWE: https://cwe.mitre.org/data/definitions/457.html
- @name Conditionally uninitialized variable
- @description An initialization function is used to initialize a
local variable, but the returned status code is
not checked. The variable may be left in an
uninitialized state, and reading the variable may
result in undefined behavior.
- @kind problem
- @problem.severity warning
- @security-severity 7.8
- @id cpp/conditionally-uninitialized-variable
- @tags security
- external/cwe/cwe-457
2. cpp/pointer-overflow-check
- CWE: https://cwe.mitre.org/data/definitions/758.html
- @name Pointer overflow check
- @description Adding a value to a pointer to check if it
overflows relies on undefined behavior and
may lead to memory corruption.
- @kind problem
- @problem.severity error
- @security-severity 2.1
- @precision high
- @id cpp/pointer-overflow-check
- @tags reliability
- security
- external/cwe/cwe-758
3. cpp/potential-buffer-overflow
- CWE: https://cwe.mitre.org/data/definitions/676.html
- @name Potential buffer overflow
- @description Using a library function that does not check
buffer bounds requires the surrounding program
to be very carefully written to avoid buffer
overflows.
- @kind problem
- @id cpp/potential-buffer-overflow
- @problem.severity warning
- @security-severity 10.0
- @tags reliability
- security
- external/cwe/cwe-676
- @deprecated This query is deprecated, use
Potentially overrunning write
(`cpp/overrunning-write`) and
Potentially overrunning write with float to string
conversion
(`cpp/overrunning-write-with-float`) instead.
Note that cpp/potential-buffer-overflow is deprecated. This query
will be updated to the succeeding queries in the next commit. The
query is used in this commit to show that we considered and tested
the query in history.
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
.github/codeql/edk2.qls | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/.github/codeql/edk2.qls b/.github/codeql/edk2.qls
index ef9aae790f5f..dc2d87764e93 100644
--- a/.github/codeql/edk2.qls
+++ b/.github/codeql/edk2.qls
@@ -8,7 +8,14 @@
# Enable individual queries below.
+- include:
+ id: cpp/conditionallyuninitializedvariable
- include:
id: cpp/infinite-loop-with-unsatisfiable-exit-condition
- include:
id: cpp/overflow-buffer
+- include:
+ id: cpp/pointer-overflow-check
+- include:
+ id: cpp/potential-buffer-overflow
+
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v1 12/12] .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
` (10 preceding siblings ...)
2022-11-09 17:32 ` [PATCH v1 11/12] .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries Michael Kubacki
@ 2022-11-09 17:32 ` Michael Kubacki
2022-11-24 2:06 ` Michael D Kinney
11 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-09 17:32 UTC (permalink / raw)
To: devel; +Cc: Sean Brogan, Michael Kubacki, Michael D Kinney
From: Michael Kubacki <michael.kubacki@microsoft.com>
As recommended by CodeQL this change replaces
cpp/potential-buffer-overflow with cpp/overrunning-write-with-float
and cpp/overrunning-write.
Enables:
1. cpp/overrunning-write
- @name Likely overrunning write
- @description Buffer write operations that do not control the length
data written may overflow
- @kind problem
- @problem.severity error
- @security-severity 9.3
- @precision high
- @id cpp/very-likely-overrunning-write
- @tags reliability
- security
- external/cwe/cwe-120
- external/cwe/cwe-787
- external/cwe/cwe-805
2. cpp/overrunning-write-with-float
- @name Potentially overrunning write with float to string conversion
- @description Buffer write operations that do not control the length
of data written may overflow when floating point inputs
take extreme values.
- @kind problem
- @problem.severity error
- @security-severity 9.3
- @precision medium
- @id cpp/overrunning-write-with-float
- @tags reliability
- security
- external/cwe/cwe-120
- external/cwe/cwe-787
- external/cwe/cwe-805
3. cpp/very-likely-overrunning-write
- @name Likely overrunning write
- @description Buffer write operations that do not control the length
of data written may overflow
- @kind problem
- @problem.severity error
- @security-severity 9.3
- @precision high
- @id cpp/very-likely-overrunning-write
- @tags reliability
- security
- external/cwe/cwe-120
- external/cwe/cwe-787
- external/cwe/cwe-805
- CWEs:
- https://cwe.mitre.org/data/definitions/120.html
- https://cwe.mitre.org/data/definitions/787.html
- https://cwe.mitre.org/data/definitions/805.html
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
.github/codeql/edk2.qls | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/.github/codeql/edk2.qls b/.github/codeql/edk2.qls
index dc2d87764e93..9bea9ba01f24 100644
--- a/.github/codeql/edk2.qls
+++ b/.github/codeql/edk2.qls
@@ -14,8 +14,11 @@
id: cpp/infinite-loop-with-unsatisfiable-exit-condition
- include:
id: cpp/overflow-buffer
+- include:
+ id: cpp/overrunning-write
+- include:
+ id: cpp/overrunning-write-with-float
- include:
id: cpp/pointer-overflow-check
- include:
- id: cpp/potential-buffer-overflow
-
+ id: cpp/very-likely-overrunning-write
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
2022-11-09 17:32 ` [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts Michael Kubacki
@ 2022-11-24 1:28 ` Michael D Kinney
2022-11-24 1:46 ` Michael Kubacki
0 siblings, 1 reply; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 1:28 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com,
Kinney, Michael D
Cc: Bi, Dandan, Erich McMillan, Wang, Jian J, Gao, Liming, Zeng, Star,
Gao, Zhichao, Liu, Zhiguang, Kubacki, Michael
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@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Michael Kubacki <mikuback@linux.microsoft.com>; Zeng, Star <star.zeng@intel.com>; Gao,
> Zhichao <zhichao.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>
> Subject: [edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
>
> From: Erich McMillan <emcmillan@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@intel.com>
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Co-authored-by: Michael Kubacki <michael.kubacki@microsoft.com>
> Signed-off-by: Erich McMillan <emcmillan@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@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1 02/12] BaseTools/PatchCheck.py: Add PCCTS to tab exemption list
2022-11-09 17:32 ` [PATCH v1 02/12] BaseTools/PatchCheck.py: Add PCCTS to tab exemption list Michael Kubacki
@ 2022-11-24 1:30 ` Michael D Kinney
0 siblings, 0 replies; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 1:30 UTC (permalink / raw)
To: mikuback@linux.microsoft.com, devel@edk2.groups.io,
Kinney, Michael D
Cc: Feng, Bob C, Gao, Liming, Sean Brogan, Chen, Christine
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel@edk2.groups.io
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Chen, Christine <yuwei.chen@intel.com>
> Subject: [PATCH v1 02/12] BaseTools/PatchCheck.py: Add PCCTS to tab exemption list
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Purdue Compiler Construction Tool Set (PCCTS) source code was copied/
> pasted into BaseTools/Source/C/VfrCompile/Pccts/.
>
> The code contains tab characters instead of spaces.
>
> PatchCheck.py gives an error on modifications to files that
> contain tabs.
>
> This change adds that directory to the pre-existing list of
> directories in which tab checks are ignored in PatchCheck.py.
>
> The goal of my upcoming change there is not to mix tabs and spaces
> but to fix a bug while preserving its current formatting characters.
>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> BaseTools/Scripts/PatchCheck.py | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 475b3a8c27d9..a02fdb470ee2 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -384,7 +384,9 @@ class GitDiffCheck:
> self.force_crlf = False
> self.force_notabs = False
> if os.path.basename(self.filename) == 'GNUmakefile' or \
> - os.path.basename(self.filename) == 'Makefile':
> + os.path.basename(self.filename) == 'Makefile' or \
> + self.filename.startswith(
> + 'BaseTools/Source/C/VfrCompile/Pccts/'):
> self.force_notabs = False
> elif len(line.rstrip()) != 0:
> self.format_error("didn't find diff command")
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites
2022-11-09 17:32 ` [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites Michael Kubacki
@ 2022-11-24 1:32 ` Michael D Kinney
0 siblings, 0 replies; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 1:32 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com,
Kinney, Michael D
Cc: Feng, Bob C, Gao, Liming, Sean Brogan, Chen, Christine
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> -----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: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Chen, Christine <yuwei.chen@intel.com>
> Subject: [edk2-devel] [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> While more portable methods exist to handle these cases, this change
> does not attempt to do more than fix the immediate problem and
> follow the conventions already established in this code.
>
> `snprintf()` is introduced as the minimum improvement apart from
> making the buffers larger.
>
> Fixes the following CodeQL alerts:
>
> 1. Failure on line 2339 in
> BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
>
> - Type: Potentially overrunning write
> - Severity: Critical
> - Problem: This 'call to sprintf' operation requires 17 bytes but
> the destination is only 16 bytes.
>
> 2. Failure on line 2341 in
> BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
>
> - Type: Potentially overrunning write
> - Severity: Critical
> - Problem: This 'call to sprintf' operation requires 17 bytes but
> the destination is only 16 bytes.
>
> 3. Failure on line 1309 in
> BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
>
> - Type: Potentially overrunning write
> - Severity: Critical
> - Problem: This 'call to sprintf' operation requires 25 bytes but
> the destination is only 20 bytes.
>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c | 10 +++++-----
> BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c | 4 ++--
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c b/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
> index 8e41239f4751..33d9cac4c7de 100644
> --- a/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
> +++ b/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
> @@ -2331,14 +2331,14 @@ TokNode *p;
> set_nameErrSet = bufErrSet; /* MR23 */
> }
> else { /* wild card */
> - static char buf[sizeof("zzerr")+10];
> - static char bufErrSet[sizeof("zzerr")+10];
> + static char buf[sizeof("zzerr")+11];
> + static char bufErrSet[sizeof("zzerr")+11];
> int n = DefErrSet( &b, 0, NULL );
> int nErrSet = DefErrSetWithSuffix(0, &bErrSet, 1, NULL, "_set");
> - if ( GenCC ) sprintf(buf, "err%d", n);
> - else sprintf(buf, "zzerr%d", n);
> + if ( GenCC ) snprintf(buf, 11, "err%d", n);
> + else snprintf(buf, 11, "zzerr%d", n);
> if ( GenCC ) sprintf(bufErrSet, "err%d", nErrSet);
> - else sprintf(bufErrSet, "zzerr%d", nErrSet);
> + else snprintf(bufErrSet, 11, "zzerr%d", nErrSet);
> set_name = buf;
> set_nameErrSet = bufErrSet;
> }
> diff --git a/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c b/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
> index 051ee4ec5d28..488b4b90461c 100644
> --- a/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
> +++ b/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
> @@ -1295,7 +1295,7 @@ int token;
> #endif
> {
> int j;
> - static char imag_name[20];
> + static char imag_name[25];
>
> /* look in all lexclasses for the token */
> if ( TokenString(token) != NULL ) return TokenString(token);
> @@ -1306,7 +1306,7 @@ int token;
> }
>
> if (1) {
> - sprintf(imag_name,"UnknownToken#%d",token); /* MR13 */
> + snprintf(imag_name, 25, "UnknownToken#%d", token); /* MR13 */
> return imag_name; /* MR13 */
> }
>
> --
> 2.28.0.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96149): https://edk2.groups.io/g/devel/message/96149
> Mute This Topic: https://groups.io/mt/94918087/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable
2022-11-09 17:32 ` [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable Michael Kubacki
@ 2022-11-24 1:37 ` Michael D Kinney
2022-11-24 1:47 ` Michael Kubacki
0 siblings, 1 reply; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 1:37 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com,
Kinney, Michael D
Cc: Erich McMillan, Jiang, Guomin, Wang, Jian J, Yao, Jiewen,
Lu, Xiaoyu1
Hi Michael,
Comments 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: Erich McMillan <emcmillan@microsoft.com>; Jiang, Guomin <guomin.jiang@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Lu, Xiaoyu1
> <xiaoyu1.lu@intel.com>
> Subject: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
>
> Checks the return value from `ASN1_get_object()` to verify values
> set by the function are valid.
>
> Note that the function returns literal `0x80`:
> `return (0x80);`
>
> That is used to check the return value is as the case in other areas
> of the code.
>
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Xiaoyu Lu <xiaoyu1.lu@intel.com>
> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c | 21 +++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
> index 2333157e0d17..f867656e888c 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
> @@ -807,6 +807,7 @@ X509GetTBSCert (
> UINT32 Asn1Tag;
> UINT32 ObjClass;
> UINTN Length;
> + UINTN Inf;
>
> //
> // Check input parameters.
> @@ -836,9 +837,9 @@ X509GetTBSCert (
> //
> Temp = Cert;
> Length = 0;
> - ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjClass, (long)CertSize);
> + Inf = ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjClass, (long)CertSize);
>
> - if (Asn1Tag != V_ASN1_SEQUENCE) {
> + if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
> return FALSE;
> }
>
> @@ -848,7 +849,7 @@ X509GetTBSCert (
> //
> // Verify the parsed TBSCertificate is one correct SEQUENCE data.
> //
> - if (Asn1Tag != V_ASN1_SEQUENCE) {
> + if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
> return FALSE;
> }
>
> @@ -1888,18 +1889,20 @@ Asn1GetTag (
> IN UINT32 Tag
> )
> {
> - UINT8 *PtrOld;
> - INT32 ObjTag;
> - INT32 ObjCls;
> - long ObjLength;
> + UINT8 *PtrOld;
> + INT32 ObjTag;
> + INT32 ObjCls;
> + long ObjLength;
> + UINT32 Inf;
>
> //
> // Save Ptr position
> //
> PtrOld = *Ptr;
>
> - ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
> - if ((ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
> + Inf = ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
> + if (!(Inf & 0x80) &&
The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
> + (ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
> (ObjCls == (INT32)(Tag & CRYPTO_ASN1_TAG_CLASS_MASK)))
> {
> *Length = (UINTN)ObjLength;
> --
> 2.28.0.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96150): https://edk2.groups.io/g/devel/message/96150
> Mute This Topic: https://groups.io/mt/94918089/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
2022-11-24 1:28 ` [edk2-devel] " Michael D Kinney
@ 2022-11-24 1:46 ` Michael Kubacki
0 siblings, 0 replies; 33+ messages in thread
From: Michael Kubacki @ 2022-11-24 1:46 UTC (permalink / raw)
To: devel, michael.d.kinney
Cc: Bi, Dandan, Erich McMillan, Wang, Jian J, Gao, Liming, Zeng, Star,
Gao, Zhichao, Liu, Zhiguang, Kubacki, Michael
Thanks Mike. Erich, I'll include the update for this in the v2 series.
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@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao,
>> Liming <gaoliming@byosoft.com.cn>; Michael Kubacki <mikuback@linux.microsoft.com>; Zeng, Star <star.zeng@intel.com>; Gao,
>> Zhichao <zhichao.gao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>
>> Subject: [edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
>>
>> From: Erich McMillan <emcmillan@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@intel.com>
>> Cc: Erich McMillan <emcmillan@microsoft.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Cc: Zhichao Gao <zhichao.gao@intel.com>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Co-authored-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> Signed-off-by: Erich McMillan <emcmillan@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@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
>> -=-=-=-=-=-=
>>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable
2022-11-24 1:37 ` [edk2-devel] " Michael D Kinney
@ 2022-11-24 1:47 ` Michael Kubacki
0 siblings, 0 replies; 33+ messages in thread
From: Michael Kubacki @ 2022-11-24 1:47 UTC (permalink / raw)
To: devel, michael.d.kinney
Cc: Erich McMillan, Jiang, Guomin, Wang, Jian J, Yao, Jiewen,
Lu, Xiaoyu1
Thanks. I'll make the suggested change in the v2 series.
On 11/23/2022 8:37 PM, Michael D Kinney wrote:
> Hi Michael,
>
> Comments 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: Erich McMillan <emcmillan@microsoft.com>; Jiang, Guomin <guomin.jiang@intel.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Lu, Xiaoyu1
>> <xiaoyu1.lu@intel.com>
>> Subject: [edk2-devel] [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Fixes CodeQL alerts for CWE-457:
>> https://cwe.mitre.org/data/definitions/457.html
>>
>> Checks the return value from `ASN1_get_object()` to verify values
>> set by the function are valid.
>>
>> Note that the function returns literal `0x80`:
>> `return (0x80);`
>>
>> That is used to check the return value is as the case in other areas
>> of the code.
>>
>> Cc: Erich McMillan <emcmillan@microsoft.com>
>> Cc: Guomin Jiang <guomin.jiang@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Cc: Xiaoyu Lu <xiaoyu1.lu@intel.com>
>> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>> CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c | 21 +++++++++++---------
>> 1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
>> index 2333157e0d17..f867656e888c 100644
>> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
>> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
>> @@ -807,6 +807,7 @@ X509GetTBSCert (
>> UINT32 Asn1Tag;
>> UINT32 ObjClass;
>> UINTN Length;
>> + UINTN Inf;
>>
>> //
>> // Check input parameters.
>> @@ -836,9 +837,9 @@ X509GetTBSCert (
>> //
>> Temp = Cert;
>> Length = 0;
>> - ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjClass, (long)CertSize);
>> + Inf = ASN1_get_object (&Temp, (long *)&Length, (int *)&Asn1Tag, (int *)&ObjClass, (long)CertSize);
>>
>> - if (Asn1Tag != V_ASN1_SEQUENCE) {
>> + if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
>
> The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
> I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
>
>> return FALSE;
>> }
>>
>> @@ -848,7 +849,7 @@ X509GetTBSCert (
>> //
>> // Verify the parsed TBSCertificate is one correct SEQUENCE data.
>> //
>> - if (Asn1Tag != V_ASN1_SEQUENCE) {
>> + if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
>
> The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
> I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
>
>> return FALSE;
>> }
>>
>> @@ -1888,18 +1889,20 @@ Asn1GetTag (
>> IN UINT32 Tag
>> )
>> {
>> - UINT8 *PtrOld;
>> - INT32 ObjTag;
>> - INT32 ObjCls;
>> - long ObjLength;
>> + UINT8 *PtrOld;
>> + INT32 ObjTag;
>> + INT32 ObjCls;
>> + long ObjLength;
>> + UINT32 Inf;
>>
>> //
>> // Save Ptr position
>> //
>> PtrOld = *Ptr;
>>
>> - ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
>> - if ((ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
>> + Inf = ASN1_get_object ((CONST UINT8 **)Ptr, &ObjLength, &ObjTag, &ObjCls, (INT32)(End - (*Ptr)));
>> + if (!(Inf & 0x80) &&
>
> The result of bitwise operation (Inf & 0x80) is a value, not a BOOLEAN.
> I think the more correct way to do this check is ((Inf & 0x80) == 0x00).
>
>> + (ObjTag == (INT32)(Tag & CRYPTO_ASN1_TAG_VALUE_MASK)) &&
>> (ObjCls == (INT32)(Tag & CRYPTO_ASN1_TAG_CLASS_MASK)))
>> {
>> *Length = (UINTN)ObjLength;
>> --
>> 2.28.0.windows.1
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#96150): https://edk2.groups.io/g/devel/message/96150
>> Mute This Topic: https://groups.io/mt/94918089/1643496
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
>> -=-=-=-=-=-=
>>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
2022-11-09 17:32 ` [PATCH v1 06/12] MdePkg: " Michael Kubacki
@ 2022-11-24 1:53 ` Michael D Kinney
2022-11-24 1:59 ` Michael Kubacki
0 siblings, 1 reply; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 1:53 UTC (permalink / raw)
To: mikuback@linux.microsoft.com, devel@edk2.groups.io,
Kinney, Michael D
Cc: Erich McMillan, Gao, Liming, Liu, Zhiguang
Hi Michael,
Comments below.
Mike
> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel@edk2.groups.io
> Cc: Erich McMillan <emcmillan@microsoft.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
>
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> MdePkg/Library/BaseLib/String.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
> index 98e6d31463e0..0ff0454b9d98 100644
> --- a/MdePkg/Library/BaseLib/String.c
> +++ b/MdePkg/Library/BaseLib/String.c
> @@ -6,6 +6,7 @@
>
> **/
>
> +#include <Uefi/UefiBaseType.h>
Why is this change needed?
I think this should be <Base.h> for a library of type BASE
and BaseLibInternals.h includes <Base.h>. I see the use
of EFI_ERROR() in changes below. The BASE lib macro to use
that does not require UEFI types is the RETURN_ERROR() macro.
> #include "BaseLibInternals.h"
>
> /**
> @@ -408,7 +409,8 @@ StrDecimalToUintn (
> {
> UINTN Result;
>
> - StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result);
> + Result = !EFI_ERROR (StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINTN;> +
I think RETURN_ERROR() should be used instead of EFI_ERROR()), and putting
this on a single line makes it hard to understand. Perhaps the following
style:
if (RETURN_ERROR (StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result))) {
return MAX_UINTN;
}
return Result;
I would also add more details to the commit message. The current form would
return an undefined Result value from the stack if StrDecimalToUintnS()
returned an error. This change now consistently returns MAX_UINTN.
This may impact the caller of this API.
These comments apply to the similar changes below.
> return Result;
> }
>
> @@ -454,7 +456,8 @@ StrDecimalToUint64 (
> {
> UINT64 Result;
>
> - StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result);
> + Result = !EFI_ERROR (StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINT64;
> +
> return Result;
> }
>
> @@ -501,7 +504,8 @@ StrHexToUintn (
> {
> UINTN Result;
>
> - StrHexToUintnS (String, (CHAR16 **)NULL, &Result);
> + Result = !EFI_ERROR (StrHexToUintnS (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINTN;
> +
> return Result;
> }
>
> @@ -548,7 +552,7 @@ StrHexToUint64 (
> {
> UINT64 Result;
>
> - StrHexToUint64S (String, (CHAR16 **)NULL, &Result);
> + Result = !EFI_ERROR (StrHexToUint64S (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINT64;
> return Result;
> }
>
> @@ -989,7 +993,7 @@ AsciiStrDecimalToUintn (
> {
> UINTN Result;
>
> - AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result);
> + Result = !EFI_ERROR (AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINTN;
> return Result;
> }
>
> @@ -1031,7 +1035,7 @@ AsciiStrDecimalToUint64 (
> {
> UINT64 Result;
>
> - AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result);
> + Result = !EFI_ERROR (AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINT64;
> return Result;
> }
>
> @@ -1077,7 +1081,7 @@ AsciiStrHexToUintn (
> {
> UINTN Result;
>
> - AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result);
> + Result = !EFI_ERROR (AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINTN;
> return Result;
> }
>
> @@ -1123,7 +1127,7 @@ AsciiStrHexToUint64 (
> {
> UINT64 Result;
>
> - AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result);
> + Result = !EFI_ERROR (AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINT64;
> return Result;
> }
>
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
2022-11-24 1:53 ` Michael D Kinney
@ 2022-11-24 1:59 ` Michael Kubacki
0 siblings, 0 replies; 33+ messages in thread
From: Michael Kubacki @ 2022-11-24 1:59 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Erich McMillan, Gao, Liming, Liu, Zhiguang
Thanks. Responses inline.
On 11/23/2022 8:53 PM, Kinney, Michael D wrote:
> Hi Michael,
>
> Comments below.
>
> Mike
>
>> -----Original Message-----
>> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
>> Sent: Wednesday, November 9, 2022 9:33 AM
>> To: devel@edk2.groups.io
>> Cc: Erich McMillan <emcmillan@microsoft.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
>> Subject: [PATCH v1 06/12] MdePkg: Fix conditionally uninitialized variables
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Fixes CodeQL alerts for CWE-457:
>> https://cwe.mitre.org/data/definitions/457.html
>>
>> Cc: Erich McMillan <emcmillan@microsoft.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>> MdePkg/Library/BaseLib/String.c | 20 ++++++++++++--------
>> 1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
>> index 98e6d31463e0..0ff0454b9d98 100644
>> --- a/MdePkg/Library/BaseLib/String.c
>> +++ b/MdePkg/Library/BaseLib/String.c
>> @@ -6,6 +6,7 @@
>>
>> **/
>>
>> +#include <Uefi/UefiBaseType.h>
>
> Why is this change needed?
>
> I think this should be <Base.h> for a library of type BASE
> and BaseLibInternals.h includes <Base.h>. I see the use
> of EFI_ERROR() in changes below. The BASE lib macro to use
> that does not require UEFI types is the RETURN_ERROR() macro.
>
I'll double check it.
>> #include "BaseLibInternals.h"
>>
>> /**
>> @@ -408,7 +409,8 @@ StrDecimalToUintn (
>> {
>> UINTN Result;
>>
>> - StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result);
>> + Result = !EFI_ERROR (StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINTN;> +
>
> I think RETURN_ERROR() should be used instead of EFI_ERROR()), and putting
> this on a single line makes it hard to understand. Perhaps the following
> style:
>
>
> if (RETURN_ERROR (StrDecimalToUintnS (String, (CHAR16 **)NULL, &Result))) {
> return MAX_UINTN;
> }
> return Result;
>
> I would also add more details to the commit message. The current form would
> return an undefined Result value from the stack if StrDecimalToUintnS()
> returned an error. This change now consistently returns MAX_UINTN.
> This may impact the caller of this API.
>
> These comments apply to the similar changes below.
>
I agree the suggested style is easier to read.
I will also add a note about the change in return value behavior.
>> return Result;
>> }
>>
>> @@ -454,7 +456,8 @@ StrDecimalToUint64 (
>> {
>> UINT64 Result;
>>
>> - StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result);
>> + Result = !EFI_ERROR (StrDecimalToUint64S (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINT64;
>> +
>> return Result;
>> }
>>
>> @@ -501,7 +504,8 @@ StrHexToUintn (
>> {
>> UINTN Result;
>>
>> - StrHexToUintnS (String, (CHAR16 **)NULL, &Result);
>> + Result = !EFI_ERROR (StrHexToUintnS (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINTN;
>> +
>> return Result;
>> }
>>
>> @@ -548,7 +552,7 @@ StrHexToUint64 (
>> {
>> UINT64 Result;
>>
>> - StrHexToUint64S (String, (CHAR16 **)NULL, &Result);
>> + Result = !EFI_ERROR (StrHexToUint64S (String, (CHAR16 **)NULL, &Result)) ? Result : MAX_UINT64;
>> return Result;
>> }
>>
>> @@ -989,7 +993,7 @@ AsciiStrDecimalToUintn (
>> {
>> UINTN Result;
>>
>> - AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result);
>> + Result = !EFI_ERROR (AsciiStrDecimalToUintnS (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINTN;
>> return Result;
>> }
>>
>> @@ -1031,7 +1035,7 @@ AsciiStrDecimalToUint64 (
>> {
>> UINT64 Result;
>>
>> - AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result);
>> + Result = !EFI_ERROR (AsciiStrDecimalToUint64S (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINT64;
>> return Result;
>> }
>>
>> @@ -1077,7 +1081,7 @@ AsciiStrHexToUintn (
>> {
>> UINTN Result;
>>
>> - AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result);
>> + Result = !EFI_ERROR (AsciiStrHexToUintnS (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINTN;
>> return Result;
>> }
>>
>> @@ -1123,7 +1127,7 @@ AsciiStrHexToUint64 (
>> {
>> UINT64 Result;
>>
>> - AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result);
>> + Result = !EFI_ERROR (AsciiStrHexToUint64S (String, (CHAR8 **)NULL, &Result)) ? Result : MAX_UINT64;
>> return Result;
>> }
>>
>> --
>> 2.28.0.windows.1
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1 07/12] NetworkPkg: Fix conditionally uninitialized variables
2022-11-09 17:32 ` [PATCH v1 07/12] NetworkPkg: " Michael Kubacki
@ 2022-11-24 1:59 ` Michael D Kinney
0 siblings, 0 replies; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 1:59 UTC (permalink / raw)
To: mikuback@linux.microsoft.com, devel@edk2.groups.io,
Kinney, Michael D
Cc: Erich McMillan, Wu, Jiaxin, Maciej Rabeda, Siyuan Fu
Hi Michael,
Comment below.
Mike
> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel@edk2.groups.io
> Cc: Erich McMillan <emcmillan@microsoft.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Maciej Rabeda
> <maciej.rabeda@linux.intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>; Siyuan Fu <siyuan.fu@intel.com>
> Subject: [PATCH v1 07/12] NetworkPkg: Fix conditionally uninitialized variables
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
>
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c | 2 +-
> NetworkPkg/TcpDxe/TcpInput.c | 3 +++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> index 6a5d78629bb3..71c98abc820e 100644
> --- a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -753,7 +753,7 @@ HttpUrlGetPort (
>
> Status = AsciiStrDecimalToUintnS (Url + Parser->FieldData[HTTP_URI_FIELD_PORT].Offset, (CHAR8 **)NULL, &Data);
>
> - if (Data > HTTP_URI_PORT_MAX_NUM) {
> + if (!EFI_ERROR (Status) && (Data > HTTP_URI_PORT_MAX_NUM)) {
I do not think this logic change is correct. If the string can not be
converted to a value, then Status will be an error. If that happens,
then the value of Data is undefined. An error should be returned if
Status is an error or Data is out of range.
if (EFI_ERROR (Status) || (Data > HTTP_URI_PORT_MAX_NUM)) {
> Status = EFI_INVALID_PARAMETER;
> goto ON_EXIT;
> }
> diff --git a/NetworkPkg/TcpDxe/TcpInput.c b/NetworkPkg/TcpDxe/TcpInput.c
> index fb1aa827f8ba..7b329be64dfe 100644
> --- a/NetworkPkg/TcpDxe/TcpInput.c
> +++ b/NetworkPkg/TcpDxe/TcpInput.c
> @@ -1570,6 +1570,9 @@ TcpIcmpInput (
> BOOLEAN IcmpErrIsHard;
> BOOLEAN IcmpErrNotify;
>
> + IcmpErrIsHard = FALSE;
> + IcmpErrNotify = FALSE;
> +
> if (Nbuf->TotalSize < sizeof (TCP_HEAD)) {
> goto CLEAN_EXIT;
> }
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1 08/12] PcAtChipsetPkg: Fix conditionally uninitialized variables
2022-11-09 17:32 ` [PATCH v1 08/12] PcAtChipsetPkg: " Michael Kubacki
@ 2022-11-24 2:00 ` Michael D Kinney
2022-11-24 5:01 ` Ni, Ray
0 siblings, 1 reply; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 2:00 UTC (permalink / raw)
To: mikuback@linux.microsoft.com, devel@edk2.groups.io,
Kinney, Michael D
Cc: Erich McMillan, Ni, Ray
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel@edk2.groups.io
> Cc: Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v1 08/12] PcAtChipsetPkg: Fix conditionally uninitialized variables
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
>
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> index 9242a2e82600..57ea3153aa6b 100644
> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> @@ -344,7 +344,7 @@ PcRtcInit (
> // so we can use them to get and set wakeup time.
> //
> Status = PcRtcGetWakeupTime (&Enabled, &Pending, &Time, Global);
> - if ((Enabled) || (!EFI_ERROR (Status))) {
> + if ((!EFI_ERROR (Status)) || (Enabled)) {
> return EFI_SUCCESS;
> }
>
> @@ -836,8 +836,11 @@ PcRtcSetWakeupTime (
> //
> // Just support set alarm time within 24 hours
> //
> - PcRtcGetTime (&RtcTime, &Capabilities, Global);
> - Status = RtcTimeFieldsValid (&RtcTime);
> + Status = PcRtcGetTime (&RtcTime, &Capabilities, Global);
> + if (!EFI_ERROR (Status)) {
> + Status = RtcTimeFieldsValid (&RtcTime);
> + }
> +
> if (EFI_ERROR (Status)) {
> return EFI_DEVICE_ERROR;
> }
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
2022-11-09 17:32 ` [PATCH v1 10/12] UefiCpuPkg: " Michael Kubacki
@ 2022-11-24 2:04 ` Michael D Kinney
2022-11-24 2:14 ` Michael Kubacki
0 siblings, 1 reply; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 2:04 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com,
Kinney, Michael D
Cc: Dong, Eric, Erich McMillan, Kumar, Rahul R, Ni, Ray
Hi Michael,
comments 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: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Erich McMillan <emcmillan@microsoft.com>
> 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>
> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> UefiCpuPkg/CpuMpPei/CpuBist.c | 8 +++++++-
> UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++++++-
> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++-
> 3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c
> index 7dc93cd784d4..122808139b87 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c
> @@ -175,7 +175,13 @@ CollectBistDataFromPpi (
> EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2;
> EFI_SEC_PLATFORM_INFORMATION_CPU *CpuInstanceInHob;
>
> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
> + ASSERT_EFI_ERROR (Status);
I think this ASSERT() should be removed. The added error status check looks correct.
> +
> + if (EFI_ERROR (Status)) {
> + NumberOfProcessors = 1u;
> + NumberOfEnabledProcessors = 1u;
> + }
>
> BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) +
> sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors;
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> index e7f1fe9f426c..a84304273168 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers (
> return;
> }
>
> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> + ASSERT_EFI_ERROR (Status);
I think this ASSERT() should be removed. The added error status check looks correct.
> +
> + if (EFI_ERROR (Status)) {
> + NumberOfProcessors = 1u;
> + }
> +
> SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
> ASSERT (SwitchStackData != NULL);
> ZeroMem (SwitchStackData, NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index 135422225340..1322fcb77f28 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -538,6 +538,7 @@ SetupStackGuardPage (
> UINTN NumberOfProcessors;
> UINTN Bsp;
> UINTN Index;
> + EFI_STATUS Status;
>
> //
> // One extra page at the bottom of the stack is needed for Guard page.
> @@ -547,7 +548,13 @@ SetupStackGuardPage (
> ASSERT (FALSE);
> }
>
> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> + ASSERT_EFI_ERROR (Status);
I think this ASSERT() should be removed. The added error status check looks correct.
> +
> + if (EFI_ERROR (Status)) {
> + NumberOfProcessors = 1u;
> + }
> +
> MpInitLibWhoAmI (&Bsp);
> for (Index = 0; Index < NumberOfProcessors; ++Index) {
> StackBase = 0;
> --
> 2.28.0.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96156): https://edk2.groups.io/g/devel/message/96156
> Mute This Topic: https://groups.io/mt/94918104/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 11/12] .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries
2022-11-09 17:32 ` [PATCH v1 11/12] .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries Michael Kubacki
@ 2022-11-24 2:05 ` Michael D Kinney
0 siblings, 0 replies; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 2:05 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com,
Kinney, Michael D
Cc: Sean Brogan
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> -----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: Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [edk2-devel] [PATCH v1 11/12] .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> The previous commits fixed issues with these queries across various
> packages. Now that those are resolved, enable the queries in the
> edk2 query set so regressions can be found in the future.
>
> Enables:
>
> 1. cpp/conditionallyuninitializedvariable
> - CWE: https://cwe.mitre.org/data/definitions/457.html
> - @name Conditionally uninitialized variable
> - @description An initialization function is used to initialize a
> local variable, but the returned status code is
> not checked. The variable may be left in an
> uninitialized state, and reading the variable may
> result in undefined behavior.
> - @kind problem
> - @problem.severity warning
> - @security-severity 7.8
> - @id cpp/conditionally-uninitialized-variable
> - @tags security
> - external/cwe/cwe-457
> 2. cpp/pointer-overflow-check
> - CWE: https://cwe.mitre.org/data/definitions/758.html
> - @name Pointer overflow check
> - @description Adding a value to a pointer to check if it
> overflows relies on undefined behavior and
> may lead to memory corruption.
> - @kind problem
> - @problem.severity error
> - @security-severity 2.1
> - @precision high
> - @id cpp/pointer-overflow-check
> - @tags reliability
> - security
> - external/cwe/cwe-758
> 3. cpp/potential-buffer-overflow
> - CWE: https://cwe.mitre.org/data/definitions/676.html
> - @name Potential buffer overflow
> - @description Using a library function that does not check
> buffer bounds requires the surrounding program
> to be very carefully written to avoid buffer
> overflows.
> - @kind problem
> - @id cpp/potential-buffer-overflow
> - @problem.severity warning
> - @security-severity 10.0
> - @tags reliability
> - security
> - external/cwe/cwe-676
> - @deprecated This query is deprecated, use
> Potentially overrunning write
> (`cpp/overrunning-write`) and
> Potentially overrunning write with float to string
> conversion
> (`cpp/overrunning-write-with-float`) instead.
>
> Note that cpp/potential-buffer-overflow is deprecated. This query
> will be updated to the succeeding queries in the next commit. The
> query is used in this commit to show that we considered and tested
> the query in history.
>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> .github/codeql/edk2.qls | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/.github/codeql/edk2.qls b/.github/codeql/edk2.qls
> index ef9aae790f5f..dc2d87764e93 100644
> --- a/.github/codeql/edk2.qls
> +++ b/.github/codeql/edk2.qls
> @@ -8,7 +8,14 @@
>
> # Enable individual queries below.
>
> +- include:
> + id: cpp/conditionallyuninitializedvariable
> - include:
> id: cpp/infinite-loop-with-unsatisfiable-exit-condition
> - include:
> id: cpp/overflow-buffer
> +- include:
> + id: cpp/pointer-overflow-check
> +- include:
> + id: cpp/potential-buffer-overflow
> +
> --
> 2.28.0.windows.1
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96157): https://edk2.groups.io/g/devel/message/96157
> Mute This Topic: https://groups.io/mt/94918106/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1 12/12] .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries
2022-11-09 17:32 ` [PATCH v1 12/12] .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries Michael Kubacki
@ 2022-11-24 2:06 ` Michael D Kinney
0 siblings, 0 replies; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 2:06 UTC (permalink / raw)
To: mikuback@linux.microsoft.com, devel@edk2.groups.io,
Kinney, Michael D
Cc: Sean Brogan
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH v1 12/12] .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> As recommended by CodeQL this change replaces
> cpp/potential-buffer-overflow with cpp/overrunning-write-with-float
> and cpp/overrunning-write.
>
> Enables:
>
> 1. cpp/overrunning-write
> - @name Likely overrunning write
> - @description Buffer write operations that do not control the length
> data written may overflow
> - @kind problem
> - @problem.severity error
> - @security-severity 9.3
> - @precision high
> - @id cpp/very-likely-overrunning-write
> - @tags reliability
> - security
> - external/cwe/cwe-120
> - external/cwe/cwe-787
> - external/cwe/cwe-805
> 2. cpp/overrunning-write-with-float
> - @name Potentially overrunning write with float to string conversion
> - @description Buffer write operations that do not control the length
> of data written may overflow when floating point inputs
> take extreme values.
> - @kind problem
> - @problem.severity error
> - @security-severity 9.3
> - @precision medium
> - @id cpp/overrunning-write-with-float
> - @tags reliability
> - security
> - external/cwe/cwe-120
> - external/cwe/cwe-787
> - external/cwe/cwe-805
> 3. cpp/very-likely-overrunning-write
> - @name Likely overrunning write
> - @description Buffer write operations that do not control the length
> of data written may overflow
> - @kind problem
> - @problem.severity error
> - @security-severity 9.3
> - @precision high
> - @id cpp/very-likely-overrunning-write
> - @tags reliability
> - security
> - external/cwe/cwe-120
> - external/cwe/cwe-787
> - external/cwe/cwe-805
>
> - CWEs:
> - https://cwe.mitre.org/data/definitions/120.html
> - https://cwe.mitre.org/data/definitions/787.html
> - https://cwe.mitre.org/data/definitions/805.html
>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> .github/codeql/edk2.qls | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/.github/codeql/edk2.qls b/.github/codeql/edk2.qls
> index dc2d87764e93..9bea9ba01f24 100644
> --- a/.github/codeql/edk2.qls
> +++ b/.github/codeql/edk2.qls
> @@ -14,8 +14,11 @@
> id: cpp/infinite-loop-with-unsatisfiable-exit-condition
> - include:
> id: cpp/overflow-buffer
> +- include:
> + id: cpp/overrunning-write
> +- include:
> + id: cpp/overrunning-write-with-float
> - include:
> id: cpp/pointer-overflow-check
> - include:
> - id: cpp/potential-buffer-overflow
> -
> + id: cpp/very-likely-overrunning-write
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
2022-11-24 2:04 ` [edk2-devel] " Michael D Kinney
@ 2022-11-24 2:14 ` Michael Kubacki
2022-11-24 2:31 ` Michael D Kinney
0 siblings, 1 reply; 33+ messages in thread
From: Michael Kubacki @ 2022-11-24 2:14 UTC (permalink / raw)
To: devel, michael.d.kinney
Cc: Dong, Eric, Erich McMillan, Kumar, Rahul R, Ni, Ray
The ASSERT() was added to aid debugging by bringing attention to the
point where the fallback assignment occurs in the error status check block.
Are you suggesting the ASSERT() be removed because of a known case where
it might trigger but the case is expected to return an error? Or, that
is is not necessary in general?
Thanks,
Michael
On 11/23/2022 9:04 PM, Michael D Kinney wrote:
> Hi Michael,
>
> comments 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: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
>> Ni, Ray <ray.ni@intel.com>
>> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Fixes CodeQL alerts for CWE-457:
>> https://cwe.mitre.org/data/definitions/457.html
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Erich McMillan <emcmillan@microsoft.com>
>> 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>
>> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>> UefiCpuPkg/CpuMpPei/CpuBist.c | 8 +++++++-
>> UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++++++-
>> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++-
>> 3 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c
>> index 7dc93cd784d4..122808139b87 100644
>> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c
>> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c
>> @@ -175,7 +175,13 @@ CollectBistDataFromPpi (
>> EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2;
>> EFI_SEC_PLATFORM_INFORMATION_CPU *CpuInstanceInHob;
>>
>> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
>> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
>> + ASSERT_EFI_ERROR (Status);
>
> I think this ASSERT() should be removed. The added error status check looks correct.
>
>> +
>> + if (EFI_ERROR (Status)) {
>> + NumberOfProcessors = 1u;
>> + NumberOfEnabledProcessors = 1u;
>> + }
>>
>> BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) +
>> sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors;
>> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>> index e7f1fe9f426c..a84304273168 100644
>> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers (
>> return;
>> }
>>
>> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>> + ASSERT_EFI_ERROR (Status);
>
> I think this ASSERT() should be removed. The added error status check looks correct.
>
>> +
>> + if (EFI_ERROR (Status)) {
>> + NumberOfProcessors = 1u;
>> + }
>> +
>> SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
>> ASSERT (SwitchStackData != NULL);
>> ZeroMem (SwitchStackData, NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> index 135422225340..1322fcb77f28 100644
>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> @@ -538,6 +538,7 @@ SetupStackGuardPage (
>> UINTN NumberOfProcessors;
>> UINTN Bsp;
>> UINTN Index;
>> + EFI_STATUS Status;
>>
>> //
>> // One extra page at the bottom of the stack is needed for Guard page.
>> @@ -547,7 +548,13 @@ SetupStackGuardPage (
>> ASSERT (FALSE);
>> }
>>
>> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>> + ASSERT_EFI_ERROR (Status);
>
> I think this ASSERT() should be removed. The added error status check looks correct.
>
>> +
>> + if (EFI_ERROR (Status)) {
>> + NumberOfProcessors = 1u;
>> + }
>> +
>> MpInitLibWhoAmI (&Bsp);
>> for (Index = 0; Index < NumberOfProcessors; ++Index) {
>> StackBase = 0;
>> --
>> 2.28.0.windows.1
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#96156): https://edk2.groups.io/g/devel/message/96156
>> Mute This Topic: https://groups.io/mt/94918104/1643496
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
>> -=-=-=-=-=-=
>>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables
2022-11-09 17:32 ` [PATCH v1 09/12] ShellPkg: " Michael Kubacki
@ 2022-11-24 2:19 ` Gao, Zhichao
2022-11-24 2:36 ` [edk2-devel] " Michael Kubacki
0 siblings, 1 reply; 33+ messages in thread
From: Gao, Zhichao @ 2022-11-24 2:19 UTC (permalink / raw)
To: mikuback@linux.microsoft.com, devel@edk2.groups.io
Cc: Erich McMillan, Kinney, Michael D, Ni, Ray
See comments below:
> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Thursday, November 10, 2022 1:33 AM
> To: devel@edk2.groups.io
> Cc: Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
>
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> ShellPkg/Application/Shell/Shell.c | 2 +-
> ShellPkg/Application/Shell/ShellProtocol.c | 4 +-
> 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 ++++----
> 8 files changed, 78 insertions(+), 63 deletions(-)
>
> diff --git a/ShellPkg/Application/Shell/Shell.c
> b/ShellPkg/Application/Shell/Shell.c
> index df00adfdfa5b..86db2f4ebb6e 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -1324,7 +1324,7 @@ DoStartupScript (
> }
>
> Status = RunShellCommand (FileStringPath, &CalleeStatus);
> - if (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE) {
> + if (!EFI_ERROR (Status) &&
> + (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE)) {
Incorrect here. Cannot handle the unsuccess condition. Better to assign the success initial value to Calleestatus and keep the org logic.
> ShellCommandRegisterExit (gEfiShellProtocol->BatchIsActive (),
> (UINT64)CalleeStatus);
> }
>
> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
> b/ShellPkg/Application/Shell/ShellProtocol.c
> index 509eb60e40f4..9183da284fff 100644
> --- a/ShellPkg/Application/Shell/ShellProtocol.c
> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
> @@ -729,8 +729,8 @@ EfiShellGetDeviceName (
> //
> // Now check the parent controller using this as the child.
> //
> - if (DeviceNameToReturn == NULL) {
> - PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle,
> &ParentControllerCount, &ParentControllerBuffer);
> + Status = PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle,
> &ParentControllerCount, &ParentControllerBuffer);
> + if ((DeviceNameToReturn == NULL) && !EFI_ERROR (Status)) {
> for (LoopVar = 0; LoopVar < ParentControllerCount; LoopVar++) {
> PARSE_HANDLE_DATABASE_UEFI_DRIVERS
> (ParentControllerBuffer[LoopVar], &ParentDriverCount,
> &ParentDriverBuffer);
Should we cover above function as well?
Others looks good to me.
Thanks,
Zhichao
> for (HandleCount = 0; HandleCount < ParentDriverCount;
> HandleCount++) { diff --git
> a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
> b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
> index 36cf46fb2c38..4549cbde9b9a 100644
> --- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
> +++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
> @@ -1399,10 +1399,11 @@ ShellCommandCreateInitialMappingsAndPaths (
> CHAR16 *MapName;
> SHELL_MAP_LIST *MapListItem;
>
> - SplitCurDir = NULL;
> - MapName = NULL;
> - MapListItem = NULL;
> - HandleList = NULL;
> + ConsistMappingTable = NULL;
> + SplitCurDir = NULL;
> + MapName = NULL;
> + MapListItem = NULL;
> + HandleList = NULL;
>
> //
> // Reset the static members back to zero @@ -1458,32 +1459,35 @@
> ShellCommandCreateInitialMappingsAndPaths (
> //
> PerformQuickSort (DevicePathList, Count, sizeof
> (EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);
>
> - ShellCommandConsistMappingInitialize (&ConsistMappingTable);
> - //
> - // Assign new Mappings to all...
> - //
> - for (Count = 0; HandleList[Count] != NULL; Count++) {
> + if (!EFI_ERROR (ShellCommandConsistMappingInitialize
> + (&ConsistMappingTable))) {
> //
> - // Get default name first
> + // Assign new Mappings to all...
> //
> - NewDefaultName = ShellCommandCreateNewMappingName
> (MappingTypeFileSystem);
> - ASSERT (NewDefaultName != NULL);
> - Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName,
> DevicePathList[Count], 0, TRUE);
> - ASSERT_EFI_ERROR (Status);
> - FreePool (NewDefaultName);
> -
> - //
> - // Now do consistent name
> - //
> - NewConsistName = ShellCommandConsistMappingGenMappingName
> (DevicePathList[Count], ConsistMappingTable);
> - if (NewConsistName != NULL) {
> - Status = ShellCommandAddMapItemAndUpdatePath
> (NewConsistName, DevicePathList[Count], 0, FALSE);
> + for (Count = 0; HandleList[Count] != NULL; Count++) {
> + //
> + // Get default name first
> + //
> + NewDefaultName = ShellCommandCreateNewMappingName
> (MappingTypeFileSystem);
> + ASSERT (NewDefaultName != NULL);
> + Status = ShellCommandAddMapItemAndUpdatePath
> (NewDefaultName,
> + DevicePathList[Count], 0, TRUE);
> ASSERT_EFI_ERROR (Status);
> - FreePool (NewConsistName);
> + FreePool (NewDefaultName);
> +
> + //
> + // Now do consistent name
> + //
> + NewConsistName = ShellCommandConsistMappingGenMappingName
> (DevicePathList[Count], ConsistMappingTable);
> + if (NewConsistName != NULL) {
> + Status = ShellCommandAddMapItemAndUpdatePath
> (NewConsistName, DevicePathList[Count], 0, FALSE);
> + ASSERT_EFI_ERROR (Status);
> + FreePool (NewConsistName);
> + }
> }
> }
>
> - ShellCommandConsistMappingUnInitialize (ConsistMappingTable);
> + if (ConsistMappingTable != NULL) {
> + ShellCommandConsistMappingUnInitialize (ConsistMappingTable);
> + }
>
> SHELL_FREE_NON_NULL (HandleList);
> SHELL_FREE_NON_NULL (DevicePathList); @@ -1626,12 +1630,12 @@
> ShellCommandUpdateMapping (
> //
> PerformQuickSort (DevicePathList, Count, sizeof
> (EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);
>
> - ShellCommandConsistMappingInitialize (&ConsistMappingTable);
> + Status = ShellCommandConsistMappingInitialize
> + (&ConsistMappingTable);
>
> //
> // Assign new Mappings to remainders
> //
> - for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL
> && !EFI_ERROR (Status); Count++) {
> + for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL;
> + Count++) {
> //
> // Skip ones that already have
> //
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
> index 97a4b57a932f..5329b559ba46 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
> @@ -158,7 +158,10 @@ ShellCommandRunDblk (
> ShellStatus = SHELL_INVALID_PARAMETER;
> }
>
> - ShellConvertStringToUint64 (LbaString, &Lba, TRUE, FALSE);
> + if (EFI_ERROR (ShellConvertStringToUint64 (LbaString, &Lba, TRUE,
> FALSE))) {
> + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
> gShellDebug1HiiHandle, L"dblk", LbaString);
> + ShellStatus = SHELL_INVALID_PARAMETER;
> + }
> }
>
> if (BlockCountString == NULL) {
> @@ -169,12 +172,13 @@ ShellCommandRunDblk (
> ShellStatus = SHELL_INVALID_PARAMETER;
> }
>
> - ShellConvertStringToUint64 (BlockCountString, &BlockCount, TRUE,
> FALSE);
> - if (BlockCount > 0x10) {
> - BlockCount = 0x10;
> - } else if (BlockCount == 0) {
> - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
> gShellDebug1HiiHandle, L"dblk", BlockCountString);
> - ShellStatus = SHELL_INVALID_PARAMETER;
> + if (!EFI_ERROR (ShellConvertStringToUint64 (BlockCountString,
> &BlockCount, TRUE, FALSE))) {
> + if (BlockCount > 0x10) {
> + BlockCount = 0x10;
> + } else if (BlockCount == 0) {
> + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
> gShellDebug1HiiHandle, L"dblk", BlockCountString);
> + ShellStatus = SHELL_INVALID_PARAMETER;
> + }
> }
> }
>
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
> index 8bf23a2076a1..72f8c087cb69 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
> @@ -112,10 +112,13 @@ ShellCommandRunEfiDecompress (
>
> if (ShellStatus == SHELL_SUCCESS) {
> Status = FileHandleGetSize (InFileHandle, &Temp64Bit);
> - ASSERT (Temp64Bit <= (UINT32)(-1));
> - InSize = (UINTN)Temp64Bit;
> ASSERT_EFI_ERROR (Status);
> - InBuffer = AllocateZeroPool (InSize);
> + if (!EFI_ERROR (Status)) {
> + ASSERT (Temp64Bit <= (UINT32)(-1));
> + InSize = (UINTN)Temp64Bit;
> + InBuffer = AllocateZeroPool (InSize);
> + }
> +
> if (InBuffer == NULL) {
> Status = EFI_OUT_OF_RESOURCES;
> } else {
> diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
> b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
> index d7a133c0c5b4..870c5b0d1da7 100644
> --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
> +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
> @@ -508,9 +508,10 @@ ShellCommandRunConnect (
> Count = ShellCommandLineGetCount (Package);
>
> if (Param1 != NULL) {
> - Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE,
> FALSE);
> - Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
> - if (EFI_ERROR (Status)) {
> + Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE,
> FALSE);
> + if (!EFI_ERROR (Status)) {
> + Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
> + } else {
> ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
> gShellDriver1HiiHandle, L"connect", Param1);
> ShellStatus = SHELL_INVALID_PARAMETER;
> }
> @@ -519,9 +520,10 @@ ShellCommandRunConnect (
> }
>
> if (Param2 != NULL) {
> - Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE,
> FALSE);
> - Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
> - if (EFI_ERROR (Status)) {
> + Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE,
> FALSE);
> + if (!EFI_ERROR (Status)) {
> + Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
> + } else {
> ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
> gShellDriver1HiiHandle, L"connect", Param2);
> ShellStatus = SHELL_INVALID_PARAMETER;
> }
> diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> index 009ae5282b27..fd49d1f7ceb4 100644
> --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> @@ -160,12 +160,17 @@ ShellCommandRunDisconnect (
> Param1 = ShellCommandLineGetRawValue (Package, 1);
> Param2 = ShellCommandLineGetRawValue (Package, 2);
> Param3 = ShellCommandLineGetRawValue (Package, 3);
> - ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE);
> - Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate1) : NULL;
> - ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE);
> - Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate2) : NULL;
> - ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE);
> - Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate3) : NULL;
> + if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1,
> TRUE, FALSE))) {
> + Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate1) : NULL;
> + }
> +
> + if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2,
> TRUE, FALSE))) {
> + Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate2) : NULL;
> + }
> +
> + if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3,
> TRUE, FALSE))) {
> + Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate3) : NULL;
> + }
>
> if ((Param1 != NULL) && (Handle1 == NULL)) {
> ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
> gShellDriver1HiiHandle, L"disconnect", Param1); diff --git
> a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
> b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
> index c645c9fd6882..8f70d6b6af39 100644
> --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
> +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
> @@ -438,25 +438,22 @@ ShellCommandRunDrvDiag (
> ControllerHandleStr = ShellCommandLineGetRawValue (Package, 2);
> ChildHandleStr = ShellCommandLineGetRawValue (Package, 3);
>
> - if (DriverHandleStr == NULL) {
> - Handle1 = NULL;
> - } else {
> - ShellConvertStringToUint64 (DriverHandleStr, &Intermediate, TRUE,
> FALSE);
> + if ((DriverHandleStr != NULL) && ShellConvertStringToUint64
> + (DriverHandleStr, &Intermediate, TRUE, FALSE)) {
> Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
> + } else {
> + Handle1 = NULL;
> }
>
> - if (ControllerHandleStr == NULL) {
> - Handle2 = NULL;
> - } else {
> - ShellConvertStringToUint64 (ControllerHandleStr, &Intermediate, TRUE,
> FALSE);
> + if ((ControllerHandleStr != NULL) && ShellConvertStringToUint64
> + (ControllerHandleStr, &Intermediate, TRUE, FALSE)) {
> Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
> + } else {
> + Handle2 = NULL;
> }
>
> - if (ChildHandleStr == NULL) {
> - Handle3 = NULL;
> - } else {
> - ShellConvertStringToUint64 (ChildHandleStr, &Intermediate, TRUE,
> FALSE);
> + if ((ChildHandleStr != NULL) && ShellConvertStringToUint64
> + (ChildHandleStr, &Intermediate, TRUE, FALSE)) {
> Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
> + } else {
> + Handle3 = NULL;
> }
>
> Status = DoDiagnostics (
> --
> 2.28.0.windows.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
2022-11-24 2:14 ` Michael Kubacki
@ 2022-11-24 2:31 ` Michael D Kinney
2022-11-24 5:12 ` Ni, Ray
0 siblings, 1 reply; 33+ messages in thread
From: Michael D Kinney @ 2022-11-24 2:31 UTC (permalink / raw)
To: Michael Kubacki, devel@edk2.groups.io, Kinney, Michael D, Ni, Ray
Cc: Dong, Eric, Erich McMillan, Kumar, Rahul R, Ni, Ray
Hi Michael,
It does not look like it is required. If the MP init fails for any reason. Could be
HW failure, but the BSP is still working because it is obviously executing code, then
the system should continue to boot with the BSP.
I will defer to Ray on this topic.
Mike
> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Wednesday, November 23, 2022 6:14 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
>
> The ASSERT() was added to aid debugging by bringing attention to the
> point where the fallback assignment occurs in the error status check block.
>
> Are you suggesting the ASSERT() be removed because of a known case where
> it might trigger but the case is expected to return an error? Or, that
> is is not necessary in general?
>
> Thanks,
> Michael
>
> On 11/23/2022 9:04 PM, Michael D Kinney wrote:
> > Hi Michael,
> >
> > comments 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: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> >> Ni, Ray <ray.ni@intel.com>
> >> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
> >>
> >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>
> >> Fixes CodeQL alerts for CWE-457:
> >> https://cwe.mitre.org/data/definitions/457.html
> >>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Erich McMillan <emcmillan@microsoft.com>
> >> 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>
> >> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >> ---
> >> UefiCpuPkg/CpuMpPei/CpuBist.c | 8 +++++++-
> >> UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++++++-
> >> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++-
> >> 3 files changed, 22 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c
> >> index 7dc93cd784d4..122808139b87 100644
> >> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c
> >> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c
> >> @@ -175,7 +175,13 @@ CollectBistDataFromPpi (
> >> EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2;
> >> EFI_SEC_PLATFORM_INFORMATION_CPU *CpuInstanceInHob;
> >>
> >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
> >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
> >> + ASSERT_EFI_ERROR (Status);
> >
> > I think this ASSERT() should be removed. The added error status check looks correct.
> >
> >> +
> >> + if (EFI_ERROR (Status)) {
> >> + NumberOfProcessors = 1u;
> >> + NumberOfEnabledProcessors = 1u;
> >> + }
> >>
> >> BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) +
> >> sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors;
> >> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> >> index e7f1fe9f426c..a84304273168 100644
> >> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> >> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> >> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers (
> >> return;
> >> }
> >>
> >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> >> + ASSERT_EFI_ERROR (Status);
> >
> > I think this ASSERT() should be removed. The added error status check looks correct.
> >
> >> +
> >> + if (EFI_ERROR (Status)) {
> >> + NumberOfProcessors = 1u;
> >> + }
> >> +
> >> SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT)));
> >> ASSERT (SwitchStackData != NULL);
> >> ZeroMem (SwitchStackData, NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
> >> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> >> index 135422225340..1322fcb77f28 100644
> >> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> >> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> >> @@ -538,6 +538,7 @@ SetupStackGuardPage (
> >> UINTN NumberOfProcessors;
> >> UINTN Bsp;
> >> UINTN Index;
> >> + EFI_STATUS Status;
> >>
> >> //
> >> // One extra page at the bottom of the stack is needed for Guard page.
> >> @@ -547,7 +548,13 @@ SetupStackGuardPage (
> >> ASSERT (FALSE);
> >> }
> >>
> >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> >> + ASSERT_EFI_ERROR (Status);
> >
> > I think this ASSERT() should be removed. The added error status check looks correct.
> >
> >> +
> >> + if (EFI_ERROR (Status)) {
> >> + NumberOfProcessors = 1u;
> >> + }
> >> +
> >> MpInitLibWhoAmI (&Bsp);
> >> for (Index = 0; Index < NumberOfProcessors; ++Index) {
> >> StackBase = 0;
> >> --
> >> 2.28.0.windows.1
> >>
> >>
> >>
> >> -=-=-=-=-=-=
> >> Groups.io Links: You receive all messages sent to this group.
> >> View/Reply Online (#96156): https://edk2.groups.io/g/devel/message/96156
> >> Mute This Topic: https://groups.io/mt/94918104/1643496
> >> Group Owner: devel+owner@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> >> -=-=-=-=-=-=
> >>
> >
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables
2022-11-24 2:19 ` Gao, Zhichao
@ 2022-11-24 2:36 ` Michael Kubacki
0 siblings, 0 replies; 33+ messages in thread
From: Michael Kubacki @ 2022-11-24 2:36 UTC (permalink / raw)
To: devel, zhichao.gao; +Cc: Erich McMillan, Kinney, Michael D, Ni, Ray
Thanks. I will include this in the v2 series.
On 11/23/2022 9:19 PM, Gao, Zhichao wrote:
> See comments below:
>
>> -----Original Message-----
>> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
>> Sent: Thursday, November 10, 2022 1:33 AM
>> To: devel@edk2.groups.io
>> Cc: Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Michael Kubacki
>> <mikuback@linux.microsoft.com>; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
>> <zhichao.gao@intel.com>
>> Subject: [PATCH v1 09/12] ShellPkg: Fix conditionally uninitialized variables
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Fixes CodeQL alerts for CWE-457:
>> https://cwe.mitre.org/data/definitions/457.html
>>
>> Cc: Erich McMillan <emcmillan@microsoft.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Zhichao Gao <zhichao.gao@intel.com>
>> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>> ShellPkg/Application/Shell/Shell.c | 2 +-
>> ShellPkg/Application/Shell/ShellProtocol.c | 4 +-
>> 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 ++++----
>> 8 files changed, 78 insertions(+), 63 deletions(-)
>>
>> diff --git a/ShellPkg/Application/Shell/Shell.c
>> b/ShellPkg/Application/Shell/Shell.c
>> index df00adfdfa5b..86db2f4ebb6e 100644
>> --- a/ShellPkg/Application/Shell/Shell.c
>> +++ b/ShellPkg/Application/Shell/Shell.c
>> @@ -1324,7 +1324,7 @@ DoStartupScript (
>> }
>>
>> Status = RunShellCommand (FileStringPath, &CalleeStatus);
>> - if (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE) {
>> + if (!EFI_ERROR (Status) &&
>> + (ShellInfoObject.ShellInitSettings.BitUnion.Bits.Exit == TRUE)) {
>
> Incorrect here. Cannot handle the unsuccess condition. Better to assign the success initial value to Calleestatus and keep the org logic.
>
>> ShellCommandRegisterExit (gEfiShellProtocol->BatchIsActive (),
>> (UINT64)CalleeStatus);
>> }
>>
>> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
>> b/ShellPkg/Application/Shell/ShellProtocol.c
>> index 509eb60e40f4..9183da284fff 100644
>> --- a/ShellPkg/Application/Shell/ShellProtocol.c
>> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
>> @@ -729,8 +729,8 @@ EfiShellGetDeviceName (
>> //
>> // Now check the parent controller using this as the child.
>> //
>> - if (DeviceNameToReturn == NULL) {
>> - PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle,
>> &ParentControllerCount, &ParentControllerBuffer);
>> + Status = PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle,
>> &ParentControllerCount, &ParentControllerBuffer);
>> + if ((DeviceNameToReturn == NULL) && !EFI_ERROR (Status)) {
>> for (LoopVar = 0; LoopVar < ParentControllerCount; LoopVar++) {
>> PARSE_HANDLE_DATABASE_UEFI_DRIVERS
>> (ParentControllerBuffer[LoopVar], &ParentDriverCount,
>> &ParentDriverBuffer);
>
> Should we cover above function as well?
>
It was not identified by the query results, but I can add it if we like.
> Others looks good to me.
>
> Thanks,
> Zhichao
>
>> for (HandleCount = 0; HandleCount < ParentDriverCount;
>> HandleCount++) { diff --git
>> a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
>> b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
>> index 36cf46fb2c38..4549cbde9b9a 100644
>> --- a/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
>> +++ b/ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c
>> @@ -1399,10 +1399,11 @@ ShellCommandCreateInitialMappingsAndPaths (
>> CHAR16 *MapName;
>> SHELL_MAP_LIST *MapListItem;
>>
>> - SplitCurDir = NULL;
>> - MapName = NULL;
>> - MapListItem = NULL;
>> - HandleList = NULL;
>> + ConsistMappingTable = NULL;
>> + SplitCurDir = NULL;
>> + MapName = NULL;
>> + MapListItem = NULL;
>> + HandleList = NULL;
>>
>> //
>> // Reset the static members back to zero @@ -1458,32 +1459,35 @@
>> ShellCommandCreateInitialMappingsAndPaths (
>> //
>> PerformQuickSort (DevicePathList, Count, sizeof
>> (EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);
>>
>> - ShellCommandConsistMappingInitialize (&ConsistMappingTable);
>> - //
>> - // Assign new Mappings to all...
>> - //
>> - for (Count = 0; HandleList[Count] != NULL; Count++) {
>> + if (!EFI_ERROR (ShellCommandConsistMappingInitialize
>> + (&ConsistMappingTable))) {
>> //
>> - // Get default name first
>> + // Assign new Mappings to all...
>> //
>> - NewDefaultName = ShellCommandCreateNewMappingName
>> (MappingTypeFileSystem);
>> - ASSERT (NewDefaultName != NULL);
>> - Status = ShellCommandAddMapItemAndUpdatePath (NewDefaultName,
>> DevicePathList[Count], 0, TRUE);
>> - ASSERT_EFI_ERROR (Status);
>> - FreePool (NewDefaultName);
>> -
>> - //
>> - // Now do consistent name
>> - //
>> - NewConsistName = ShellCommandConsistMappingGenMappingName
>> (DevicePathList[Count], ConsistMappingTable);
>> - if (NewConsistName != NULL) {
>> - Status = ShellCommandAddMapItemAndUpdatePath
>> (NewConsistName, DevicePathList[Count], 0, FALSE);
>> + for (Count = 0; HandleList[Count] != NULL; Count++) {
>> + //
>> + // Get default name first
>> + //
>> + NewDefaultName = ShellCommandCreateNewMappingName
>> (MappingTypeFileSystem);
>> + ASSERT (NewDefaultName != NULL);
>> + Status = ShellCommandAddMapItemAndUpdatePath
>> (NewDefaultName,
>> + DevicePathList[Count], 0, TRUE);
>> ASSERT_EFI_ERROR (Status);
>> - FreePool (NewConsistName);
>> + FreePool (NewDefaultName);
>> +
>> + //
>> + // Now do consistent name
>> + //
>> + NewConsistName = ShellCommandConsistMappingGenMappingName
>> (DevicePathList[Count], ConsistMappingTable);
>> + if (NewConsistName != NULL) {
>> + Status = ShellCommandAddMapItemAndUpdatePath
>> (NewConsistName, DevicePathList[Count], 0, FALSE);
>> + ASSERT_EFI_ERROR (Status);
>> + FreePool (NewConsistName);
>> + }
>> }
>> }
>>
>> - ShellCommandConsistMappingUnInitialize (ConsistMappingTable);
>> + if (ConsistMappingTable != NULL) {
>> + ShellCommandConsistMappingUnInitialize (ConsistMappingTable);
>> + }
>>
>> SHELL_FREE_NON_NULL (HandleList);
>> SHELL_FREE_NON_NULL (DevicePathList); @@ -1626,12 +1630,12 @@
>> ShellCommandUpdateMapping (
>> //
>> PerformQuickSort (DevicePathList, Count, sizeof
>> (EFI_DEVICE_PATH_PROTOCOL *), DevicePathCompare);
>>
>> - ShellCommandConsistMappingInitialize (&ConsistMappingTable);
>> + Status = ShellCommandConsistMappingInitialize
>> + (&ConsistMappingTable);
>>
>> //
>> // Assign new Mappings to remainders
>> //
>> - for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL
>> && !EFI_ERROR (Status); Count++) {
>> + for (Count = 0; !EFI_ERROR (Status) && HandleList[Count] != NULL;
>> + Count++) {
>> //
>> // Skip ones that already have
>> //
>> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
>> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
>> index 97a4b57a932f..5329b559ba46 100644
>> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
>> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c
>> @@ -158,7 +158,10 @@ ShellCommandRunDblk (
>> ShellStatus = SHELL_INVALID_PARAMETER;
>> }
>>
>> - ShellConvertStringToUint64 (LbaString, &Lba, TRUE, FALSE);
>> + if (EFI_ERROR (ShellConvertStringToUint64 (LbaString, &Lba, TRUE,
>> FALSE))) {
>> + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
>> gShellDebug1HiiHandle, L"dblk", LbaString);
>> + ShellStatus = SHELL_INVALID_PARAMETER;
>> + }
>> }
>>
>> if (BlockCountString == NULL) {
>> @@ -169,12 +172,13 @@ ShellCommandRunDblk (
>> ShellStatus = SHELL_INVALID_PARAMETER;
>> }
>>
>> - ShellConvertStringToUint64 (BlockCountString, &BlockCount, TRUE,
>> FALSE);
>> - if (BlockCount > 0x10) {
>> - BlockCount = 0x10;
>> - } else if (BlockCount == 0) {
>> - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
>> gShellDebug1HiiHandle, L"dblk", BlockCountString);
>> - ShellStatus = SHELL_INVALID_PARAMETER;
>> + if (!EFI_ERROR (ShellConvertStringToUint64 (BlockCountString,
>> &BlockCount, TRUE, FALSE))) {
>> + if (BlockCount > 0x10) {
>> + BlockCount = 0x10;
>> + } else if (BlockCount == 0) {
>> + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
>> gShellDebug1HiiHandle, L"dblk", BlockCountString);
>> + ShellStatus = SHELL_INVALID_PARAMETER;
>> + }
>> }
>> }
>>
>> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
>> b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
>> index 8bf23a2076a1..72f8c087cb69 100644
>> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
>> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c
>> @@ -112,10 +112,13 @@ ShellCommandRunEfiDecompress (
>>
>> if (ShellStatus == SHELL_SUCCESS) {
>> Status = FileHandleGetSize (InFileHandle, &Temp64Bit);
>> - ASSERT (Temp64Bit <= (UINT32)(-1));
>> - InSize = (UINTN)Temp64Bit;
>> ASSERT_EFI_ERROR (Status);
>> - InBuffer = AllocateZeroPool (InSize);
>> + if (!EFI_ERROR (Status)) {
>> + ASSERT (Temp64Bit <= (UINT32)(-1));
>> + InSize = (UINTN)Temp64Bit;
>> + InBuffer = AllocateZeroPool (InSize);
>> + }
>> +
>> if (InBuffer == NULL) {
>> Status = EFI_OUT_OF_RESOURCES;
>> } else {
>> diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
>> b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
>> index d7a133c0c5b4..870c5b0d1da7 100644
>> --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
>> +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c
>> @@ -508,9 +508,10 @@ ShellCommandRunConnect (
>> Count = ShellCommandLineGetCount (Package);
>>
>> if (Param1 != NULL) {
>> - Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE,
>> FALSE);
>> - Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
>> - if (EFI_ERROR (Status)) {
>> + Status = ShellConvertStringToUint64 (Param1, &Intermediate, TRUE,
>> FALSE);
>> + if (!EFI_ERROR (Status)) {
>> + Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
>> + } else {
>> ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
>> gShellDriver1HiiHandle, L"connect", Param1);
>> ShellStatus = SHELL_INVALID_PARAMETER;
>> }
>> @@ -519,9 +520,10 @@ ShellCommandRunConnect (
>> }
>>
>> if (Param2 != NULL) {
>> - Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE,
>> FALSE);
>> - Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
>> - if (EFI_ERROR (Status)) {
>> + Status = ShellConvertStringToUint64 (Param2, &Intermediate, TRUE,
>> FALSE);
>> + if (!EFI_ERROR (Status)) {
>> + Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
>> + } else {
>> ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
>> gShellDriver1HiiHandle, L"connect", Param2);
>> ShellStatus = SHELL_INVALID_PARAMETER;
>> }
>> diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
>> b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
>> index 009ae5282b27..fd49d1f7ceb4 100644
>> --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
>> +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
>> @@ -160,12 +160,17 @@ ShellCommandRunDisconnect (
>> Param1 = ShellCommandLineGetRawValue (Package, 1);
>> Param2 = ShellCommandLineGetRawValue (Package, 2);
>> Param3 = ShellCommandLineGetRawValue (Package, 3);
>> - ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE);
>> - Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle
>> ((UINTN)Intermediate1) : NULL;
>> - ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE);
>> - Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle
>> ((UINTN)Intermediate2) : NULL;
>> - ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE);
>> - Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle
>> ((UINTN)Intermediate3) : NULL;
>> + if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1,
>> TRUE, FALSE))) {
>> + Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle
>> ((UINTN)Intermediate1) : NULL;
>> + }
>> +
>> + if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2,
>> TRUE, FALSE))) {
>> + Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle
>> ((UINTN)Intermediate2) : NULL;
>> + }
>> +
>> + if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3,
>> TRUE, FALSE))) {
>> + Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle
>> ((UINTN)Intermediate3) : NULL;
>> + }
>>
>> if ((Param1 != NULL) && (Handle1 == NULL)) {
>> ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_GEN_INV_HANDLE),
>> gShellDriver1HiiHandle, L"disconnect", Param1); diff --git
>> a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
>> b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
>> index c645c9fd6882..8f70d6b6af39 100644
>> --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
>> +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c
>> @@ -438,25 +438,22 @@ ShellCommandRunDrvDiag (
>> ControllerHandleStr = ShellCommandLineGetRawValue (Package, 2);
>> ChildHandleStr = ShellCommandLineGetRawValue (Package, 3);
>>
>> - if (DriverHandleStr == NULL) {
>> - Handle1 = NULL;
>> - } else {
>> - ShellConvertStringToUint64 (DriverHandleStr, &Intermediate, TRUE,
>> FALSE);
>> + if ((DriverHandleStr != NULL) && ShellConvertStringToUint64
>> + (DriverHandleStr, &Intermediate, TRUE, FALSE)) {
>> Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
>> + } else {
>> + Handle1 = NULL;
>> }
>>
>> - if (ControllerHandleStr == NULL) {
>> - Handle2 = NULL;
>> - } else {
>> - ShellConvertStringToUint64 (ControllerHandleStr, &Intermediate, TRUE,
>> FALSE);
>> + if ((ControllerHandleStr != NULL) && ShellConvertStringToUint64
>> + (ControllerHandleStr, &Intermediate, TRUE, FALSE)) {
>> Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
>> + } else {
>> + Handle2 = NULL;
>> }
>>
>> - if (ChildHandleStr == NULL) {
>> - Handle3 = NULL;
>> - } else {
>> - ShellConvertStringToUint64 (ChildHandleStr, &Intermediate, TRUE,
>> FALSE);
>> + if ((ChildHandleStr != NULL) && ShellConvertStringToUint64
>> + (ChildHandleStr, &Intermediate, TRUE, FALSE)) {
>> Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate);
>> + } else {
>> + Handle3 = NULL;
>> }
>>
>> Status = DoDiagnostics (
>> --
>> 2.28.0.windows.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v1 08/12] PcAtChipsetPkg: Fix conditionally uninitialized variables
2022-11-24 2:00 ` Michael D Kinney
@ 2022-11-24 5:01 ` Ni, Ray
0 siblings, 0 replies; 33+ messages in thread
From: Ni, Ray @ 2022-11-24 5:01 UTC (permalink / raw)
To: Kinney, Michael D, mikuback@linux.microsoft.com,
devel@edk2.groups.io
Cc: Erich McMillan
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, November 24, 2022 10:01 AM
> To: mikuback@linux.microsoft.com; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Erich McMillan <emcmillan@microsoft.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH v1 08/12] PcAtChipsetPkg: Fix conditionally uninitialized variables
>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
>
> > -----Original Message-----
> > From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> > Sent: Wednesday, November 9, 2022 9:33 AM
> > To: devel@edk2.groups.io
> > Cc: Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Michael Kubacki
> > <mikuback@linux.microsoft.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: [PATCH v1 08/12] PcAtChipsetPkg: Fix conditionally uninitialized variables
> >
> > From: Michael Kubacki <michael.kubacki@microsoft.com>
> >
> > Fixes CodeQL alerts for CWE-457:
> > https://cwe.mitre.org/data/definitions/457.html
> >
> > Cc: Erich McMillan <emcmillan@microsoft.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> > Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> > ---
> > PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > index 9242a2e82600..57ea3153aa6b 100644
> > --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> > @@ -344,7 +344,7 @@ PcRtcInit (
> > // so we can use them to get and set wakeup time.
> > //
> > Status = PcRtcGetWakeupTime (&Enabled, &Pending, &Time, Global);
> > - if ((Enabled) || (!EFI_ERROR (Status))) {
> > + if ((!EFI_ERROR (Status)) || (Enabled)) {
> > return EFI_SUCCESS;
> > }
> >
> > @@ -836,8 +836,11 @@ PcRtcSetWakeupTime (
> > //
> > // Just support set alarm time within 24 hours
> > //
> > - PcRtcGetTime (&RtcTime, &Capabilities, Global);
> > - Status = RtcTimeFieldsValid (&RtcTime);
> > + Status = PcRtcGetTime (&RtcTime, &Capabilities, Global);
> > + if (!EFI_ERROR (Status)) {
> > + Status = RtcTimeFieldsValid (&RtcTime);
> > + }
> > +
> > if (EFI_ERROR (Status)) {
> > return EFI_DEVICE_ERROR;
> > }
> > --
> > 2.28.0.windows.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
2022-11-24 2:31 ` Michael D Kinney
@ 2022-11-24 5:12 ` Ni, Ray
2022-11-28 22:50 ` Michael Kubacki
0 siblings, 1 reply; 33+ messages in thread
From: Ni, Ray @ 2022-11-24 5:12 UTC (permalink / raw)
To: Kinney, Michael D, Michael Kubacki, devel@edk2.groups.io
Cc: Dong, Eric, Erich McMillan, Kumar, Rahul R
Mike,
IMO, the failure is not expected at all. When such failure appears, no guaranteed system can run further because the hw is in a unstable state and I think sw should just give up.
But ASSERT() is a NOP in release image.
I feel that we may need some macro that can still work in release image.
E.g.: VERIFY_EFI_SUCCESS (Status)? so if-check is not needed.
But it's a different topic.
Michael, can CodeQL be happy with only ASSERT()? I just feel the "if" check looks like the failure should be handled by sw.
Thanks,
Ray
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Thursday, November 24, 2022 10:31 AM
> To: Michael Kubacki <mikuback@linux.microsoft.com>; devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
>
> Hi Michael,
>
> It does not look like it is required. If the MP init fails for any reason. Could be
> HW failure, but the BSP is still working because it is obviously executing code, then
> the system should continue to boot with the BSP.
>
> I will defer to Ray on this topic.
>
> Mike
>
> > -----Original Message-----
> > From: Michael Kubacki <mikuback@linux.microsoft.com>
> > Sent: Wednesday, November 23, 2022 6:14 PM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>;
> > Ni, Ray <ray.ni@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
> >
> > The ASSERT() was added to aid debugging by bringing attention to the
> > point where the fallback assignment occurs in the error status check block.
> >
> > Are you suggesting the ASSERT() be removed because of a known case where
> > it might trigger but the case is expected to return an error? Or, that
> > is is not necessary in general?
> >
> > Thanks,
> > Michael
> >
> > On 11/23/2022 9:04 PM, Michael D Kinney wrote:
> > > Hi Michael,
> > >
> > > comments 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: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
> > >> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>;
> > >> Ni, Ray <ray.ni@intel.com>
> > >> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
> > >>
> > >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> > >>
> > >> Fixes CodeQL alerts for CWE-457:
> > >> https://cwe.mitre.org/data/definitions/457.html
> > >>
> > >> Cc: Eric Dong <eric.dong@intel.com>
> > >> Cc: Erich McMillan <emcmillan@microsoft.com>
> > >> 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>
> > >> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> > >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> > >> ---
> > >> UefiCpuPkg/CpuMpPei/CpuBist.c | 8 +++++++-
> > >> UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++++++-
> > >> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++-
> > >> 3 files changed, 22 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c
> > >> index 7dc93cd784d4..122808139b87 100644
> > >> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c
> > >> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c
> > >> @@ -175,7 +175,13 @@ CollectBistDataFromPpi (
> > >> EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2;
> > >> EFI_SEC_PLATFORM_INFORMATION_CPU *CpuInstanceInHob;
> > >>
> > >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
> > >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
> > >> + ASSERT_EFI_ERROR (Status);
> > >
> > > I think this ASSERT() should be removed. The added error status check looks correct.
> > >
> > >> +
> > >> + if (EFI_ERROR (Status)) {
> > >> + NumberOfProcessors = 1u;
> > >> + NumberOfEnabledProcessors = 1u;
> > >> + }
> > >>
> > >> BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) +
> > >> sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors;
> > >> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> > >> index e7f1fe9f426c..a84304273168 100644
> > >> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> > >> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> > >> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers (
> > >> return;
> > >> }
> > >>
> > >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> > >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> > >> + ASSERT_EFI_ERROR (Status);
> > >
> > > I think this ASSERT() should be removed. The added error status check looks correct.
> > >
> > >> +
> > >> + if (EFI_ERROR (Status)) {
> > >> + NumberOfProcessors = 1u;
> > >> + }
> > >> +
> > >> SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof
> (EXCEPTION_STACK_SWITCH_CONTEXT)));
> > >> ASSERT (SwitchStackData != NULL);
> > >> ZeroMem (SwitchStackData, NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
> > >> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > >> index 135422225340..1322fcb77f28 100644
> > >> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > >> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> > >> @@ -538,6 +538,7 @@ SetupStackGuardPage (
> > >> UINTN NumberOfProcessors;
> > >> UINTN Bsp;
> > >> UINTN Index;
> > >> + EFI_STATUS Status;
> > >>
> > >> //
> > >> // One extra page at the bottom of the stack is needed for Guard page.
> > >> @@ -547,7 +548,13 @@ SetupStackGuardPage (
> > >> ASSERT (FALSE);
> > >> }
> > >>
> > >> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> > >> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> > >> + ASSERT_EFI_ERROR (Status);
> > >
> > > I think this ASSERT() should be removed. The added error status check looks correct.
> > >
> > >> +
> > >> + if (EFI_ERROR (Status)) {
> > >> + NumberOfProcessors = 1u;
> > >> + }
> > >> +
> > >> MpInitLibWhoAmI (&Bsp);
> > >> for (Index = 0; Index < NumberOfProcessors; ++Index) {
> > >> StackBase = 0;
> > >> --
> > >> 2.28.0.windows.1
> > >>
> > >>
> > >>
> > >> -=-=-=-=-=-=
> > >> Groups.io Links: You receive all messages sent to this group.
> > >> View/Reply Online (#96156): https://edk2.groups.io/g/devel/message/96156
> > >> Mute This Topic: https://groups.io/mt/94918104/1643496
> > >> Group Owner: devel+owner@edk2.groups.io
> > >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> > >> -=-=-=-=-=-=
> > >>
> > >
> > >
> > >
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
2022-11-24 5:12 ` Ni, Ray
@ 2022-11-28 22:50 ` Michael Kubacki
0 siblings, 0 replies; 33+ messages in thread
From: Michael Kubacki @ 2022-11-28 22:50 UTC (permalink / raw)
To: Ni, Ray, Kinney, Michael D, devel@edk2.groups.io
Cc: Dong, Eric, Erich McMillan, Kumar, Rahul R
Hi Ray,
CodeQL is mainly concerned that the Status returned from
MpInitLibGetNumberOfProcessors() is not checked.
So this was meant to address that while providing a consistent value in
case of an error. The assert is used to alert developers if this
condition occurs.
I'm open to other changes but I was hesitant to make a major functional
change that impacts RELEASE images since I'm not sure how many, if any,
systems may be continuing to execute despite an error being returned here.
Thanks,
Michael
On 11/24/2022 12:12 AM, Ni, Ray wrote:
> Mike,
> IMO, the failure is not expected at all. When such failure appears, no guaranteed system can run further because the hw is in a unstable state and I think sw should just give up.
> But ASSERT() is a NOP in release image.
>
> I feel that we may need some macro that can still work in release image.
> E.g.: VERIFY_EFI_SUCCESS (Status)? so if-check is not needed.
>
> But it's a different topic.
>
> Michael, can CodeQL be happy with only ASSERT()? I just feel the "if" check looks like the failure should be handled by sw.
>
> Thanks,
> Ray
>
>
>> -----Original Message-----
>> From: Kinney, Michael D <michael.d.kinney@intel.com>
>> Sent: Thursday, November 24, 2022 10:31 AM
>> To: Michael Kubacki <mikuback@linux.microsoft.com>; devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kumar, Rahul R
>> <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: RE: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
>>
>> Hi Michael,
>>
>> It does not look like it is required. If the MP init fails for any reason. Could be
>> HW failure, but the BSP is still working because it is obviously executing code, then
>> the system should continue to boot with the BSP.
>>
>> I will defer to Ray on this topic.
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: Michael Kubacki <mikuback@linux.microsoft.com>
>>> Sent: Wednesday, November 23, 2022 6:14 PM
>>> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kumar, Rahul R
>> <rahul.r.kumar@intel.com>;
>>> Ni, Ray <ray.ni@intel.com>
>>> Subject: Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
>>>
>>> The ASSERT() was added to aid debugging by bringing attention to the
>>> point where the fallback assignment occurs in the error status check block.
>>>
>>> Are you suggesting the ASSERT() be removed because of a known case where
>>> it might trigger but the case is expected to return an error? Or, that
>>> is is not necessary in general?
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 11/23/2022 9:04 PM, Michael D Kinney wrote:
>>>> Hi Michael,
>>>>
>>>> comments 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: Dong, Eric <eric.dong@intel.com>; Erich McMillan <emcmillan@microsoft.com>; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Kumar, Rahul R
>> <rahul.r.kumar@intel.com>;
>>>>> Ni, Ray <ray.ni@intel.com>
>>>>> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables
>>>>>
>>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>
>>>>> Fixes CodeQL alerts for CWE-457:
>>>>> https://cwe.mitre.org/data/definitions/457.html
>>>>>
>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>> Cc: Erich McMillan <emcmillan@microsoft.com>
>>>>> 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>
>>>>> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
>>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>> ---
>>>>> UefiCpuPkg/CpuMpPei/CpuBist.c | 8 +++++++-
>>>>> UefiCpuPkg/CpuMpPei/CpuMpPei.c | 8 +++++++-
>>>>> UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++-
>>>>> 3 files changed, 22 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c b/UefiCpuPkg/CpuMpPei/CpuBist.c
>>>>> index 7dc93cd784d4..122808139b87 100644
>>>>> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c
>>>>> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c
>>>>> @@ -175,7 +175,13 @@ CollectBistDataFromPpi (
>>>>> EFI_SEC_PLATFORM_INFORMATION_RECORD2 *PlatformInformationRecord2;
>>>>> EFI_SEC_PLATFORM_INFORMATION_CPU *CpuInstanceInHob;
>>>>>
>>>>> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
>>>>> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, &NumberOfEnabledProcessors);
>>>>> + ASSERT_EFI_ERROR (Status);
>>>>
>>>> I think this ASSERT() should be removed. The added error status check looks correct.
>>>>
>>>>> +
>>>>> + if (EFI_ERROR (Status)) {
>>>>> + NumberOfProcessors = 1u;
>>>>> + NumberOfEnabledProcessors = 1u;
>>>>> + }
>>>>>
>>>>> BistInformationSize = sizeof (EFI_SEC_PLATFORM_INFORMATION_RECORD2) +
>>>>> sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) * NumberOfProcessors;
>>>>> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>>>>> index e7f1fe9f426c..a84304273168 100644
>>>>> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>>>>> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
>>>>> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers (
>>>>> return;
>>>>> }
>>>>>
>>>>> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>>>>> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>>>>> + ASSERT_EFI_ERROR (Status);
>>>>
>>>> I think this ASSERT() should be removed. The added error status check looks correct.
>>>>
>>>>> +
>>>>> + if (EFI_ERROR (Status)) {
>>>>> + NumberOfProcessors = 1u;
>>>>> + }
>>>>> +
>>>>> SwitchStackData = AllocatePages (EFI_SIZE_TO_PAGES (NumberOfProcessors * sizeof
>> (EXCEPTION_STACK_SWITCH_CONTEXT)));
>>>>> ASSERT (SwitchStackData != NULL);
>>>>> ZeroMem (SwitchStackData, NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
>>>>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>>>> index 135422225340..1322fcb77f28 100644
>>>>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>>>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>>>> @@ -538,6 +538,7 @@ SetupStackGuardPage (
>>>>> UINTN NumberOfProcessors;
>>>>> UINTN Bsp;
>>>>> UINTN Index;
>>>>> + EFI_STATUS Status;
>>>>>
>>>>> //
>>>>> // One extra page at the bottom of the stack is needed for Guard page.
>>>>> @@ -547,7 +548,13 @@ SetupStackGuardPage (
>>>>> ASSERT (FALSE);
>>>>> }
>>>>>
>>>>> - MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>>>>> + Status = MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
>>>>> + ASSERT_EFI_ERROR (Status);
>>>>
>>>> I think this ASSERT() should be removed. The added error status check looks correct.
>>>>
>>>>> +
>>>>> + if (EFI_ERROR (Status)) {
>>>>> + NumberOfProcessors = 1u;
>>>>> + }
>>>>> +
>>>>> MpInitLibWhoAmI (&Bsp);
>>>>> for (Index = 0; Index < NumberOfProcessors; ++Index) {
>>>>> StackBase = 0;
>>>>> --
>>>>> 2.28.0.windows.1
>>>>>
>>>>>
>>>>>
>>>>> -=-=-=-=-=-=
>>>>> Groups.io Links: You receive all messages sent to this group.
>>>>> View/Reply Online (#96156): https://edk2.groups.io/g/devel/message/96156
>>>>> Mute This Topic: https://groups.io/mt/94918104/1643496
>>>>> Group Owner: devel+owner@edk2.groups.io
>>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
>>>>> -=-=-=-=-=-=
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2022-11-28 22:50 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-09 17:32 [PATCH v1 00/12] Enable New CodeQL Queries Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts Michael Kubacki
2022-11-24 1:28 ` [edk2-devel] " Michael D Kinney
2022-11-24 1:46 ` Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 02/12] BaseTools/PatchCheck.py: Add PCCTS to tab exemption list Michael Kubacki
2022-11-24 1:30 ` Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites Michael Kubacki
2022-11-24 1:32 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 04/12] CryptoPkg: Fix conditionally uninitialized variable Michael Kubacki
2022-11-24 1:37 ` [edk2-devel] " Michael D Kinney
2022-11-24 1:47 ` Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 05/12] MdeModulePkg: Fix conditionally uninitialized variables Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 06/12] MdePkg: " Michael Kubacki
2022-11-24 1:53 ` Michael D Kinney
2022-11-24 1:59 ` Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 07/12] NetworkPkg: " Michael Kubacki
2022-11-24 1:59 ` Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 08/12] PcAtChipsetPkg: " Michael Kubacki
2022-11-24 2:00 ` Michael D Kinney
2022-11-24 5:01 ` Ni, Ray
2022-11-09 17:32 ` [PATCH v1 09/12] ShellPkg: " Michael Kubacki
2022-11-24 2:19 ` Gao, Zhichao
2022-11-24 2:36 ` [edk2-devel] " Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 10/12] UefiCpuPkg: " Michael Kubacki
2022-11-24 2:04 ` [edk2-devel] " Michael D Kinney
2022-11-24 2:14 ` Michael Kubacki
2022-11-24 2:31 ` Michael D Kinney
2022-11-24 5:12 ` Ni, Ray
2022-11-28 22:50 ` Michael Kubacki
2022-11-09 17:32 ` [PATCH v1 11/12] .github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries Michael Kubacki
2022-11-24 2:05 ` [edk2-devel] " Michael D Kinney
2022-11-09 17:32 ` [PATCH v1 12/12] .github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries Michael Kubacki
2022-11-24 2:06 ` Michael D Kinney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox