From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id AF9F9AC0A69 for ; Wed, 28 Feb 2024 03:55:16 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=pSnrhurOWSsiVA7bpFxvxCnvl5Hx3u/BPte+Lc0ZnJA=; c=relaxed/simple; d=groups.io; h=DKIM-Filter:Message-ID:Date:MIME-Version:User-Agent:Subject:To:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1709092515; v=1; b=FMfpbwH0ijEJPxnnfSZsqJfpx4//H5abgRlJW87TfQSJAZv/q25PhgjCg8YEgIkVnpELSKOh xyAvXcH89W7HKt+3Ma4CAgQBCxTJ+SRPx1M/xFfLrdtxShPNCkphUhEz9QUM60ZDJn/a4ORSwGZ s5X1I1R9NhOfYTmIe5cRWdpE= X-Received: by 127.0.0.2 with SMTP id wHkwYY7687511xUrAJ1XZYEh; Tue, 27 Feb 2024 19:55:15 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.5824.1709092514846249515 for ; Tue, 27 Feb 2024 19:55:14 -0800 X-Received: from [10.0.0.154] (unknown [20.39.63.12]) by linux.microsoft.com (Postfix) with ESMTPSA id F13D420B74C0; Tue, 27 Feb 2024 19:55:13 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com F13D420B74C0 Message-ID: Date: Tue, 27 Feb 2024 22:55:13 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] CodeQL Analysis in edk2 To: Laszlo Ersek , devel@edk2.groups.io, Gerd Hoffmann , Kenneth Lautner References: <80abb140-9a9c-43b8-ba0b-d8ea631d9051@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,mikuback@linux.microsoft.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: yyhVho72BYID1Y7967qpuTtIx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=FMfpbwH0; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=linux.microsoft.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 2/27/2024 10:43 PM, Laszlo Ersek wrote: > On 2/27/24 17:04, Michael Kubacki wrote: >> Hi Gerd, >> >> There is a way to suppress results explained here: >> https://github.com/tianocore/edk2/tree/master/BaseTools/Plugin/CodeQL#fi= lter-patterns >> >> A real-world example is here: >> https://github.com/microsoft/mu_basecore/blob/release/202311/CodeQlFilte= rs.yml >> >> That can currently operate at the file and CodeQL rule level >> granularity. In this case, the null pointer test rule >> ("cpp/missing-null-test" as shown in >> https://github.com/tianocore/edk2/security/code-scanning/1277) could be >> excluded in MpLib.c. >> >> --- >> >> Taking a quick look at the code highlighted: >> >> =C2=A0=C2=A0=C2=A0 MpHandOffConfig =3D GetMpHandOffConfigHob (); >> =C2=A0=C2=A0=C2=A0 ASSERT (MpHandOffConfig !=3D NULL); >> =C2=A0=C2=A0=C2=A0 DEBUG (( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG_INFO, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "FirstMpHandOff->WaitLoopExecutionMode: = %04d, sizeof (VOID *): >> %04d\n", >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MpHandOffConfig->WaitLoopExecutionMode, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sizeof (VOID *) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 )); >> =C2=A0=C2=A0=C2=A0 if (MpHandOffConfig->WaitLoopExecutionMode =3D=3D si= zeof (VOID *)) { >> >> CodeQL flagged the two MpHandOffConfig dereferences. These is assigned >> on the return value from GetMpHandOffConfigHob () defined as: >> >> /** >> =C2=A0 Get pointer to MP_HAND_OFF_CONFIG GUIDed HOB body. >> >> =C2=A0 @return=C2=A0 The pointer to MP_HAND_OFF_CONFIG structure. >> **/ >> MP_HAND_OFF_CONFIG * >> GetMpHandOffConfigHob ( >> =C2=A0 VOID >> =C2=A0 ) >> { >> =C2=A0 EFI_HOB_GUID_TYPE=C2=A0 *GuidHob; >> >> =C2=A0 GuidHob =3D GetFirstGuidHob (&mMpHandOffConfigGuid); >> =C2=A0 if (GuidHob =3D=3D NULL) { >> =C2=A0=C2=A0=C2=A0 return NULL; >> =C2=A0 } >> >> =C2=A0 return (MP_HAND_OFF_CONFIG *)GET_GUID_HOB_DATA (GuidHob); >> } >> >> Can you please provide more context about why you believe a NULL return >> value from the function is not a consideration? Generally, anything is >> possible in the HOB list, for example, other code could overflow a HOB >> boundary and mutate the this HOB's header preventing it from being found= . >=20 > The GetMpHandOffConfigHob() function itself is right, when viewed in > isolation, to deal with the possibility of the HOB missing. >=20 > However, the GetMpHandOffConfigHob() *call site* is only reached if the > "FirstMpHandOff" variable is not NULL. >=20 > And if "FirstMpHandOff" is not NULL, then in the PEI phase, the > SaveCpuMpData() function [UefiCpuPkg/Library/MpInitLib/PeiMpLib.c] will > have prepared *both* at least one MpHandOff GUID HOB, *and* the sole > MpHandOffConfig GUID HOB. >=20 > In other words, the presence of at least one MpHandOff GUID HOB > guarantees that there is going to be exactly one MpHandOffConfig GUID HOB= . >=20 > Therefore the ASSERT() is right -- it expresses an invariant. >=20 Thanks. With those details, I agree ASSERT() is reasonable. > That said: >=20 >> >> ASSERT() is useful for debug but it's control path is unpredictable in >> core code based on platform policies that often have varying >> perspectives of how to apply asserts and how they should be configured >> in different scenarios. We introduced a "panic library" to use in rare >> cases where we want more consistent behavior for fatal conditions. >=20 > I agree that making this more explicit wouldn't hurt. >=20 > The panic library is not upstream (yet? :)) I added Ken from my team as a reminder to prepare that for upstream soon. , so I'll suggest a > CpuDeadLoop() under the patch itself: >=20 > [PATCH 1/1] UefiCpuPkg/MpInitLib: add struct MP_HAND_OFF_CONFIG > msgid <20240227114122.1140614-1-kraxel@redhat.com> > https://edk2.groups.io/g/devel/message/116029 >=20 > Thanks, > Laszlo >=20 >> >> Thanks, >> Michael >> >> On 2/27/2024 6:39 AM, Gerd Hoffmann wrote: >>> =C2=A0=C2=A0 Hi, >>> >>>> I am hoping we can work together to improve the overall quality of the >>>> code and minimize the number of CodeQL alerts. >>> >>> Seems CodeQL now runs as part of CI and flags issues it has found. >>> >>> It complains about a possible NULL pointer dereference: >>> https://github.com/tianocore/edk2/runs/22021016348 >>> >>> This is not correct, but I doubt code analysis will ever be clever >>> enough to figure this automatically.=C2=A0 So I've added an ASSERT() >>> explicitly saying so, which should help both human reviewers and >>> code analyzers. >>> >>> Apparently that does not change anything for CodeQL though.=C2=A0 I gue= ss >>> the CodeQL config must be updated so it knows what ASSERT() means? >>> Maybe it is ignored simply because it is upper case (unlike the >>> standard C library version which is lower case)? >>> >>> thanks & take care, >>> =C2=A0=C2=A0 Gerd >> >> >>=20 >> >> -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116078): https://edk2.groups.io/g/devel/message/116078 Mute This Topic: https://groups.io/mt/102444916/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-