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.web10.14844.1679408760611800893 for ; Tue, 21 Mar 2023 07:26:00 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=G5mg3DrG; 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 64EB420FB43F; Tue, 21 Mar 2023 07:25:59 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 64EB420FB43F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1679408760; bh=f06sSZc1qRueWkATRlopCK6m7dvODQ+0BxOdZzxeTDo=; h=Date:Subject:From:To:Cc:References:In-Reply-To:From; b=G5mg3DrG5z+NpitLpz7f/fzd7fFHmlKM17R19KiSRmwE2aZBpUuH652day25dx3+L U/F8MACbSWcR4ifKH2l7u7ODoPk8u8AekOwKQsIg3nSyOvbk6LZoLGaZ+hSvVuQsa0 LrL5YT1K5TnqHZ1MOFWRJ6JGBhJoi1Ww6w0YC3Rk= Message-ID: Date: Tue, 21 Mar 2023 10:25:58 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [edk2-devel] [PATCH v4 10/12] UefiCpuPkg: Fix conditionally uninitialized variables From: "Michael Kubacki" To: devel@edk2.groups.io, michael.d.kinney@intel.com, Erich McMillan Cc: "Dong, Eric" , "Kumar, Rahul R" , "Ni, Ray" References: <20230310184238.2999-1-mikuback@linux.microsoft.com> <20230310184238.2999-11-mikuback@linux.microsoft.com> <01b6726a-0011-250e-ccbf-0c0ed2e4c654@linux.microsoft.com> In-Reply-To: <01b6726a-0011-250e-ccbf-0c0ed2e4c654@linux.microsoft.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable I spoke to Erich offline and he mentioned that a previous coding=20 practice he used specified unsigned integer literals when intended so he=20 applied that here. Thanks, Michael On 3/10/2023 5:59 PM, Michael Kubacki wrote: > Erich introduced this particular change, he may be able to provide more= =20 > info. >=20 > On 3/10/2023 3:03 PM, Michael D Kinney wrote: >> >> >>> -----Original Message----- >>> From: devel@edk2.groups.io On Behalf Of=20 >>> Michael Kubacki >>> Sent: Friday, March 10, 2023 10:43 AM >>> To: devel@edk2.groups.io >>> Cc: Dong, Eric ; Erich McMillan=20 >>> ; Kinney, Michael D >>> ; Michael Kubacki=20 >>> ; Kumar, Rahul R >>> ; Ni, Ray >>> Subject: [edk2-devel] [PATCH v4 10/12] UefiCpuPkg: Fix conditionally=20 >>> 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 >>> --- >>> =C2=A0 UefiCpuPkg/CpuMpPei/CpuBist.c=C2=A0=C2=A0 | 8 +++++++- >>> =C2=A0 UefiCpuPkg/CpuMpPei/CpuMpPei.c=C2=A0 | 8 +++++++- >>> =C2=A0 UefiCpuPkg/CpuMpPei/CpuPaging.c | 9 ++++++++- >>> =C2=A0 3 files changed, 22 insertions(+), 3 deletions(-) >>> >>> diff --git a/UefiCpuPkg/CpuMpPei/CpuBist.c=20 >>> b/UefiCpuPkg/CpuMpPei/CpuBist.c >>> index 7dc93cd784d4..122808139b87 100644 >>> --- a/UefiCpuPkg/CpuMpPei/CpuBist.c >>> +++ b/UefiCpuPkg/CpuMpPei/CpuBist.c >>> @@ -175,7 +175,13 @@ CollectBistDataFromPpi ( >>> =C2=A0=C2=A0=C2=A0 EFI_SEC_PLATFORM_INFORMATION_RECORD2=C2=A0 *Platform= InformationRecord2; >>> =C2=A0=C2=A0=C2=A0 EFI_SEC_PLATFORM_INFORMATION_CPU=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 *CpuInstanceInHob; >>> >>> -=C2=A0 MpInitLibGetNumberOfProcessors (&NumberOfProcessors,=20 >>> &NumberOfEnabledProcessors); >>> +=C2=A0 Status =3D MpInitLibGetNumberOfProcessors (&NumberOfProcessors,= =20 >>> &NumberOfEnabledProcessors); >>> +=C2=A0 ASSERT_EFI_ERROR (Status); >>> + >>> +=C2=A0 if (EFI_ERROR (Status)) { >>> +=C2=A0=C2=A0=C2=A0 NumberOfProcessors=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 =3D 1u; >>> +=C2=A0=C2=A0=C2=A0 NumberOfEnabledProcessors =3D 1u; >> >> Why is '1u' used instead of '1'?=C2=A0 I don't see a lot of usage of the= =20 >> postfix >> unless the const needs to be forced to a larger bitwidth than default=20 >> int type. >> >>> +=C2=A0 } >>> >>> =C2=A0=C2=A0=C2=A0 BistInformationSize =3D sizeof=20 >>> (EFI_SEC_PLATFORM_INFORMATION_RECORD2) + >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 sizeof (EFI_SEC_PLATFORM_INFORMATION_CPU) *=20 >>> NumberOfProcessors; >>> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c=20 >>> b/UefiCpuPkg/CpuMpPei/CpuMpPei.c >>> index e7f1fe9f426c..a84304273168 100644 >>> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c >>> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c >>> @@ -473,7 +473,13 @@ InitializeMpExceptionStackSwitchHandlers ( >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; >>> =C2=A0=C2=A0=C2=A0 } >>> >>> -=C2=A0 MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); >>> +=C2=A0 Status =3D MpInitLibGetNumberOfProcessors (&NumberOfProcessors,= NULL); >>> +=C2=A0 ASSERT_EFI_ERROR (Status); >>> + >>> +=C2=A0 if (EFI_ERROR (Status)) { >>> +=C2=A0=C2=A0=C2=A0 NumberOfProcessors =3D 1u; >>> +=C2=A0 } >>> + >>> =C2=A0=C2=A0=C2=A0 SwitchStackData =3D AllocatePages (EFI_SIZE_TO_PAGES= =20 >>> (NumberOfProcessors * sizeof >>> (EXCEPTION_STACK_SWITCH_CONTEXT))); >>> =C2=A0=C2=A0=C2=A0 ASSERT (SwitchStackData !=3D NULL); >>> =C2=A0=C2=A0=C2=A0 ZeroMem (SwitchStackData, NumberOfProcessors * sizeo= f=20 >>> (EXCEPTION_STACK_SWITCH_CONTEXT)); >>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c=20 >>> b/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> index 135422225340..1322fcb77f28 100644 >>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c >>> @@ -538,6 +538,7 @@ SetupStackGuardPage ( >>> =C2=A0=C2=A0=C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 NumberOfProcessors; >>> =C2=A0=C2=A0=C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Bsp; >>> =C2=A0=C2=A0=C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Index; >>> +=C2=A0 EFI_STATUS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 Status; >>> >>> =C2=A0=C2=A0=C2=A0 // >>> =C2=A0=C2=A0=C2=A0 // One extra page at the bottom of the stack is need= ed for Guard=20 >>> page. >>> @@ -547,7 +548,13 @@ SetupStackGuardPage ( >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ASSERT (FALSE); >>> =C2=A0=C2=A0=C2=A0 } >>> >>> -=C2=A0 MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL); >>> +=C2=A0 Status =3D MpInitLibGetNumberOfProcessors (&NumberOfProcessors,= NULL); >>> +=C2=A0 ASSERT_EFI_ERROR (Status); >>> + >>> +=C2=A0 if (EFI_ERROR (Status)) { >>> +=C2=A0=C2=A0=C2=A0 NumberOfProcessors =3D 1u; >>> +=C2=A0 } >>> + >>> =C2=A0=C2=A0=C2=A0 MpInitLibWhoAmI (&Bsp); >>> =C2=A0=C2=A0=C2=A0 for (Index =3D 0; Index < NumberOfProcessors; ++Inde= x) { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 StackBase =3D 0; >>> --=20 >>> 2.39.2.windows.1 >>> >>> >>> >>> -=3D-=3D-=3D-=3D-=3D-=3D >>> Groups.io Links: You receive all messages sent to this group. >>> View/Reply Online (#101030):=20 >>> https://edk2.groups.io/g/devel/message/101030 >>> Mute This Topic: https://groups.io/mt/97526805/1643496 >>> Group Owner: devel+owner@edk2.groups.io >>> Unsubscribe: https://edk2.groups.io/g/devel/unsub=20 >>> [michael.d.kinney@intel.com] >>> -=3D-=3D-=3D-=3D-=3D-=3D >>> >> >> >> >>=20 >> >>