Re: [edk2-devel] CodeQL Analysis in edk2

["Michael Kubacki" <mikuback@linux.microsoft.com>]

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