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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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]
> -=-=-=-=-=-=
>