From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.134145.1669675845549301932 for ; Mon, 28 Nov 2022 14:50:45 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=ADGEYjZn; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id 40C1120B717A; Mon, 28 Nov 2022 14:50:44 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 40C1120B717A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1669675844; bh=dMa+w/+Go2HzHg4VoZvyJHEae/gbbuxriDHID8Zgtfk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=ADGEYjZnMtuaSQH9tJnP2RV46HFjWKMPBKsyQvNvj5J+vRFTz6uggyW9pIRPKpGwU 7+1LpCLiSAJGQ1+39LNZl3wwgNSKxuNsgp9GUY+Dy8XlqGhjhXGGB+TSzdE109Im4T okRfn3ayhU6mYQIrnkGUNdfv1QxenB1qqDkJUO94= Message-ID: Date: Mon, 28 Nov 2022 17:50:43 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables To: "Ni, Ray" , "Kinney, Michael D" , "devel@edk2.groups.io" Cc: "Dong, Eric" , Erich McMillan , "Kumar, Rahul R" References: <20221109173246.174-1-mikuback@linux.microsoft.com> <20221109173246.174-11-mikuback@linux.microsoft.com> <3af32683-0162-dc9c-3cb7-e21751fe6e1f@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 >> Sent: Thursday, November 24, 2022 10:31 AM >> To: Michael Kubacki ; devel@edk2.groups.io; Kinney, Michael D >> ; Ni, Ray >> Cc: Dong, Eric ; Erich McMillan ; Kumar, Rahul R >> ; Ni, Ray >> 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 >>> Sent: Wednesday, November 23, 2022 6:14 PM >>> To: devel@edk2.groups.io; Kinney, Michael D >>> Cc: Dong, Eric ; Erich McMillan ; Kumar, Rahul R >> ; >>> Ni, Ray >>> 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 On Behalf Of Michael Kubacki >>>>> Sent: Wednesday, November 9, 2022 9:33 AM >>>>> To: devel@edk2.groups.io >>>>> Cc: Dong, Eric ; Erich McMillan ; Kinney, Michael D >>>>> ; Michael Kubacki ; Kumar, Rahul R >> ; >>>>> Ni, Ray >>>>> Subject: [edk2-devel] [PATCH v1 10/12] UefiCpuPkg: Fix conditionally uninitialized variables >>>>> >>>>> From: Michael Kubacki >>>>> >>>>> Fixes CodeQL alerts for CWE-457: >>>>> https://cwe.mitre.org/data/definitions/457.html >>>>> >>>>> Cc: Eric Dong >>>>> Cc: Erich McMillan >>>>> Cc: Michael D Kinney >>>>> Cc: Michael Kubacki >>>>> Cc: Rahul Kumar >>>>> Cc: Ray Ni >>>>> Co-authored-by: Erich McMillan >>>>> Signed-off-by: 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@intel.com] >>>>> -=-=-=-=-=-= >>>>> >>>> >>>> >>>> >>>> >>>> >>>>