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 38D427803E1 for ; Tue, 27 Feb 2024 16:04:51 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=IPXh2l0wZcsQRxhAoDgjOmq6wwou2lpPhJ6RBNkcsEE=; 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=1709049889; v=1; b=UMMH6cJB6lFxvQZm2Fdyz34f+4WaN+R7w1NcbsIXEsHbGEPd7pi4MgBLz1NWFtrxY8wwZa62 nKJq+YkEnSJUhqDVgXFoPaQJIDwq1dSa3/oBx4DGsFuwL1NP8tQxyj0KRJiwT5cyraFNd1aIR6G Zat5AUlrV1l0y136hSHvHKyU= X-Received: by 127.0.0.2 with SMTP id BLVsYY7687511xWZUJ1iLaxl; Tue, 27 Feb 2024 08:04:49 -0800 X-Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.15976.1709049889371759623 for ; Tue, 27 Feb 2024 08:04:49 -0800 X-Received: from [10.0.0.154] (unknown [20.39.63.13]) by linux.microsoft.com (Postfix) with ESMTPSA id AF81820B74C0; Tue, 27 Feb 2024 08:04:48 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com AF81820B74C0 Message-ID: <80abb140-9a9c-43b8-ba0b-d8ea631d9051@linux.microsoft.com> Date: Tue, 27 Feb 2024 11:04:47 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] CodeQL Analysis in edk2 To: Gerd Hoffmann , devel@edk2.groups.io References: 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: 3yR349hM0wwLxI9CO8enn6ucx7686176AA= 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=UMMH6cJB; 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 Hi Gerd, There is a way to suppress results explained here:=20 https://github.com/tianocore/edk2/tree/master/BaseTools/Plugin/CodeQL#filte= r-patterns A real-world example is here:=20 https://github.com/microsoft/mu_basecore/blob/release/202311/CodeQlFilters.= yml That can currently operate at the file and CodeQL rule level=20 granularity. In this case, the null pointer test rule=20 ("cpp/missing-null-test" as shown in=20 https://github.com/tianocore/edk2/security/code-scanning/1277) could be=20 excluded in MpLib.c. --- Taking a quick look at the code highlighted: MpHandOffConfig =3D GetMpHandOffConfigHob (); ASSERT (MpHandOffConfig !=3D NULL); DEBUG (( DEBUG_INFO, "FirstMpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *):=20 %04d\n", MpHandOffConfig->WaitLoopExecutionMode, sizeof (VOID *) )); if (MpHandOffConfig->WaitLoopExecutionMode =3D=3D sizeof (VOID *)) { CodeQL flagged the two MpHandOffConfig dereferences. These is assigned=20 on the return value from GetMpHandOffConfigHob () defined as: /** Get pointer to MP_HAND_OFF_CONFIG GUIDed HOB body. @return The pointer to MP_HAND_OFF_CONFIG structure. **/ MP_HAND_OFF_CONFIG * GetMpHandOffConfigHob ( VOID ) { EFI_HOB_GUID_TYPE *GuidHob; GuidHob =3D GetFirstGuidHob (&mMpHandOffConfigGuid); if (GuidHob =3D=3D NULL) { return NULL; } return (MP_HAND_OFF_CONFIG *)GET_GUID_HOB_DATA (GuidHob); } Can you please provide more context about why you believe a NULL return=20 value from the function is not a consideration? Generally, anything is=20 possible in the HOB list, for example, other code could overflow a HOB=20 boundary and mutate the this HOB's header preventing it from being found. ASSERT() is useful for debug but it's control path is unpredictable in=20 core code based on platform policies that often have varying=20 perspectives of how to apply asserts and how they should be configured=20 in different scenarios. We introduced a "panic library" to use in rare=20 cases where we want more consistent behavior for fatal conditions. Thanks, Michael On 2/27/2024 6:39 AM, Gerd Hoffmann wrote: > Hi, >=20 >> I am hoping we can work together to improve the overall quality of the >> code and minimize the number of CodeQL alerts. >=20 > Seems CodeQL now runs as part of CI and flags issues it has found. >=20 > It complains about a possible NULL pointer dereference: > https://github.com/tianocore/edk2/runs/22021016348 >=20 > This is not correct, but I doubt code analysis will ever be clever > enough to figure this automatically. So I've added an ASSERT() > explicitly saying so, which should help both human reviewers and > code analyzers. >=20 > Apparently that does not change anything for CodeQL though. I guess > 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)? >=20 > thanks & take care, > Gerd -=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 (#116054): https://edk2.groups.io/g/devel/message/116054 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-