[edk2-devel] [PATCH v3 0/2] BZ 4212: Fix MdeModulePkg/Bus/Pci/PciHostBridgeDxe issues pointed by Coverity

[edk2-devel] [PATCH v3 0/2] BZ 4212: Fix MdeModulePkg/Bus/Pci/PciHostBridgeDxe issues pointed by Coverity

[Ranbir Singh]

v2 -> v3:
  - Rebased to latest master HEAD
  - Replaced continue with CpuDeadLoop wrt OVERRUN issue
  - Changed to more generic solution wrt MISSING_BREAK issue

v1 -> v2:
  - Rebased to latest master HEAD
  - Changed the commit message wrt MISSING_BREAK issues
  - Cc update

Ranbir Singh (2):
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity
    issue

 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 65 ++++++++++----------
 1 file changed, 31 insertions(+), 34 deletions(-)

-- 
2.34.1



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

[edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function NotifyPhase has a check

    ASSERT (Index < TypeMax);

but this comes into play only in DEBUG mode. In Release mode, there is
no handling if the Index value is within array limits or not. If for
whatever reasons, the Index does not get re-assigned to Index2 at line
937, then it remains at TypeMax as assigned earlier at line 929. This
poses array overrun risk at lines 942 and 943. It is better to deploy
a safety check on Index limit before accessing array elements.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212

Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index d573e532bac8..c2c143068cd2 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -939,6 +939,11 @@ NotifyPhase (
             }
 
             ASSERT (Index < TypeMax);
+
+            if (Index == TypeMax) {
+              CpuDeadLoop ();
+            }
+
             ResNodeHandled[Index] = TRUE;
             Alignment             = RootBridge->ResAllocNode[Index].Alignment;
             BitsOfAlignment       = LowBitSet64 (Alignment + 1);
-- 
2.34.1



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

[edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function SubmitResources has a switch-case code in which the
case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check.

While this may be intentional, it may not be evident to any general code
reader/developer or static analyis tool why there is no break in between.

SubmitResources function is supposed to handle only Mem or IO resources.
So, update the ResType parameter check reflecting that and re-model the
switch-case code in contention using just one if condition to further
handle other parameter checks specific to ACPI_ADDRESS_SPACE_TYPE_MEM.
This leads to mostly indentation level code changes. Few ASSERT's later
present are henceforth not required.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212

Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60 +++++++++-----------
 1 file changed, 26 insertions(+), 34 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index c2c143068cd2..ed0aa455bfd4 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1453,7 +1453,7 @@ SetBusNumbers (
   Submits the I/O and memory resource requirements for the specified PCI Root Bridge.
 
   @param This              The EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_ PROTOCOL instance.
-  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory resource requirements.
+  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory resource requirements
                            are being submitted.
   @param Configuration     The pointer to the PCI I/O and PCI memory resource descriptor.
 
@@ -1496,7 +1496,7 @@ SubmitResources (
       // descriptors are ignored and the function returns EFI_INVALID_PARAMETER.
       //
       for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
-        if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) {
+        if ((Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM) && (Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_IO)) {
           return EFI_INVALID_PARAMETER;
         }
 
@@ -1509,40 +1509,34 @@ SubmitResources (
           (Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0 ? L" (Prefetchable)" : L""
           ));
         DEBUG ((DEBUG_INFO, "      Length/Alignment = 0x%lx / 0x%lx\n", Descriptor->AddrLen, Descriptor->AddrRangeMax));
-        switch (Descriptor->ResType) {
-          case ACPI_ADDRESS_SPACE_TYPE_MEM:
-            if ((Descriptor->AddrSpaceGranularity != 32) && (Descriptor->AddrSpaceGranularity != 64)) {
-              return EFI_INVALID_PARAMETER;
-            }
 
-            if ((Descriptor->AddrSpaceGranularity == 32) && (Descriptor->AddrLen >= SIZE_4GB)) {
-              return EFI_INVALID_PARAMETER;
-            }
+        if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
+          if ((Descriptor->AddrSpaceGranularity != 32) && (Descriptor->AddrSpaceGranularity != 64)) {
+            return EFI_INVALID_PARAMETER;
+          }
 
-            //
-            // If the PCI root bridge does not support separate windows for nonprefetchable and
-            // prefetchable memory, then the PCI bus driver needs to include requests for
-            // prefetchable memory in the nonprefetchable memory pool.
-            //
-            if (((RootBridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0) &&
-                ((Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0)
-                )
-            {
-              return EFI_INVALID_PARAMETER;
-            }
+          if ((Descriptor->AddrSpaceGranularity == 32) && (Descriptor->AddrLen >= SIZE_4GB)) {
+            return EFI_INVALID_PARAMETER;
+          }
 
-          case ACPI_ADDRESS_SPACE_TYPE_IO:
-            //
-            // Check aligment, it should be of the form 2^n-1
-            //
-            if (GetPowerOfTwo64 (Descriptor->AddrRangeMax + 1) != (Descriptor->AddrRangeMax + 1)) {
-              return EFI_INVALID_PARAMETER;
-            }
+          //
+          // If the PCI root bridge does not support separate windows for nonprefetchable and
+          // prefetchable memory, then the PCI bus driver needs to include requests for
+          // prefetchable memory in the nonprefetchable memory pool.
+          //
+          if (((RootBridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0) &&
+              ((Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0)
+              )
+          {
+            return EFI_INVALID_PARAMETER;
+          }
+        }
 
-            break;
-          default:
-            ASSERT (FALSE);
-            break;
+        //
+        // Check aligment, it should be of the form 2^n-1
+        //
+        if (GetPowerOfTwo64 (Descriptor->AddrRangeMax + 1) != (Descriptor->AddrRangeMax + 1)) {
+          return EFI_INVALID_PARAMETER;
         }
       }
 
@@ -1559,7 +1553,6 @@ SubmitResources (
               Type = TypeMem32;
             }
           } else {
-            ASSERT (Descriptor->AddrSpaceGranularity == 64);
             if ((Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) {
               Type = TypePMem64;
             } else {
@@ -1567,7 +1560,6 @@ SubmitResources (
             }
           }
         } else {
-          ASSERT (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO);
           Type = TypeIo;
         }
 
-- 
2.34.1



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

Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Michael D Kinney]

Hi Ranbir,

A deadloop without even a debug print is not good behavior.

If this condition really represents a condition where it is not possible
to complete the PCI resource allocation/assignment, then an error status
code should be returned to the caller of NotifyPhase().  Perhaps
EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API should
likely be updated to do the same.

This may also require the caller of this service, the PCI Bus Driver,
to be reviewed to make sure it handles error conditions from NotifyPhase().

I recommend you get help on the proposed code changes from the PCI
subsystem maintainers.

Thanks,

Mike



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
> Singh
> Sent: Thursday, November 9, 2023 9:39 AM
> To: devel@edk2.groups.io; rsingh@ventanamicro.com
> Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli
> <veeresh.sangolli@dellteam.com>
> Subject: [edk2-devel] [PATCH v3 1/2]
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
> 
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function NotifyPhase has a check
> 
>     ASSERT (Index < TypeMax);
> 
> but this comes into play only in DEBUG mode. In Release mode, there is
> no handling if the Index value is within array limits or not. If for
> whatever reasons, the Index does not get re-assigned to Index2 at line
> 937, then it remains at TypeMax as assigned earlier at line 929. This
> poses array overrun risk at lines 942 and 943. It is better to deploy
> a safety check on Index limit before accessing array elements.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index d573e532bac8..c2c143068cd2 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -939,6 +939,11 @@ NotifyPhase (
>              }
> 
> 
> 
>              ASSERT (Index < TypeMax);
> 
> +
> 
> +            if (Index == TypeMax) {
> 
> +              CpuDeadLoop ();
> 
> +            }
> 
> +
> 
>              ResNodeHandled[Index] = TRUE;
> 
>              Alignment             = RootBridge-
> >ResAllocNode[Index].Alignment;
> 
>              BitsOfAlignment       = LowBitSet64 (Alignment + 1);
> 
> --
> 2.34.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#110993):
> https://edk2.groups.io/g/devel/message/110993
> Mute This Topic: https://groups.io/mt/102490513/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110999): https://edk2.groups.io/g/devel/message/110999
Mute This Topic: https://groups.io/mt/102490513/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] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 4485 bytes --]

As far as I know, from a secure coding perspective, it would be recommended
that array overrun condition check is captured in the code even if it is
felt that it will never hit.

Generally speaking, I won't be in favour of handling other ASSERT
conditions updates even if required if they are not related to array
overrun conditions i.e., the context of the patch.

If someone / PCI maintainers can advise in this patch context what should
be done in the array overrun condition, I will be happy to update,
otherwise, sorry to say I won't be able to pursue this particular one
further and hence would be leaving the related code with the status quo
here.

On Fri, Nov 10, 2023 at 2:10 AM Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> Hi Ranbir,
>
> A deadloop without even a debug print is not good behavior.
>
> If this condition really represents a condition where it is not possible
> to complete the PCI resource allocation/assignment, then an error status
> code should be returned to the caller of NotifyPhase().  Perhaps
> EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API should
> likely be updated to do the same.
>
> This may also require the caller of this service, the PCI Bus Driver,
> to be reviewed to make sure it handles error conditions from NotifyPhase().
>
> I recommend you get help on the proposed code changes from the PCI
> subsystem maintainers.
>
> Thanks,
>
> Mike
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
> > Singh
> > Sent: Thursday, November 9, 2023 9:39 AM
> > To: devel@edk2.groups.io; rsingh@ventanamicro.com
> > Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli
> > <veeresh.sangolli@dellteam.com>
> > Subject: [edk2-devel] [PATCH v3 1/2]
> > MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
> >
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function NotifyPhase has a check
> >
> >     ASSERT (Index < TypeMax);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there is
> > no handling if the Index value is within array limits or not. If for
> > whatever reasons, the Index does not get re-assigned to Index2 at line
> > 937, then it remains at TypeMax as assigned earlier at line 929. This
> > poses array overrun risk at lines 942 and 943. It is better to deploy
> > a safety check on Index limit before accessing array elements.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > index d573e532bac8..c2c143068cd2 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > @@ -939,6 +939,11 @@ NotifyPhase (
> >              }
> >
> >
> >
> >              ASSERT (Index < TypeMax);
> >
> > +
> >
> > +            if (Index == TypeMax) {
> >
> > +              CpuDeadLoop ();
> >
> > +            }
> >
> > +
> >
> >              ResNodeHandled[Index] = TRUE;
> >
> >              Alignment             = RootBridge-
> > >ResAllocNode[Index].Alignment;
> >
> >              BitsOfAlignment       = LowBitSet64 (Alignment + 1);
> >
> > --
> > 2.34.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#110993):
> > https://edk2.groups.io/g/devel/message/110993
> > Mute This Topic: https://groups.io/mt/102490513/1643496
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [michael.d.kinney@intel.com]
> > -=-=-=-=-=-=
> >
>
>


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



[-- Attachment #2: Type: text/html, Size: 6883 bytes --]

Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 4914 bytes --]

Options before us till now -

1. Add array overrun check and Debug statement before CpuDeadLoop within
2. Status Quo (not everything can be ideal :-))

Question before us
     - Is 1 better than 2 ?


On Fri, Nov 10, 2023 at 8:41 AM Ranbir Singh <rsingh@ventanamicro.com>
wrote:

> As far as I know, from a secure coding perspective, it would be
> recommended that array overrun condition check is captured in the code even
> if it is felt that it will never hit.
>
> Generally speaking, I won't be in favour of handling other ASSERT
> conditions updates even if required if they are not related to array
> overrun conditions i.e., the context of the patch.
>
> If someone / PCI maintainers can advise in this patch context what should
> be done in the array overrun condition, I will be happy to update,
> otherwise, sorry to say I won't be able to pursue this particular one
> further and hence would be leaving the related code with the status quo
> here.
>
> On Fri, Nov 10, 2023 at 2:10 AM Kinney, Michael D <
> michael.d.kinney@intel.com> wrote:
>
>> Hi Ranbir,
>>
>> A deadloop without even a debug print is not good behavior.
>>
>> If this condition really represents a condition where it is not possible
>> to complete the PCI resource allocation/assignment, then an error status
>> code should be returned to the caller of NotifyPhase().  Perhaps
>> EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API should
>> likely be updated to do the same.
>>
>> This may also require the caller of this service, the PCI Bus Driver,
>> to be reviewed to make sure it handles error conditions from
>> NotifyPhase().
>>
>> I recommend you get help on the proposed code changes from the PCI
>> subsystem maintainers.
>>
>> Thanks,
>>
>> Mike
>>
>>
>>
>> > -----Original Message-----
>> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
>> > Singh
>> > Sent: Thursday, November 9, 2023 9:39 AM
>> > To: devel@edk2.groups.io; rsingh@ventanamicro.com
>> > Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli
>> > <veeresh.sangolli@dellteam.com>
>> > Subject: [edk2-devel] [PATCH v3 1/2]
>> > MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
>> >
>> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> >
>> > The function NotifyPhase has a check
>> >
>> >     ASSERT (Index < TypeMax);
>> >
>> > but this comes into play only in DEBUG mode. In Release mode, there is
>> > no handling if the Index value is within array limits or not. If for
>> > whatever reasons, the Index does not get re-assigned to Index2 at line
>> > 937, then it remains at TypeMax as assigned earlier at line 929. This
>> > poses array overrun risk at lines 942 and 943. It is better to deploy
>> > a safety check on Index limit before accessing array elements.
>> >
>> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
>> >
>> > Cc: Ray Ni <ray.ni@intel.com>
>> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
>> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
>> > ---
>> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> > index d573e532bac8..c2c143068cd2 100644
>> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> > @@ -939,6 +939,11 @@ NotifyPhase (
>> >              }
>> >
>> >
>> >
>> >              ASSERT (Index < TypeMax);
>> >
>> > +
>> >
>> > +            if (Index == TypeMax) {
>> >
>> > +              CpuDeadLoop ();
>> >
>> > +            }
>> >
>> > +
>> >
>> >              ResNodeHandled[Index] = TRUE;
>> >
>> >              Alignment             = RootBridge-
>> > >ResAllocNode[Index].Alignment;
>> >
>> >              BitsOfAlignment       = LowBitSet64 (Alignment + 1);
>> >
>> > --
>> > 2.34.1
>> >
>> >
>> >
>> > -=-=-=-=-=-=
>> > Groups.io Links: You receive all messages sent to this group.
>> > View/Reply Online (#110993):
>> > https://edk2.groups.io/g/devel/message/110993
>> > Mute This Topic: https://groups.io/mt/102490513/1643496
>> > Group Owner: devel+owner@edk2.groups.io
>> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
>> > [michael.d.kinney@intel.com]
>> > -=-=-=-=-=-=
>> >
>>
>>


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



[-- Attachment #2: Type: text/html, Size: 7566 bytes --]

Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Laszlo Ersek]

On 11/9/23 21:40, Michael D Kinney wrote:
> Hi Ranbir,
> 
> A deadloop without even a debug print is not good behavior.

In DEBUG and NOOPT builds, the ASSERT() would fire, hence providing a
debug message.

In RELEASE builds, even if there were a separate DEBUG message, the
DEBUG would be compiled out.

> 
> If this condition really represents a condition where it is not possible
> to complete the PCI resource allocation/assignment, then an error status
> code should be returned to the caller of NotifyPhase().

That's not the case here. This ASSERT() really cannot fire:

  https://edk2.groups.io/g/devel/message/110856

In other words, it is a *true* genuine assertion.

It's only that Coverity cannot see that.

Thus the idea here is to tell Coverity that we're willing to hang even
in RELEASE builds. That hang will never ever occur (it can't), but by
adding the explicit CpuDeadLoop(), we can placate Coverity (hopefully).

Put differently: if we had an ASSERT() variant that could not be
compiled out of RELEASE builds, then we'd use that variant here.

Laszlo

> Perhaps
> EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API should
> likely be updated to do the same.
> 
> This may also require the caller of this service, the PCI Bus Driver,
> to be reviewed to make sure it handles error conditions from NotifyPhase().
> 
> I recommend you get help on the proposed code changes from the PCI
> subsystem maintainers.
> 
> Thanks,
> 
> Mike
> 
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
>> Singh
>> Sent: Thursday, November 9, 2023 9:39 AM
>> To: devel@edk2.groups.io; rsingh@ventanamicro.com
>> Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli
>> <veeresh.sangolli@dellteam.com>
>> Subject: [edk2-devel] [PATCH v3 1/2]
>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
>>
>> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>>
>> The function NotifyPhase has a check
>>
>>     ASSERT (Index < TypeMax);
>>
>> but this comes into play only in DEBUG mode. In Release mode, there is
>> no handling if the Index value is within array limits or not. If for
>> whatever reasons, the Index does not get re-assigned to Index2 at line
>> 937, then it remains at TypeMax as assigned earlier at line 929. This
>> poses array overrun risk at lines 942 and 943. It is better to deploy
>> a safety check on Index limit before accessing array elements.
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
>>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
>> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
>> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
>> ---
>>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> index d573e532bac8..c2c143068cd2 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> @@ -939,6 +939,11 @@ NotifyPhase (
>>              }
>>
>>
>>
>>              ASSERT (Index < TypeMax);
>>
>> +
>>
>> +            if (Index == TypeMax) {
>>
>> +              CpuDeadLoop ();
>>
>> +            }
>>
>> +
>>
>>              ResNodeHandled[Index] = TRUE;
>>
>>              Alignment             = RootBridge-
>>> ResAllocNode[Index].Alignment;
>>
>>              BitsOfAlignment       = LowBitSet64 (Alignment + 1);
>>
>> --
>> 2.34.1
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#110993):
>> https://edk2.groups.io/g/devel/message/110993
>> Mute This Topic: https://groups.io/mt/102490513/1643496
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>> [michael.d.kinney@intel.com]
>> -=-=-=-=-=-=
>>
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111163): https://edk2.groups.io/g/devel/message/111163
Mute This Topic: https://groups.io/mt/102490513/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] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Laszlo Ersek]

On 11/10/23 05:07, Ranbir Singh wrote:
> Options before us till now -
> 
> 1. Add array overrun check and Debug statement before CpuDeadLoop within

This would be useless.

Your current patch implements the following pattern:

  ASSERT (X);
  if (!X) {
    CpuDeadLoop ();
  }

Option#1 would mean

  ASSERT (X);
  if (!X) {
    DEBUG ((DEBUG_ERROR, "%a: condition X failed\n", __func__));
    CpuDeadLoop ();
  }

Now compare the behavior of both code snippets.

- In DEBUG and NOOPT builds, if X evaluated to FALSE, then the ASSERT()
would fire. A DEBUG_ERROR message would be logged, including "X", the
file name, and the line number. ASSERT() would then enter CpuDeadLoop()
internally, or invoke CpuBreakpoint() -- dependent on platform PCD
configuration. This applies to both snippets. In particular, the
explicit DEBUG and CpuDeadLoop() would not be reached, in the second
snippet. In other words, in DEBUG and NOOPT builds, the difference is
unobservable.

- In RELEASE builds, the ASSERT() is compiled out of both snippets.
Furthermore, the explicit DEBUG is compiled out of the second snippet.
That is, both code snippets would silently enter CpuDeadLoop(). In other
words, the behavior of both snippets is undistinguishable in RELEASE
builds too.

In my opinion, the current patch is right.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


To elaborate on that more generally:

Core edk2 code has so consistently and so broadly *abused* and *misused*
ASSERT() for error checking, that now we fail to recognize an *actual
valid use* of ASSERT() for what it is. For illustration, compare the
following two code snippets:

(a)

  UINTN Val;

  Val = 1 + 2;
  ASSERT (Val == 3);

(b)

  VOID *Ptr;

  Ptr = AllocatePool (1024);
  ASSERT (Ptr != NULL);

Snippet (a) is a *valid* use of an assert. It encapsulates a logical
predicate about the program's state, based on algorithmic and
mathematical reasoning. If the predicate turns out not to hold in
practice, at runtime,that means the programmer has retroactively caught
an issue in their own thinking, the algorithm is incorrectly designed or
implemented, and the program's state is effectively unknown at this
point (the internal invariant in question is broken), and so we must
stop immediately, because we don't know what the program is going to do
next. Given that what we thought about the program *thus far* has now
turned out to be false.

Snippet (b) is an abuse of assert. AllocatePool()'s result depends on
the environment. Available memory, input data seen from the user or the
disk or the network thus far, and a bunch of other things not under our
control. Therefore, we must explicitly deal with AllocatePool() failing.
Claiming that AllocatePool() will succeed is wrong because we generally
cannot even *attempt* to prove it.

In case (a) however, we can actually make a plausible attempt at
*proving* the predicate. The assert is there just in case our proof is
wrong.

There's an immense difference between situations (a) and (b). I cannot
emphasize that enough. Case (a) is a provable statement about the
behavior of an algorithm. Case (b) is a *guess* at input data and
environment.

The problem with edk2 core is that it has so consistently abused
ASSERT() for case (b) that now, when we occasionally see a *valid
instance* of (a), we mistake it for (b).

That's the case here. The ASSERT() seen in this patch is case (a). It's
just that Coverity cannot prove it itself, because the predicate we
assert is much more complicated than (1 + 2 == 3). But that doesn't
change the fact that we have a proof for the invariant in question.

Therefore, the solution is not to try to handle an impossible error
gracefully. The solution is two-fold:

- Tell coverity that we have a proof, in terms that it understands, to
shut it up. The terminology for this is CpuDeadLoop(). We're willing to
hang at runtime even in RELEASE builds, if the condition fails.

- If, at runtime, our proof turns out to be wrong indeed, then we must
stop immediately (because if 1+2 stops equalling 3, then we can't trust
*anything else* that our program would do).

(The more generic corollary of my last point, by the way, is that
ASSERT()s should NEVER be compiled out of programs. And if we actually
did that, like many other projects do, then this Coverity warning
wouldn't even exist, in the first place, because Coverity would *always*
see -- with the ASSERT() being there in all builds -- a range check on
Index.

Put differently, having to add a CpuDeadLoop() explicitly is just
"damage control" for the self-inflicted damage of compiling out ASSERTs.)

Laszlo



> 2. Status Quo (not everything can be ideal :-))
> 
> Question before us
>      - Is 1 better than 2 ?
> 
> 
> On Fri, Nov 10, 2023 at 8:41 AM Ranbir Singh <rsingh@ventanamicro.com
> <mailto:rsingh@ventanamicro.com>> wrote:
> 
>     As far as I know, from a secure coding perspective, it would be
>     recommended that array overrun condition check is captured in the
>     code even if it is felt that it will never hit.
> 
>     Generally speaking, I won't be in favour of handling other ASSERT
>     conditions updates even if required if they are not related to array
>     overrun conditions i.e., the context of the patch.
> 
>     If someone / PCI maintainers can advise in this patch context what
>     should be done in the array overrun condition, I will be happy to
>     update, otherwise, sorry to say I won't be able to pursue this
>     particular one further and hence would be leaving the related code
>     with the status quo here.
> 
>     On Fri, Nov 10, 2023 at 2:10 AM Kinney, Michael D
>     <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> wrote:
> 
>         Hi Ranbir,
> 
>         A deadloop without even a debug print is not good behavior.
> 
>         If this condition really represents a condition where it is not
>         possible
>         to complete the PCI resource allocation/assignment, then an
>         error status
>         code should be returned to the caller of NotifyPhase().  Perhaps
>         EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API
>         should
>         likely be updated to do the same.
> 
>         This may also require the caller of this service, the PCI Bus
>         Driver,
>         to be reviewed to make sure it handles error conditions from
>         NotifyPhase().
> 
>         I recommend you get help on the proposed code changes from the PCI
>         subsystem maintainers.
> 
>         Thanks,
> 
>         Mike
> 
> 
> 
>         > -----Original Message-----
>         > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>         <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf
>         Of Ranbir
>         > Singh
>         > Sent: Thursday, November 9, 2023 9:39 AM
>         > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
>         rsingh@ventanamicro.com <mailto:rsingh@ventanamicro.com>
>         > Cc: Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>;
>         Veeresh Sangolli
>         > <veeresh.sangolli@dellteam.com
>         <mailto:veeresh.sangolli@dellteam.com>>
>         > Subject: [edk2-devel] [PATCH v3 1/2]
>         > MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
>         >
>         > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>         >
>         > The function NotifyPhase has a check
>         >
>         >     ASSERT (Index < TypeMax);
>         >
>         > but this comes into play only in DEBUG mode. In Release mode,
>         there is
>         > no handling if the Index value is within array limits or not.
>         If for
>         > whatever reasons, the Index does not get re-assigned to Index2
>         at line
>         > 937, then it remains at TypeMax as assigned earlier at line
>         929. This
>         > poses array overrun risk at lines 942 and 943. It is better to
>         deploy
>         > a safety check on Index limit before accessing array elements.
>         >
>         > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
>         <https://bugzilla.tianocore.org/show_bug.cgi?id=4212>
>         >
>         > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>         > Co-authored-by: Veeresh Sangolli
>         <veeresh.sangolli@dellteam.com
>         <mailto:veeresh.sangolli@dellteam.com>>
>         > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
>         > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
>         <mailto:rsingh@ventanamicro.com>>
>         > ---
>         >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +++++
>         >  1 file changed, 5 insertions(+)
>         >
>         > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>         > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>         > index d573e532bac8..c2c143068cd2 100644
>         > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>         > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>         > @@ -939,6 +939,11 @@ NotifyPhase (
>         >              }
>         >
>         >
>         >
>         >              ASSERT (Index < TypeMax);
>         >
>         > +
>         >
>         > +            if (Index == TypeMax) {
>         >
>         > +              CpuDeadLoop ();
>         >
>         > +            }
>         >
>         > +
>         >
>         >              ResNodeHandled[Index] = TRUE;
>         >
>         >              Alignment             = RootBridge-
>         > >ResAllocNode[Index].Alignment;
>         >
>         >              BitsOfAlignment       = LowBitSet64 (Alignment + 1);
>         >
>         > --
>         > 2.34.1
>         >
>         >
>         >
>         > -=-=-=-=-=-=
>         > Groups.io Links: You receive all messages sent to this group.
>         > View/Reply Online (#110993):
>         > https://edk2.groups.io/g/devel/message/110993
>         <https://edk2.groups.io/g/devel/message/110993>
>         > Mute This Topic: https://groups.io/mt/102490513/1643496
>         <https://groups.io/mt/102490513/1643496>
>         > Group Owner: devel+owner@edk2.groups.io
>         <mailto:devel%2Bowner@edk2.groups.io>
>         > Unsubscribe: https://edk2.groups.io/g/devel/unsub
>         <https://edk2.groups.io/g/devel/unsub>
>         > [michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>]
>         > -=-=-=-=-=-=
>         >
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111164): https://edk2.groups.io/g/devel/message/111164
Mute This Topic: https://groups.io/mt/102490513/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] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue

[Laszlo Ersek]

On 11/9/23 18:39, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> The function SubmitResources has a switch-case code in which the
> case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
> case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check.
>
> While this may be intentional, it may not be evident to any general code
> reader/developer or static analyis tool why there is no break in between.
>
> SubmitResources function is supposed to handle only Mem or IO resources.
> So, update the ResType parameter check reflecting that and re-model the
> switch-case code in contention using just one if condition to further
> handle other parameter checks specific to ACPI_ADDRESS_SPACE_TYPE_MEM.
> This leads to mostly indentation level code changes. Few ASSERT's later
> present are henceforth not required.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60 +++++++++-----------
>  1 file changed, 26 insertions(+), 34 deletions(-)

I have applied this patch locally, and displayed it with

  git show -W -b

Let me make comments on that:

>  /**
>
>    Submits the I/O and memory resource requirements for the specified PCI Root Bridge.
>
>    @param This              The EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_ PROTOCOL instance.
> -  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory resource requirements.
> +  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory resource requirements
>                             are being submitted.
>    @param Configuration     The pointer to the PCI I/O and PCI memory resource descriptor.
>
>    @retval EFI_SUCCESS            Succeed.
>    @retval EFI_INVALID_PARAMETER  Wrong parameters passed in.
>  **/
> @@ -1460,137 +1460,129 @@ EFIAPI
>  SubmitResources (
>    IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL  *This,
>    IN EFI_HANDLE                                        RootBridgeHandle,
>    IN VOID                                              *Configuration
>    )
>  {
>    LIST_ENTRY                         *Link;
>    PCI_HOST_BRIDGE_INSTANCE           *HostBridge;
>    PCI_ROOT_BRIDGE_INSTANCE           *RootBridge;
>    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *Descriptor;
>    PCI_RESOURCE_TYPE                  Type;
>
>    //
>    // Check the input parameter: Configuration
>    //
>    if (Configuration == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }
>
>    HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
>    for (Link = GetFirstNode (&HostBridge->RootBridges)
>         ; !IsNull (&HostBridge->RootBridges, Link)
>         ; Link = GetNextNode (&HostBridge->RootBridges, Link)
>         )
>    {
>      RootBridge = ROOT_BRIDGE_FROM_LINK (Link);
>      if (RootBridgeHandle == RootBridge->Handle) {
>        DEBUG ((DEBUG_INFO, "PciHostBridge: SubmitResources for %s\n", RootBridge->DevicePathStr));
>        //
>        // Check the resource descriptors.
>        // If the Configuration includes one or more invalid resource descriptors, all the resource
>        // descriptors are ignored and the function returns EFI_INVALID_PARAMETER.
>        //
>        for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
> -        if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) {
> +        if ((Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM) && (Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_IO)) {
>            return EFI_INVALID_PARAMETER;
>          }

This is a slight logic change.

Previously, the code did the following:

- Any resource types that were different from MEM, IO, and BUS, would be
  rejected with EFI_INVALID_PARAMETER.

- MEM and IO would be handled.

- BUS resource types would trigger an ASSERT().

In effect, the code claimed that BUS resource types were *impossible*
here, due to prior filtering or whatever.

The logic change is that now we reject BUS resource types explicitly.

I think that's not ideal. Here's why:

- If the preexistent ASSERT() about BUS resource types being impossible
  was right, then now we are occluding that fact, and pretending /
  suggesting that BUS types are something we *should* handle. This
  creates a confusion about data flow.

- If the preexistent ASSERT() about BUS resource types being impossible
  was wrong (i.e., dependent on input data, we could actuall trigger the
  ASSERT()), then this change is very welcome, but *then* it is a bugfix
  in its own right! And therefore should be documented separately.

Anyway. I'm getting exhausted.

>
>          DEBUG ((
>            DEBUG_INFO,
>            " %s: Granularity/SpecificFlag = %ld / %02x%s\n",
>            mAcpiAddressSpaceTypeStr[Descriptor->ResType],
>            Descriptor->AddrSpaceGranularity,
>            Descriptor->SpecificFlag,
>            (Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0 ? L" (Prefetchable)" : L""
>            ));
>          DEBUG ((DEBUG_INFO, "      Length/Alignment = 0x%lx / 0x%lx\n", Descriptor->AddrLen, Descriptor->AddrRangeMax));
> -        switch (Descriptor->ResType) {
> -          case ACPI_ADDRESS_SPACE_TYPE_MEM:
> +
> +        if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
>            if ((Descriptor->AddrSpaceGranularity != 32) && (Descriptor->AddrSpaceGranularity != 64)) {
>              return EFI_INVALID_PARAMETER;
>            }
>
>            if ((Descriptor->AddrSpaceGranularity == 32) && (Descriptor->AddrLen >= SIZE_4GB)) {
>              return EFI_INVALID_PARAMETER;
>            }
>
>            //
>            // If the PCI root bridge does not support separate windows for nonprefetchable and
>            // prefetchable memory, then the PCI bus driver needs to include requests for
>            // prefetchable memory in the nonprefetchable memory pool.
>            //
>            if (((RootBridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0) &&
>                ((Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0)
>                )
>            {
>              return EFI_INVALID_PARAMETER;
>            }
> +        }
>
> -          case ACPI_ADDRESS_SPACE_TYPE_IO:
>          //
>          // Check aligment, it should be of the form 2^n-1
>          //
>          if (GetPowerOfTwo64 (Descriptor->AddrRangeMax + 1) != (Descriptor->AddrRangeMax + 1)) {
>            return EFI_INVALID_PARAMETER;
>          }
> -
> -            break;
> -          default:
> -            ASSERT (FALSE);
> -            break;
> -        }
>        }

The patch is good and clever thus far. We restrict the types we handle
to MEM and IO. Then we have a block of code that runs only for MEM, and
then another that -- due to being unrestricted -- runs for both MEM and
IO.

>
>        if (Descriptor->Desc != ACPI_END_TAG_DESCRIPTOR) {
>          return EFI_INVALID_PARAMETER;
>        }
>
>        for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
>          if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
>            if (Descriptor->AddrSpaceGranularity == 32) {
>              if ((Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) {
>                Type = TypePMem32;
>              } else {
>                Type = TypeMem32;
>              }
>            } else {
> -            ASSERT (Descriptor->AddrSpaceGranularity == 64);
>              if ((Descriptor->SpecificFlag & EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) {
>                Type = TypePMem64;
>              } else {
>                Type = TypeMem64;
>              }
>            }
>          } else {
> -          ASSERT (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO);
>            Type = TypeIo;
>          }

But the removal of these ASSERT()s is hard to justify.

Yes, these predicates are always TRUE at this point, due to checks
performed above.

But that's *exactly the reason* for including an ASSERT() in the code!
To remind the reader that a (perhaps non-obvious) predicate always holds
at that location!

So, the argument

  ASSERT(X) is unneeded because X always holds

is backwards. You do an ASSERT(X) *because* X always holds, and X is
non-trivial!

The only reason for removing an ASSERT(X) is that X, while true, is
*trivial*.

In this particular case, we do indeed explicitly restrict
Descriptor->AddrSpaceGranularity to 32 or 64 above, on the MEM branch.

We also ensure that the only resource type other than MEM can be IO.

In other words, the assertions are true.

Now the question is: are those true statements *trivial*? If they are
trivial, then removing them is OK. If they are not trivial, then they
are worth keeping.

In my opinion, they are worth keeping. They are *reminders* that we
performed those checks above.

But I'm not going to die on this hill, so if you don't want to respin:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

>
>          RootBridge->ResAllocNode[Type].Length    = Descriptor->AddrLen;
>          RootBridge->ResAllocNode[Type].Alignment = Descriptor->AddrRangeMax;
>          RootBridge->ResAllocNode[Type].Status    = ResSubmitted;
>        }
>
>        RootBridge->ResourceSubmitted = TRUE;
>        return EFI_SUCCESS;
>      }
>    }
>
>    return EFI_INVALID_PARAMETER;
>  }



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111165): https://edk2.groups.io/g/devel/message/111165
Mute This Topic: https://groups.io/mt/102490514/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] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 12244 bytes --]

On Mon, Nov 13, 2023 at 10:03 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/9/23 18:39, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function SubmitResources has a switch-case code in which the
> > case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
> > case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check.
> >
> > While this may be intentional, it may not be evident to any general code
> > reader/developer or static analyis tool why there is no break in between.
> >
> > SubmitResources function is supposed to handle only Mem or IO resources.
> > So, update the ResType parameter check reflecting that and re-model the
> > switch-case code in contention using just one if condition to further
> > handle other parameter checks specific to ACPI_ADDRESS_SPACE_TYPE_MEM.
> > This leads to mostly indentation level code changes. Few ASSERT's later
> > present are henceforth not required.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60
> +++++++++-----------
> >  1 file changed, 26 insertions(+), 34 deletions(-)
>
> I have applied this patch locally, and displayed it with
>
>   git show -W -b
>
> Let me make comments on that:
>
> >  /**
> >
> >    Submits the I/O and memory resource requirements for the specified
> PCI Root Bridge.
> >
> >    @param This              The EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_
> PROTOCOL instance.
> > -  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory
> resource requirements.
> > +  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory
> resource requirements
> >                             are being submitted.
> >    @param Configuration     The pointer to the PCI I/O and PCI memory
> resource descriptor.
> >
> >    @retval EFI_SUCCESS            Succeed.
> >    @retval EFI_INVALID_PARAMETER  Wrong parameters passed in.
> >  **/
> > @@ -1460,137 +1460,129 @@ EFIAPI
> >  SubmitResources (
> >    IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL  *This,
> >    IN EFI_HANDLE                                        RootBridgeHandle,
> >    IN VOID                                              *Configuration
> >    )
> >  {
> >    LIST_ENTRY                         *Link;
> >    PCI_HOST_BRIDGE_INSTANCE           *HostBridge;
> >    PCI_ROOT_BRIDGE_INSTANCE           *RootBridge;
> >    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *Descriptor;
> >    PCI_RESOURCE_TYPE                  Type;
> >
> >    //
> >    // Check the input parameter: Configuration
> >    //
> >    if (Configuration == NULL) {
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> >    HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
> >    for (Link = GetFirstNode (&HostBridge->RootBridges)
> >         ; !IsNull (&HostBridge->RootBridges, Link)
> >         ; Link = GetNextNode (&HostBridge->RootBridges, Link)
> >         )
> >    {
> >      RootBridge = ROOT_BRIDGE_FROM_LINK (Link);
> >      if (RootBridgeHandle == RootBridge->Handle) {
> >        DEBUG ((DEBUG_INFO, "PciHostBridge: SubmitResources for %s\n",
> RootBridge->DevicePathStr));
> >        //
> >        // Check the resource descriptors.
> >        // If the Configuration includes one or more invalid resource
> descriptors, all the resource
> >        // descriptors are ignored and the function returns
> EFI_INVALID_PARAMETER.
> >        //
> >        for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR
> *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR;
> Descriptor++) {
> > -        if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) {
> > +        if ((Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM) &&
> (Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_IO)) {
> >            return EFI_INVALID_PARAMETER;
> >          }
>
> This is a slight logic change.
>
> Previously, the code did the following:
>
> - Any resource types that were different from MEM, IO, and BUS, would be
>   rejected with EFI_INVALID_PARAMETER.
>
> - MEM and IO would be handled.
>
> - BUS resource types would trigger an ASSERT().
>
> In effect, the code claimed that BUS resource types were *impossible*
> here, due to prior filtering or whatever.
>
> The logic change is that now we reject BUS resource types explicitly.
>
> I think that's not ideal. Here's why:
>
> - If the preexistent ASSERT() about BUS resource types being impossible
>   was right, then now we are occluding that fact, and pretending /
>   suggesting that BUS types are something we *should* handle. This
>   creates a confusion about data flow.
>
> - If the preexistent ASSERT() about BUS resource types being impossible
>   was wrong (i.e., dependent on input data, we could actuall trigger the
>   ASSERT()), then this change is very welcome, but *then* it is a bugfix
>   in its own right! And therefore should be documented separately.
>
> Anyway. I'm getting exhausted.
>

My interpretation was SubmitResources function is supposed to handle only
Mem or IO resources, so > ACPI_ADDRESS_SPACE_TYPE_BUS check should actually
have been >= ACPI_ADDRESS_SPACE_TYPE_BUS. The function header also states
Mem or IO resources only and hence I considered it documented.

Also, considering only the RELEASE builds, if ResType comes as
ACPI_ADDRESS_SPACE_TYPE_BUS (even if hypothetically), then switch block
code lands in default, the ASSERT there is compiled out and after break
normal execution will carry on which at the following code point in the for
loop also contains ASSERT but that also gets compiled out (I am talking
RELEASE build only behaviour here) leading to it getting treated as IO
which doesn't seem correct at all to me.

          ...
          } else {
            ASSERT (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO);
            Type = TypeIo;
          }

There was no other handling in the function wrt BUS resource types.

If the BUS resource needs to be ASSERT'ed as before, then it can be done by
including one more if check. However, I don't think code should progress
ahead (even if hypothetically and inadvertently) to treating it as IO.


> >
> >          DEBUG ((
> >            DEBUG_INFO,
> >            " %s: Granularity/SpecificFlag = %ld / %02x%s\n",
> >            mAcpiAddressSpaceTypeStr[Descriptor->ResType],
> >            Descriptor->AddrSpaceGranularity,
> >            Descriptor->SpecificFlag,
> >            (Descriptor->SpecificFlag &
> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0 ? L"
> (Prefetchable)" : L""
> >            ));
> >          DEBUG ((DEBUG_INFO, "      Length/Alignment = 0x%lx / 0x%lx\n",
> Descriptor->AddrLen, Descriptor->AddrRangeMax));
> > -        switch (Descriptor->ResType) {
> > -          case ACPI_ADDRESS_SPACE_TYPE_MEM:
> > +
> > +        if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> >            if ((Descriptor->AddrSpaceGranularity != 32) &&
> (Descriptor->AddrSpaceGranularity != 64)) {
> >              return EFI_INVALID_PARAMETER;
> >            }
> >
> >            if ((Descriptor->AddrSpaceGranularity == 32) &&
> (Descriptor->AddrLen >= SIZE_4GB)) {
> >              return EFI_INVALID_PARAMETER;
> >            }
> >
> >            //
> >            // If the PCI root bridge does not support separate windows
> for nonprefetchable and
> >            // prefetchable memory, then the PCI bus driver needs to
> include requests for
> >            // prefetchable memory in the nonprefetchable memory pool.
> >            //
> >            if (((RootBridge->AllocationAttributes &
> EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0) &&
> >                ((Descriptor->SpecificFlag &
> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0)
> >                )
> >            {
> >              return EFI_INVALID_PARAMETER;
> >            }
> > +        }
> >
> > -          case ACPI_ADDRESS_SPACE_TYPE_IO:
> >          //
> >          // Check aligment, it should be of the form 2^n-1
> >          //
> >          if (GetPowerOfTwo64 (Descriptor->AddrRangeMax + 1) !=
> (Descriptor->AddrRangeMax + 1)) {
> >            return EFI_INVALID_PARAMETER;
> >          }
> > -
> > -            break;
> > -          default:
> > -            ASSERT (FALSE);
> > -            break;
> > -        }
> >        }
>
> The patch is good and clever thus far. We restrict the types we handle
> to MEM and IO. Then we have a block of code that runs only for MEM, and
> then another that -- due to being unrestricted -- runs for both MEM and
> IO.
>
> >
> >        if (Descriptor->Desc != ACPI_END_TAG_DESCRIPTOR) {
> >          return EFI_INVALID_PARAMETER;
> >        }
> >
> >        for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR
> *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR;
> Descriptor++) {
> >          if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> >            if (Descriptor->AddrSpaceGranularity == 32) {
> >              if ((Descriptor->SpecificFlag &
> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) {
> >                Type = TypePMem32;
> >              } else {
> >                Type = TypeMem32;
> >              }
> >            } else {
> > -            ASSERT (Descriptor->AddrSpaceGranularity == 64);
> >              if ((Descriptor->SpecificFlag &
> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) {
> >                Type = TypePMem64;
> >              } else {
> >                Type = TypeMem64;
> >              }
> >            }
> >          } else {
> > -          ASSERT (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO);
> >            Type = TypeIo;
> >          }
>
> But the removal of these ASSERT()s is hard to justify.
>
> Yes, these predicates are always TRUE at this point, due to checks
> performed above.
>
> But that's *exactly the reason* for including an ASSERT() in the code!
> To remind the reader that a (perhaps non-obvious) predicate always holds
> at that location!
>
> So, the argument
>
>   ASSERT(X) is unneeded because X always holds
>
> is backwards. You do an ASSERT(X) *because* X always holds, and X is
> non-trivial!
>
> The only reason for removing an ASSERT(X) is that X, while true, is
> *trivial*.
>

> In this particular case, we do indeed explicitly restrict
> Descriptor->AddrSpaceGranularity to 32 or 64 above, on the MEM branch.
>
> We also ensure that the only resource type other than MEM can be IO.
>
> In other words, the assertions are true.
>
> Now the question is: are those true statements *trivial*? If they are
> trivial, then removing them is OK. If they are not trivial, then they
> are worth keeping.
>
> In my opinion, they are worth keeping. They are *reminders* that we
> performed those checks above.
>
> But I'm not going to die on this hill, so if you don't want to respin:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Laszlo
>
>
Yes, I felt that to be trivial given the checks are already in place in the
same function earlier.
However, I am open to re-spin and restore these ASSERT's.


> >
> >          RootBridge->ResAllocNode[Type].Length    = Descriptor->AddrLen;
> >          RootBridge->ResAllocNode[Type].Alignment =
> Descriptor->AddrRangeMax;
> >          RootBridge->ResAllocNode[Type].Status    = ResSubmitted;
> >        }
> >
> >        RootBridge->ResourceSubmitted = TRUE;
> >        return EFI_SUCCESS;
> >      }
> >    }
> >
> >    return EFI_INVALID_PARAMETER;
> >  }
>
>


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



[-- Attachment #2: Type: text/html, Size: 15761 bytes --]

Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 12171 bytes --]

On Mon, Nov 13, 2023 at 9:42 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/10/23 05:07, Ranbir Singh wrote:
> > Options before us till now -
> >
> > 1. Add array overrun check and Debug statement before CpuDeadLoop within
>
> This would be useless.
>
> Your current patch implements the following pattern:
>
>   ASSERT (X);
>   if (!X) {
>     CpuDeadLoop ();
>   }
>
> Option#1 would mean
>
>   ASSERT (X);
>   if (!X) {
>     DEBUG ((DEBUG_ERROR, "%a: condition X failed\n", __func__));
>     CpuDeadLoop ();
>   }
>
> Now compare the behavior of both code snippets.
>
> - In DEBUG and NOOPT builds, if X evaluated to FALSE, then the ASSERT()
> would fire. A DEBUG_ERROR message would be logged, including "X", the
> file name, and the line number. ASSERT() would then enter CpuDeadLoop()
> internally, or invoke CpuBreakpoint() -- dependent on platform PCD
> configuration. This applies to both snippets. In particular, the
> explicit DEBUG and CpuDeadLoop() would not be reached, in the second
> snippet. In other words, in DEBUG and NOOPT builds, the difference is
> unobservable.
>
> - In RELEASE builds, the ASSERT() is compiled out of both snippets.
> Furthermore, the explicit DEBUG is compiled out of the second snippet.
> That is, both code snippets would silently enter CpuDeadLoop(). In other
> words, the behavior of both snippets is undistinguishable in RELEASE
> builds too.
>
> In my opinion, the current patch is right.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
>
> To elaborate on that more generally:
>
> Core edk2 code has so consistently and so broadly *abused* and *misused*
> ASSERT() for error checking, that now we fail to recognize an *actual
> valid use* of ASSERT() for what it is. For illustration, compare the
> following two code snippets:
>
> (a)
>
>   UINTN Val;
>
>   Val = 1 + 2;
>   ASSERT (Val == 3);
>
> (b)
>
>   VOID *Ptr;
>
>   Ptr = AllocatePool (1024);
>   ASSERT (Ptr != NULL);
>
> Snippet (a) is a *valid* use of an assert. It encapsulates a logical
> predicate about the program's state, based on algorithmic and
> mathematical reasoning. If the predicate turns out not to hold in
> practice, at runtime,that means the programmer has retroactively caught
> an issue in their own thinking, the algorithm is incorrectly designed or
> implemented, and the program's state is effectively unknown at this
> point (the internal invariant in question is broken), and so we must
> stop immediately, because we don't know what the program is going to do
> next. Given that what we thought about the program *thus far* has now
> turned out to be false.
>
> Snippet (b) is an abuse of assert. AllocatePool()'s result depends on
> the environment. Available memory, input data seen from the user or the
> disk or the network thus far, and a bunch of other things not under our
> control. Therefore, we must explicitly deal with AllocatePool() failing.
> Claiming that AllocatePool() will succeed is wrong because we generally
> cannot even *attempt* to prove it.
>
> In case (a) however, we can actually make a plausible attempt at
> *proving* the predicate. The assert is there just in case our proof is
> wrong.
>
> There's an immense difference between situations (a) and (b). I cannot
> emphasize that enough. Case (a) is a provable statement about the
> behavior of an algorithm. Case (b) is a *guess* at input data and
> environment.
>
> The problem with edk2 core is that it has so consistently abused
> ASSERT() for case (b) that now, when we occasionally see a *valid
> instance* of (a), we mistake it for (b).
>
> That's the case here. The ASSERT() seen in this patch is case (a). It's
> just that Coverity cannot prove it itself, because the predicate we
> assert is much more complicated than (1 + 2 == 3). But that doesn't
> change the fact that we have a proof for the invariant in question.
>
> Therefore, the solution is not to try to handle an impossible error
> gracefully. The solution is two-fold:
>
> - Tell coverity that we have a proof, in terms that it understands, to
> shut it up. The terminology for this is CpuDeadLoop(). We're willing to
> hang at runtime even in RELEASE builds, if the condition fails.
>
> - If, at runtime, our proof turns out to be wrong indeed, then we must
> stop immediately (because if 1+2 stops equalling 3, then we can't trust
> *anything else* that our program would do).
>
> (The more generic corollary of my last point, by the way, is that
> ASSERT()s should NEVER be compiled out of programs. And if we actually
> did that, like many other projects do, then this Coverity warning
> wouldn't even exist, in the first place, because Coverity would *always*
> see -- with the ASSERT() being there in all builds -- a range check on
> Index.
>
> Put differently, having to add a CpuDeadLoop() explicitly is just
> "damage control" for the self-inflicted damage of compiling out ASSERTs.)
>
> Laszlo
>
>
Thanks Laszlo for being immensely generous in writing in that much detail.
This must be beneficial to many.

Though you already gave R-b, the return statement needs to be added to
explicitly and completely rule out ARRAY_OVERRUN issue by static analysis
tools.

             ASSERT (Index < TypeMax);
+
+            if (Index == TypeMax) {
+              CpuDeadLoop ();
+              return EFI_OUT_OF_RESOURCES;
+            }
+


>
> > 2. Status Quo (not everything can be ideal :-))
> >
> > Question before us
> >      - Is 1 better than 2 ?
> >
> >
> > On Fri, Nov 10, 2023 at 8:41 AM Ranbir Singh <rsingh@ventanamicro.com
> > <mailto:rsingh@ventanamicro.com>> wrote:
> >
> >     As far as I know, from a secure coding perspective, it would be
> >     recommended that array overrun condition check is captured in the
> >     code even if it is felt that it will never hit.
> >
> >     Generally speaking, I won't be in favour of handling other ASSERT
> >     conditions updates even if required if they are not related to array
> >     overrun conditions i.e., the context of the patch.
> >
> >     If someone / PCI maintainers can advise in this patch context what
> >     should be done in the array overrun condition, I will be happy to
> >     update, otherwise, sorry to say I won't be able to pursue this
> >     particular one further and hence would be leaving the related code
> >     with the status quo here.
> >
> >     On Fri, Nov 10, 2023 at 2:10 AM Kinney, Michael D
> >     <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> wrote:
> >
> >         Hi Ranbir,
> >
> >         A deadloop without even a debug print is not good behavior.
> >
> >         If this condition really represents a condition where it is not
> >         possible
> >         to complete the PCI resource allocation/assignment, then an
> >         error status
> >         code should be returned to the caller of NotifyPhase().  Perhaps
> >         EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API
> >         should
> >         likely be updated to do the same.
> >
> >         This may also require the caller of this service, the PCI Bus
> >         Driver,
> >         to be reviewed to make sure it handles error conditions from
> >         NotifyPhase().
> >
> >         I recommend you get help on the proposed code changes from the
> PCI
> >         subsystem maintainers.
> >
> >         Thanks,
> >
> >         Mike
> >
> >
> >
> >         > -----Original Message-----
> >         > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> >         <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf
> >         Of Ranbir
> >         > Singh
> >         > Sent: Thursday, November 9, 2023 9:39 AM
> >         > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>;
> >         rsingh@ventanamicro.com <mailto:rsingh@ventanamicro.com>
> >         > Cc: Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>;
> >         Veeresh Sangolli
> >         > <veeresh.sangolli@dellteam.com
> >         <mailto:veeresh.sangolli@dellteam.com>>
> >         > Subject: [edk2-devel] [PATCH v3 1/2]
> >         > MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity
> issues
> >         >
> >         > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >         >
> >         > The function NotifyPhase has a check
> >         >
> >         >     ASSERT (Index < TypeMax);
> >         >
> >         > but this comes into play only in DEBUG mode. In Release mode,
> >         there is
> >         > no handling if the Index value is within array limits or not.
> >         If for
> >         > whatever reasons, the Index does not get re-assigned to Index2
> >         at line
> >         > 937, then it remains at TypeMax as assigned earlier at line
> >         929. This
> >         > poses array overrun risk at lines 942 and 943. It is better to
> >         deploy
> >         > a safety check on Index limit before accessing array elements.
> >         >
> >         > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
> >         <https://bugzilla.tianocore.org/show_bug.cgi?id=4212>
> >         >
> >         > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> >         > Co-authored-by: Veeresh Sangolli
> >         <veeresh.sangolli@dellteam.com
> >         <mailto:veeresh.sangolli@dellteam.com>>
> >         > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >         > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
> >         <mailto:rsingh@ventanamicro.com>>
> >         > ---
> >         >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5
> +++++
> >         >  1 file changed, 5 insertions(+)
> >         >
> >         > diff --git
> a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >         > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >         > index d573e532bac8..c2c143068cd2 100644
> >         > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >         > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >         > @@ -939,6 +939,11 @@ NotifyPhase (
> >         >              }
> >         >
> >         >
> >         >
> >         >              ASSERT (Index < TypeMax);
> >         >
> >         > +
> >         >
> >         > +            if (Index == TypeMax) {
> >         >
> >         > +              CpuDeadLoop ();
> >         >
> >         > +            }
> >         >
> >         > +
> >         >
> >         >              ResNodeHandled[Index] = TRUE;
> >         >
> >         >              Alignment             = RootBridge-
> >         > >ResAllocNode[Index].Alignment;
> >         >
> >         >              BitsOfAlignment       = LowBitSet64 (Alignment +
> 1);
> >         >
> >         > --
> >         > 2.34.1
> >         >
> >         >
> >         >
> >         > -=-=-=-=-=-=
> >         > Groups.io Links: You receive all messages sent to this group.
> >         > View/Reply Online (#110993):
> >         > https://edk2.groups.io/g/devel/message/110993
> >         <https://edk2.groups.io/g/devel/message/110993>
> >         > Mute This Topic: https://groups.io/mt/102490513/1643496
> >         <https://groups.io/mt/102490513/1643496>
> >         > Group Owner: devel+owner@edk2.groups.io
> >         <mailto:devel%2Bowner@edk2.groups.io>
> >         > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> >         <https://edk2.groups.io/g/devel/unsub>
> >         > [michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com
> >]
> >         > -=-=-=-=-=-=
> >         >
> >
> > 
>
>


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



[-- Attachment #2: Type: text/html, Size: 17734 bytes --]

Re: [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue

[Laszlo Ersek]

On 11/14/23 17:11, Ranbir Singh wrote:
> 
> 
> On Mon, Nov 13, 2023 at 10:03 PM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> 
>     On 11/9/23 18:39, Ranbir Singh wrote:
>     > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>     >
>     > The function SubmitResources has a switch-case code in which the
>     > case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
>     > case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check.
>     >
>     > While this may be intentional, it may not be evident to any
>     general code
>     > reader/developer or static analyis tool why there is no break in
>     between.
>     >
>     > SubmitResources function is supposed to handle only Mem or IO
>     resources.
>     > So, update the ResType parameter check reflecting that and
>     re-model the
>     > switch-case code in contention using just one if condition to further
>     > handle other parameter checks specific to ACPI_ADDRESS_SPACE_TYPE_MEM.
>     > This leads to mostly indentation level code changes. Few ASSERT's
>     later
>     > present are henceforth not required.
>     >
>     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
>     <https://bugzilla.tianocore.org/show_bug.cgi?id=4212>
>     >
>     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
>     <mailto:rsingh@ventanamicro.com>>
>     > ---
>     >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60
>     +++++++++-----------
>     >  1 file changed, 26 insertions(+), 34 deletions(-)
> 
>     I have applied this patch locally, and displayed it with
> 
>       git show -W -b
> 
>     Let me make comments on that:
> 
>     >  /**
>     >
>     >    Submits the I/O and memory resource requirements for the
>     specified PCI Root Bridge.
>     >
>     >    @param This              The
>     EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_ PROTOCOL instance.
>     > -  @param RootBridgeHandle  The PCI Root Bridge whose I/O and
>     memory resource requirements.
>     > +  @param RootBridgeHandle  The PCI Root Bridge whose I/O and
>     memory resource requirements
>     >                             are being submitted.
>     >    @param Configuration     The pointer to the PCI I/O and PCI
>     memory resource descriptor.
>     >
>     >    @retval EFI_SUCCESS            Succeed.
>     >    @retval EFI_INVALID_PARAMETER  Wrong parameters passed in.
>     >  **/
>     > @@ -1460,137 +1460,129 @@ EFIAPI
>     >  SubmitResources (
>     >    IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL  *This,
>     >    IN EFI_HANDLE                                       
>     RootBridgeHandle,
>     >    IN VOID                                              *Configuration
>     >    )
>     >  {
>     >    LIST_ENTRY                         *Link;
>     >    PCI_HOST_BRIDGE_INSTANCE           *HostBridge;
>     >    PCI_ROOT_BRIDGE_INSTANCE           *RootBridge;
>     >    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *Descriptor;
>     >    PCI_RESOURCE_TYPE                  Type;
>     >
>     >    //
>     >    // Check the input parameter: Configuration
>     >    //
>     >    if (Configuration == NULL) {
>     >      return EFI_INVALID_PARAMETER;
>     >    }
>     >
>     >    HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
>     >    for (Link = GetFirstNode (&HostBridge->RootBridges)
>     >         ; !IsNull (&HostBridge->RootBridges, Link)
>     >         ; Link = GetNextNode (&HostBridge->RootBridges, Link)
>     >         )
>     >    {
>     >      RootBridge = ROOT_BRIDGE_FROM_LINK (Link);
>     >      if (RootBridgeHandle == RootBridge->Handle) {
>     >        DEBUG ((DEBUG_INFO, "PciHostBridge: SubmitResources for
>     %s\n", RootBridge->DevicePathStr));
>     >        //
>     >        // Check the resource descriptors.
>     >        // If the Configuration includes one or more invalid
>     resource descriptors, all the resource
>     >        // descriptors are ignored and the function returns
>     EFI_INVALID_PARAMETER.
>     >        //
>     >        for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR
>     *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR;
>     Descriptor++) {
>     > -        if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) {
>     > +        if ((Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM)
>     && (Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_IO)) {
>     >            return EFI_INVALID_PARAMETER;
>     >          }
> 
>     This is a slight logic change.
> 
>     Previously, the code did the following:
> 
>     - Any resource types that were different from MEM, IO, and BUS, would be
>       rejected with EFI_INVALID_PARAMETER.
> 
>     - MEM and IO would be handled.
> 
>     - BUS resource types would trigger an ASSERT().
> 
>     In effect, the code claimed that BUS resource types were *impossible*
>     here, due to prior filtering or whatever.
> 
>     The logic change is that now we reject BUS resource types explicitly.
> 
>     I think that's not ideal. Here's why:
> 
>     - If the preexistent ASSERT() about BUS resource types being impossible
>       was right, then now we are occluding that fact, and pretending /
>       suggesting that BUS types are something we *should* handle. This
>       creates a confusion about data flow.
> 
>     - If the preexistent ASSERT() about BUS resource types being impossible
>       was wrong (i.e., dependent on input data, we could actuall trigger the
>       ASSERT()), then this change is very welcome, but *then* it is a bugfix
>       in its own right! And therefore should be documented separately.
> 
>     Anyway. I'm getting exhausted.
> 
> 
> My interpretation was SubmitResources function is supposed to handle
> only Mem or IO resources, so > ACPI_ADDRESS_SPACE_TYPE_BUS check should
> actually have been >= ACPI_ADDRESS_SPACE_TYPE_BUS. The function header
> also states Mem or IO resources only and hence I considered it documented.
> 
> Also, considering only the RELEASE builds, if ResType comes as
> ACPI_ADDRESS_SPACE_TYPE_BUS (even if hypothetically), then switch block
> code lands in default, the ASSERT there is compiled out and after break
> normal execution will carry on which at the following code point in the
> for loop also contains ASSERT but that also gets compiled out (I am
> talking RELEASE build only behaviour here) leading to it getting treated
> as IO which doesn't seem correct at all to me.
> 
>           ...
>           } else {
>             ASSERT (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO);
>             Type = TypeIo;
>           }
> 
> There was no other handling in the function wrt BUS resource types.
> 
> If the BUS resource needs to be ASSERT'ed as before, then it can be done
> by including one more if check. However, I don't think code should
> progress ahead (even if hypothetically and inadvertently) to treating it
> as IO.
> 
> 
>     >
>     >          DEBUG ((
>     >            DEBUG_INFO,
>     >            " %s: Granularity/SpecificFlag = %ld / %02x%s\n",
>     >            mAcpiAddressSpaceTypeStr[Descriptor->ResType],
>     >            Descriptor->AddrSpaceGranularity,
>     >            Descriptor->SpecificFlag,
>     >            (Descriptor->SpecificFlag &
>     EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0
>     ? L" (Prefetchable)" : L""
>     >            ));
>     >          DEBUG ((DEBUG_INFO, "      Length/Alignment = 0x%lx /
>     0x%lx\n", Descriptor->AddrLen, Descriptor->AddrRangeMax));
>     > -        switch (Descriptor->ResType) {
>     > -          case ACPI_ADDRESS_SPACE_TYPE_MEM:
>     > +
>     > +        if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
>     >            if ((Descriptor->AddrSpaceGranularity != 32) &&
>     (Descriptor->AddrSpaceGranularity != 64)) {
>     >              return EFI_INVALID_PARAMETER;
>     >            }
>     >
>     >            if ((Descriptor->AddrSpaceGranularity == 32) &&
>     (Descriptor->AddrLen >= SIZE_4GB)) {
>     >              return EFI_INVALID_PARAMETER;
>     >            }
>     >
>     >            //
>     >            // If the PCI root bridge does not support separate
>     windows for nonprefetchable and
>     >            // prefetchable memory, then the PCI bus driver needs
>     to include requests for
>     >            // prefetchable memory in the nonprefetchable memory pool.
>     >            //
>     >            if (((RootBridge->AllocationAttributes &
>     EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0) &&
>     >                ((Descriptor->SpecificFlag &
>     EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0)
>     >                )
>     >            {
>     >              return EFI_INVALID_PARAMETER;
>     >            }
>     > +        }
>     >
>     > -          case ACPI_ADDRESS_SPACE_TYPE_IO:
>     >          //
>     >          // Check aligment, it should be of the form 2^n-1
>     >          //
>     >          if (GetPowerOfTwo64 (Descriptor->AddrRangeMax + 1) !=
>     (Descriptor->AddrRangeMax + 1)) {
>     >            return EFI_INVALID_PARAMETER;
>     >          }
>     > -
>     > -            break;
>     > -          default:
>     > -            ASSERT (FALSE);
>     > -            break;
>     > -        }
>     >        }
> 
>     The patch is good and clever thus far. We restrict the types we handle
>     to MEM and IO. Then we have a block of code that runs only for MEM, and
>     then another that -- due to being unrestricted -- runs for both MEM and
>     IO.
> 
>     >
>     >        if (Descriptor->Desc != ACPI_END_TAG_DESCRIPTOR) {
>     >          return EFI_INVALID_PARAMETER;
>     >        }
>     >
>     >        for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR
>     *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR;
>     Descriptor++) {
>     >          if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
>     >            if (Descriptor->AddrSpaceGranularity == 32) {
>     >              if ((Descriptor->SpecificFlag &
>     EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) {
>     >                Type = TypePMem32;
>     >              } else {
>     >                Type = TypeMem32;
>     >              }
>     >            } else {
>     > -            ASSERT (Descriptor->AddrSpaceGranularity == 64);
>     >              if ((Descriptor->SpecificFlag &
>     EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0) {
>     >                Type = TypePMem64;
>     >              } else {
>     >                Type = TypeMem64;
>     >              }
>     >            }
>     >          } else {
>     > -          ASSERT (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO);
>     >            Type = TypeIo;
>     >          }
> 
>     But the removal of these ASSERT()s is hard to justify.
> 
>     Yes, these predicates are always TRUE at this point, due to checks
>     performed above.
> 
>     But that's *exactly the reason* for including an ASSERT() in the code!
>     To remind the reader that a (perhaps non-obvious) predicate always holds
>     at that location!
> 
>     So, the argument
> 
>       ASSERT(X) is unneeded because X always holds
> 
>     is backwards. You do an ASSERT(X) *because* X always holds, and X is
>     non-trivial!
> 
>     The only reason for removing an ASSERT(X) is that X, while true, is
>     *trivial*.
> 
> 
>     In this particular case, we do indeed explicitly restrict
>     Descriptor->AddrSpaceGranularity to 32 or 64 above, on the MEM branch.
> 
>     We also ensure that the only resource type other than MEM can be IO.
> 
>     In other words, the assertions are true.
> 
>     Now the question is: are those true statements *trivial*? If they are
>     trivial, then removing them is OK. If they are not trivial, then they
>     are worth keeping.
> 
>     In my opinion, they are worth keeping. They are *reminders* that we
>     performed those checks above.
> 
>     But I'm not going to die on this hill, so if you don't want to respin:
> 
>     Reviewed-by: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> 
>     Laszlo
> 
> 
> Yes, I felt that to be trivial given the checks are already in place in
> the same function earlier.
> However, I am open to re-spin and restore these ASSERT's.

I'd like you to respin the patch for two reasons, then:

- More importantly: you've demonstrated that the change regarding BUS
handling is a bugfix. As requested before, please mention it in the
commit message explicitly. (I do agree we can keep the logic change in
the same patch.)

- Less importantly: keeping these last two ASSERT()s would be nice, then.

Thanks!
Laszlo

>  
> 
>     >
>     >          RootBridge->ResAllocNode[Type].Length    =
>     Descriptor->AddrLen;
>     >          RootBridge->ResAllocNode[Type].Alignment =
>     Descriptor->AddrRangeMax;
>     >          RootBridge->ResAllocNode[Type].Status    = ResSubmitted;
>     >        }
>     >
>     >        RootBridge->ResourceSubmitted = TRUE;
>     >        return EFI_SUCCESS;
>     >      }
>     >    }
>     >
>     >    return EFI_INVALID_PARAMETER;
>     >  }
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111244): https://edk2.groups.io/g/devel/message/111244
Mute This Topic: https://groups.io/mt/102490514/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] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Laszlo Ersek]

On 11/14/23 17:34, Ranbir Singh wrote:

> Though you already gave R-b, the return statement needs to be added to
> explicitly and completely rule out ARRAY_OVERRUN issue by static
> analysis tools.
> 
>              ASSERT (Index < TypeMax);
> +
> +            if (Index == TypeMax) {
> +              CpuDeadLoop ();
> +              return EFI_OUT_OF_RESOURCES;
> +            }
> +

Works for me!

If the "return" there is the only change in the upcoming v4 of this
patch, then please just carry forward my R-b.

Thanks!
Laszlo



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