[edk2-devel] CodeQL Analysis in edk2

[edk2-devel] CodeQL Analysis in edk2

[Michael Kubacki]

[-- Attachment #1.1: Type: text/plain, Size: 2665 bytes --]

The series that makes it easy to run CodeQL locally and have access to results from any PR or push to master.

Those that have access can see the results directly in "Code Scanning" in the "Security" tab of the edk2 repo. That may be affected in times like freezes when permissions are adjusted (write permission is needed).

I am hoping we can work together to improve the overall quality of the code and minimize the number of CodeQL alerts.

This is an example of that interface:

*Overview of Issues (many)*

*Example of Details for a Specific Issue*

*---*

*However, you can always download the results for an individual package* from its GitHub Action run. I encourage people to do so.

1. Go to Actions -> CodeQL ( https://github.com/tianocore/edk2/actions/workflows/codeql.yml ) (https://github.com/tianocore/edk2/actions/workflows/codeql.yml). Anything to "master" are results at that point in time on the master branch. Individual PR branches are shown to get results for a specific PR.

2. Download and open the SARIF file for a package. In the commit to master shown above in https://github.com/tianocore/edk2/actions/runs/6779575049, for MdeModulePkg, I would download "MdeModulePkg-CodeQL-SARIF" and unzip.

3. Open the SARIF file to view results. For example, drag/drop the file "codeql-db-mdemodulepkg-debug-0.sarif" into VS Code with the "SARIF Viewer" ( https://marketplace.visualstudio.com/items?itemName=MS-SarifVSCode.sarif-viewer ) installed. It shows all of the issues by file or rule with click to the problem and more details about it. There are other SARIF viewers available as well.

Keep in mind that CodeQL will often not highlight everything that needs to be done to fix an issue. It alerts the developer to an issue and then you need to inspect the code to determine if other code paths or refactoring should be applied.

I will create a wiki page with more user focused information, but I wanted to share some quick info for getting started.

More technical details about how the plugin itself works and applying exceptions are available in its readme - edk2/BaseTools/Plugin/CodeQL/Readme.md at master · tianocore/edk2 (github.com). ( https://github.com/tianocore/edk2/blob/master/BaseTools/Plugin/CodeQL/Readme.md )

Thanks,
Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110855): https://edk2.groups.io/g/devel/message/110855
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #1.2: Type: text/html, Size: 3943 bytes --]

[-- Attachment #2: dummyfile.0.part --]
[-- Type: image/png, Size: 77392 bytes --]

[-- Attachment #3: dummyfile.1.part --]
[-- Type: image/png, Size: 110086 bytes --]

[-- Attachment #4: dummyfile.2.part --]
[-- Type: image/png, Size: 134058 bytes --]

[-- Attachment #5: dummyfile.3.part --]
[-- Type: image/png, Size: 92021 bytes --]

[-- Attachment #6: dummyfile.4.part --]
[-- Type: image/png, Size: 297236 bytes --]

Re: [edk2-devel] CodeQL Analysis in edk2

[Laszlo Ersek]

On 11/7/23 16:43, Michael Kubacki wrote:
> The series that makes it easy to run CodeQL locally and have access to
> results from any PR or push to master.
> 
> Those that have access can see the results directly in "Code Scanning"
> in the "Security" tab of the edk2 repo. That may be affected in times
> like freezes when permissions are adjusted (write permission is needed).
> 
> I am hoping we can work together to improve the overall quality of the
> code and minimize the number of CodeQL alerts.
> 
> This is an example of that interface:
> 
> *Overview of Issues (many)*
> 
> 
> *Example of Details for a Specific Issue*
> 
> *---*
> 
> *However, you can always download the results for an individual package*
> from its GitHub Action run. I encourage people to do so.
> 
> 1. Go to Actions -> CodeQL
> <https://github.com/tianocore/edk2/actions/workflows/codeql.yml>
> (https://github.com/tianocore/edk2/actions/workflows/codeql.yml).
> Anything to "master" are results at that point in time on the master
> branch. Individual PR branches are shown to get results for a specific PR.
> 
> 
> 
> 2. Download and open the SARIF file for a package. In the commit to
> master shown above in
> https://github.com/tianocore/edk2/actions/runs/6779575049, for
> MdeModulePkg, I would download "MdeModulePkg-CodeQL-SARIF" and unzip.
> 
> 
> 
> 3. Open the SARIF file to view results. For example, drag/drop the file
> "codeql-db-mdemodulepkg-debug-0.sarif" into VS Code with the "SARIF
> Viewer"
> <https://marketplace.visualstudio.com/items?itemName=MS-SarifVSCode.sarif-viewer> installed. It shows all of the issues by file or rule with click to the problem and more details about it. There are other SARIF viewers available as well.

I've investigated "sarif", from "sarif-tools version 2.0.0", at <https://github.com/microsoft/sarif-tools>.

The "emacs" output module of "sarif" would be ideal for my needs, but I have two questions / requests regarding that:

- would it be possible to run "sarif emacs" immediately in the github action, so that the text file can be downloaded at once? (I currently have sarif-tools installed in a python venv, but I'd prefer avoiding even that.)

- the "sarif emacs" output seems a bit broken, actually, so it's not usable. Consider the following entry from the original JSON file:

    }, {
      "ruleId" : "cpp/missing-null-test",
      "ruleIndex" : 0,
      "rule" : {
        "id" : "cpp/missing-null-test",
        "index" : 0
      },
      "message" : {
        "text" : "Value may be null; it should be checked before dereferencing."
      },
      "locations" : [ {
        "physicalLocation" : {
          "artifactLocation" : {
            "uri" : "MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c",
            "uriBaseId" : "%SRCROOT%",
            "index" : 0
          },
          "region" : {
            "startLine" : 355,
            "startColumn" : 48,
            "endColumn" : 52
          }
        }
      } ],
      "partialFingerprints" : {
        "primaryLocationLineHash" : "f374f6e6dfc92010:1",
        "primaryLocationStartColumnFingerprint" : "43"
      }
    }, {

In the "emacs" output, it appears as:

--------
ModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c:355: cpp/missing-null-test Value may be null; it should be checked before dereferencing.
--------

Note that the first three characters, "Mde" of "Mde" are lost.

This issue (first three chars cut) affects all other pathnames in the emacs output too.

Is this a known issue perhaps?

Thanks!
Laszlo

> 
> 
> 
> Keep in mind that CodeQL will often not highlight everything that needs
> to be done to fix an issue. It alerts the developer to an issue and then
> you need to inspect the code to determine if other code paths or
> refactoring should be applied.
> 
> I will create a wiki page with more user focused information, but I
> wanted to share some quick info for getting started.
> 
> More technical details about how the plugin itself works and applying
> exceptions are available in its readme
> - edk2/BaseTools/Plugin/CodeQL/Readme.md at master · tianocore/edk2
> (github.com).
> <https://github.com/tianocore/edk2/blob/master/BaseTools/Plugin/CodeQL/Readme.md>
> 
> Thanks,
> Michael
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111153): https://edk2.groups.io/g/devel/message/111153
Mute This Topic: https://groups.io/mt/102444916/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL Analysis in edk2

[Laszlo Ersek]

sorry, unfinished thought:

On 11/13/23 14:39, Laszlo Ersek wrote:

> - the "sarif emacs" output seems a bit broken, actually, so it's not usable. Consider the following entry from the original JSON file:
> 
>     }, {
>       "ruleId" : "cpp/missing-null-test",
>       "ruleIndex" : 0,
>       "rule" : {
>         "id" : "cpp/missing-null-test",
>         "index" : 0
>       },
>       "message" : {
>         "text" : "Value may be null; it should be checked before dereferencing."
>       },
>       "locations" : [ {
>         "physicalLocation" : {
>           "artifactLocation" : {
>             "uri" : "MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c",
>             "uriBaseId" : "%SRCROOT%",
>             "index" : 0
>           },
>           "region" : {
>             "startLine" : 355,
>             "startColumn" : 48,
>             "endColumn" : 52
>           }
>         }
>       } ],
>       "partialFingerprints" : {
>         "primaryLocationLineHash" : "f374f6e6dfc92010:1",
>         "primaryLocationStartColumnFingerprint" : "43"
>       }
>     }, {
> 
> In the "emacs" output, it appears as:
> 
> --------
> ModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c:355: cpp/missing-null-test Value may be null; it should be checked before dereferencing.
> --------
> 
> Note that the first three characters, "Mde" of "Mde" are lost.

I meant '"Mde" of "ModulePkg"'.

> 
> This issue (first three chars cut) affects all other pathnames in the emacs output too.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111154): https://edk2.groups.io/g/devel/message/111154
Mute This Topic: https://groups.io/mt/102444916/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL Analysis in edk2

[Michael Kubacki]

On 11/13/2023 8:42 AM, Laszlo Ersek wrote:
> sorry, unfinished thought:
> 
> On 11/13/23 14:39, Laszlo Ersek wrote:
> 
>> - the "sarif emacs" output seems a bit broken, actually, so it's not usable. Consider the following entry from the original JSON file:
>>
>>      }, {
>>        "ruleId" : "cpp/missing-null-test",
>>        "ruleIndex" : 0,
>>        "rule" : {
>>          "id" : "cpp/missing-null-test",
>>          "index" : 0
>>        },
>>        "message" : {
>>          "text" : "Value may be null; it should be checked before dereferencing."
>>        },
>>        "locations" : [ {
>>          "physicalLocation" : {
>>            "artifactLocation" : {
>>              "uri" : "MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c",
>>              "uriBaseId" : "%SRCROOT%",
>>              "index" : 0
>>            },
>>            "region" : {
>>              "startLine" : 355,
>>              "startColumn" : 48,
>>              "endColumn" : 52
>>            }
>>          }
>>        } ],
>>        "partialFingerprints" : {
>>          "primaryLocationLineHash" : "f374f6e6dfc92010:1",
>>          "primaryLocationStartColumnFingerprint" : "43"
>>        }
>>      }, {
>>
>> In the "emacs" output, it appears as:
>>
>> --------
>> ModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c:355: cpp/missing-null-test Value may be null; it should be checked before dereferencing.
>> --------
>>
>> Note that the first three characters, "Mde" of "Mde" are lost.
> 
> I meant '"Mde" of "ModulePkg"'.
> 
I was able to reproduce this with sarif-tools version 2.0.0.

It impacted other commands like "html" as well.

Applying the "--no-autotrim" option appears to leave the path alone. Can 
you please let me know if that works for you?

Also, yes, I can add this to the CodeQL GitHub workflow.

>>
>> This issue (first three chars cut) affects all other pathnames in the emacs output too.
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111223): https://edk2.groups.io/g/devel/message/111223
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL Analysis in edk2

[Laszlo Ersek]

On 11/15/23 01:35, Michael Kubacki wrote:
> On 11/13/2023 8:42 AM, Laszlo Ersek wrote:
>> sorry, unfinished thought:
>>
>> On 11/13/23 14:39, Laszlo Ersek wrote:
>>
>>> - the "sarif emacs" output seems a bit broken, actually, so it's not
>>> usable. Consider the following entry from the original JSON file:
>>>
>>>      }, {
>>>        "ruleId" : "cpp/missing-null-test",
>>>        "ruleIndex" : 0,
>>>        "rule" : {
>>>          "id" : "cpp/missing-null-test",
>>>          "index" : 0
>>>        },
>>>        "message" : {
>>>          "text" : "Value may be null; it should be checked before
>>> dereferencing."
>>>        },
>>>        "locations" : [ {
>>>          "physicalLocation" : {
>>>            "artifactLocation" : {
>>>              "uri" :
>>> "MdeModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c",
>>>              "uriBaseId" : "%SRCROOT%",
>>>              "index" : 0
>>>            },
>>>            "region" : {
>>>              "startLine" : 355,
>>>              "startColumn" : 48,
>>>              "endColumn" : 52
>>>            }
>>>          }
>>>        } ],
>>>        "partialFingerprints" : {
>>>          "primaryLocationLineHash" : "f374f6e6dfc92010:1",
>>>          "primaryLocationStartColumnFingerprint" : "43"
>>>        }
>>>      }, {
>>>
>>> In the "emacs" output, it appears as:
>>>
>>> --------
>>> ModulePkg/Application/UiApp/FrontPageCustomizedUiSupport.c:355:
>>> cpp/missing-null-test Value may be null; it should be checked before
>>> dereferencing.
>>> --------
>>>
>>> Note that the first three characters, "Mde" of "Mde" are lost.
>>
>> I meant '"Mde" of "ModulePkg"'.
>>
> I was able to reproduce this with sarif-tools version 2.0.0.
> 
> It impacted other commands like "html" as well.
> 
> Applying the "--no-autotrim" option appears to leave the path alone. Can
> you please let me know if that works for you?

Yes, it does!

Thanks!
Laszlo

> 
> Also, yes, I can add this to the CodeQL GitHub workflow.
> 
>>>
>>> This issue (first three chars cut) affects all other pathnames in the
>>> emacs output too.
>>
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111266): https://edk2.groups.io/g/devel/message/111266
Mute This Topic: https://groups.io/mt/102444916/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL Analysis in edk2

[Gerd Hoffmann]

  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 (#116028): https://edk2.groups.io/g/devel/message/116028
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL Analysis in edk2

[Michael Kubacki]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL Analysis in edk2

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL Analysis in edk2

[Michael Kubacki]

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#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.
> 
Thanks. With those details, I agree ASSERT() is reasonable.

> 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? :))

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:
> 
> [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 (#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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL Analysis in edk2

[Gerd Hoffmann]

On Tue, Feb 27, 2024 at 11:04:47AM -0500, Michael Kubacki wrote:
> Hi Gerd,
> 
> 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.

CodeQL apparently has support for assertions[1].  The documentation
sounds like this can be extended.  So maybe it is possible to add an
'Edk2Assert' class, to have CodeQL recognize ASSERT() + variants in the
edk2 source code?

That should reduce the number of filter rules needed and simplify
maintenance long-term.

take care,
  Gerd

[1] https://codeql.github.com/codeql-standard-libraries/cpp/semmle/code/cpp/commons/Assertions.qll/module.Assertions.html



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116112): https://edk2.groups.io/g/devel/message/116112
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]
-=-=-=-=-=-=-=-=-=-=-=-