Re: [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables


Michael D Kinney
 

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@...>
Sent: Wednesday, November 23, 2022 6:14 PM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>
Cc: Dong, Eric <eric.dong@...>; Erich McMillan <emcmillan@...>; Kumar, Rahul R <rahul.r.kumar@...>;
Ni, Ray <ray.ni@...>
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@...>; Erich McMillan <emcmillan@...>; Kinney, Michael D
<michael.d.kinney@...>; Michael Kubacki <mikuback@...>; Kumar, Rahul R <rahul.r.kumar@...>;
Ni, Ray <ray.ni@...>
Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables

From: Michael Kubacki <michael.kubacki@...>

Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html

Cc: Eric Dong <eric.dong@...>
Cc: Erich McMillan <emcmillan@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Michael Kubacki <mikuback@...>
Cc: Rahul Kumar <rahul1.kumar@...>
Cc: Ray Ni <ray.ni@...>
Co-authored-by: Erich McMillan <emcmillan@...>
Signed-off-by: Michael Kubacki <michael.kubacki@...>
---
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@...]
-=-=-=-=-=-=




Join devel@edk2.groups.io to automatically receive all group messages.