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 80E29D8066D for ; Wed, 28 Feb 2024 03:43:48 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=/k+F99iA/9biGOcwBPkQ+nzQ9531XgXP2hTimMGrji4=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version: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=1709091827; v=1; b=IL8CjQzCrmN+gGxGPsSuw/QVkKFi4xDxrSHMZFtMzV/4hP9Oj2tijqciUOdOM97ZbrBHLVDt fGUTgFxthPp1/O2QcA5lkce+w48k3KIPg9cJAQnRukKSF/m3eXM3WdyJs1x5Ah2a72NSjRtwaJj nBgnf7AGynqna2HO5gZbxlxs= X-Received: by 127.0.0.2 with SMTP id s0eFYY7687511xbqpVR3ynhN; Tue, 27 Feb 2024 19:43:47 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.5655.1709091826461786604 for ; Tue, 27 Feb 2024 19:43:46 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-549-zdNlMTyKMTa74qVlxrCiOw-1; Tue, 27 Feb 2024 22:43:42 -0500 X-MC-Unique: zdNlMTyKMTa74qVlxrCiOw-1 X-Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3FAE2185A781; Wed, 28 Feb 2024 03:43:42 +0000 (UTC) X-Received: from [10.39.192.46] (unknown [10.39.192.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5E42D1121312; Wed, 28 Feb 2024 03:43:41 +0000 (UTC) Message-ID: Date: Wed, 28 Feb 2024 04:43:40 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] CodeQL Analysis in edk2 To: devel@edk2.groups.io, mikuback@linux.microsoft.com, Gerd Hoffmann References: <80abb140-9a9c-43b8-ba0b-d8ea631d9051@linux.microsoft.com> From: "Laszlo Ersek" In-Reply-To: <80abb140-9a9c-43b8-ba0b-d8ea631d9051@linux.microsoft.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Viv9OWGjzGrJxDBJ5v1oM60Cx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=IL8CjQzC; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) 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#filter-patterns > > A real-world example is here: > https://github.com/microsoft/mu_basecore/blob/release/202311/CodeQlFilters.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: > >     MpHandOffConfig = GetMpHandOffConfigHob (); >     ASSERT (MpHandOffConfig != NULL); >     DEBUG (( >       DEBUG_INFO, >       "FirstMpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *): > %04d\n", >       MpHandOffConfig->WaitLoopExecutionMode, >       sizeof (VOID *) >       )); >     if (MpHandOffConfig->WaitLoopExecutionMode == sizeof (VOID *)) { > > CodeQL flagged the two MpHandOffConfig dereferences. These is assigned > 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 = GetFirstGuidHob (&mMpHandOffConfigGuid); >   if (GuidHob == 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 > 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. The GetMpHandOffConfigHob() function itself is right, when viewed in isolation, to deal with the possibility of the HOB missing. However, the GetMpHandOffConfigHob() *call site* is only reached if the "FirstMpHandOff" variable is not NULL. 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. In other words, the presence of at least one MpHandOff GUID HOB guarantees that there is going to be exactly one MpHandOffConfig GUID HOB. Therefore the ASSERT() is right -- it expresses an invariant. That said: > > 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. I agree that making this more explicit wouldn't hurt. The panic library is not upstream (yet? :)), so I'll suggest a CpuDeadLoop() under the patch itself: [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 Thanks, Laszlo > > Thanks, > Michael > > On 2/27/2024 6:39 AM, Gerd Hoffmann wrote: >>    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.  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.  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)? >> >> thanks & take care, >>    Gerd > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116076): https://edk2.groups.io/g/devel/message/116076 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] -=-=-=-=-=-=-=-=-=-=-=-