From: "Michael Kubacki" <mikuback@linux.microsoft.com>
To: Gerd Hoffmann <kraxel@redhat.com>, devel@edk2.groups.io
Subject: Re: [edk2-devel] CodeQL Analysis in edk2
Date: Tue, 27 Feb 2024 11:04:47 -0500 [thread overview]
Message-ID: <80abb140-9a9c-43b8-ba0b-d8ea631d9051@linux.microsoft.com> (raw)
In-Reply-To: <m5d4pkkh3heotifcj33hplpe7ipyviph2badkbzxoom4mnbaw6@pqlbrnfghgfh>
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.
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.
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 (#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]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2024-02-27 16:04 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-07 15:43 [edk2-devel] CodeQL Analysis in edk2 Michael Kubacki
2023-11-13 13:39 ` Laszlo Ersek
2023-11-13 13:42 ` Laszlo Ersek
2023-11-15 0:35 ` Michael Kubacki
2023-11-15 12:00 ` Laszlo Ersek
2024-02-27 11:39 ` Gerd Hoffmann
2024-02-27 16:04 ` Michael Kubacki [this message]
2024-02-28 3:43 ` Laszlo Ersek
2024-02-28 3:55 ` Michael Kubacki
2024-02-28 11:29 ` Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=80abb140-9a9c-43b8-ba0b-d8ea631d9051@linux.microsoft.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox