On 4/13/19 1:31 AM, Laszlo Ersek wrote:
> RH covscan reports the following "nullptr deref" warning:
>
>> Error: CLANG_WARNING:
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:312:5:
>> warning: Dereference of null pointer
>> # InstanceZero->NumInstancesUnion.NumInstances++;
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:509:7:
>> note: Assuming 'OutCapList' is not equal to NULL
>> # if (OutCapList == NULL) {
>> # ^~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:509:3:
>> note: Taking false branch
>> # if (OutCapList == NULL) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:518:7:
>> note: Assuming the condition is false
>> # if (OutCapList->Capabilities == NULL) {
>> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:518:3:
>> note: Taking false branch
>> # if (OutCapList->Capabilities == NULL) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:529:7:
>> note: Assuming 'CapHdrOffsets' is not equal to NULL
>> # if (CapHdrOffsets == NULL) {
>> # ^~~~~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:529:3:
>> note: Taking false branch
>> # if (CapHdrOffsets == NULL) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:546:3:
>> note: Taking false branch
>> # if (RETURN_ERROR (Status)) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:549:7:
>> note: Assuming the condition is true
>> # if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) {
>> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:549:3:
>> note: Taking true branch
>> # if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:557:5:
>> note: Taking false branch
>> # if (RETURN_ERROR (Status)) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:565:12:
>> note: Assuming 'NormalCapHdrOffset' is > 0
>> # while (NormalCapHdrOffset > 0) {
>> # ^~~~~~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:565:5:
>> note: Loop condition is true. Entering loop body
>> # while (NormalCapHdrOffset > 0) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:570:7:
>> note: Taking false branch
>> # if (RETURN_ERROR (Status)) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:574:16:
>> note: Calling 'InsertPciCap'
>> # Status = InsertPciCap (OutCapList, CapHdrOffsets, PciCapNormal,
>> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:235:3:
>> note: Null pointer value stored to 'InstanceZero'
>> # InstanceZero = NULL;
>> # ^~~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:243:7:
>> note: Assuming 'PciCap' is not equal to NULL
>> # if (PciCap == NULL) {
>> # ^~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:243:3:
>> note: Taking false branch
>> # if (PciCap == NULL) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:259:3:
>> note: Taking false branch
>> # if (RETURN_ERROR (Status)) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:297:3:
>> note: Taking false branch
>> # if (RETURN_ERROR (Status)) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:311:7:
>> note: Assuming the condition is true
>> # if (PciCap->Key.Instance > 0) {
>> # ^~~~~~~~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:311:3:
>> note: Taking true branch
>> # if (PciCap->Key.Instance > 0) {
>> # ^
>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:312:5:
>> note: Dereference of null pointer
>> # InstanceZero->NumInstancesUnion.NumInstances++;
>> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> # 310| //
>> # 311| if (PciCap->Key.Instance > 0) {
>> # 312|-> InstanceZero->NumInstancesUnion.NumInstances++;
>> # 313| }
>> # 314| return RETURN_SUCCESS;
>
> The warning is invalid: the flagged dereferencing of "InstanceZero" is
> gated by a condition that is only satisfied if we dereference
> "InstanceZero" *first*.
>
> (Perhaps the analyzer assumes that the OrderedCollectionInsert() call,
> just before line 259, can change the value of "PciCap->Key.Instance" via
> the last argument:
>
> 254 //
> 255 // Add PciCap to CapList.
> 256 //
> 257 Status = OrderedCollectionInsert (CapList->Capabilities,
> &PciCapEntry,
> 258 PciCap);
> 259 if (RETURN_ERROR (Status)) {
>
> That assumption is incorrect.)
>
> Add a comment and an ASSERT().
>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Jordan Justen <[email protected]>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710
> Issue: scan-0994.txt
> Signed-off-by: Laszlo Ersek <[email protected]>
> ---
> OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> index 4968d2b478db..c6f2c726509f 100644
> --- a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> +++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c
> @@ -298,16 +298,23 @@ InsertPciCap (
> goto DeletePciCapFromCapList;
> }
>
> //
> // Now we can bump the instance count maintained in Instance#0, if PciCap
> is
> // not the first instance of (Domain, CapId).
> //
> if (PciCap->Key.Instance > 0) {
> + //
> + // Suppress invalid "nullptr dereference" compiler/analyzer warnings: the
> + // only way for "PciCap->Key.Instance" to be positive here is for it to
> + // have been assigned *from* dereferencing "InstanceZero" above.
> + //
> + ASSERT (InstanceZero != NULL);
What about adding a STATIC_ANALYZER_HINT() only defined by covscan?
STATIC_ANALYZER_HINT (InstanceZero != NULL);
Regardless:
Reviewed-by: Philippe Mathieu-Daude <[email protected]>
> +
> InstanceZero->NumInstancesUnion.NumInstances++;
> }
> return RETURN_SUCCESS;
>
> DeletePciCapFromCapList:
> OrderedCollectionDelete (CapList->Capabilities, PciCapEntry, NULL);
>
> FreePciCap:
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39109): https://edk2.groups.io/g/devel/message/39109
Mute This Topic: https://groups.io/mt/31070310/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
