Re: [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues

["Laszlo Ersek" <lersek@redhat.com>]

On 11/7/23 07:19, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function PciHostBridgeResourceAllocator is not making use of the
> generic approach as is used in one of the other function namely -
> DumpResourceMap. As a result, the following warnings can be seen as
> reported by Coverity e.g.
> 
> (30) Event address_of:	Taking address with "&IoBridge" yields a
>      singleton pointer.
> (31) Event callee_ptr_arith:	Passing "&IoBridge" to function
>      "FindResourceNode" which uses it as an array. This might corrupt
>      or misinterpret adjacent memory locations.
> 
> Hence, adopt the generic approach to fix the issues at relevant points.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> 
> 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/PciBusDxe/PciLib.c | 37 ++++++++++++++++----
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index 84fc0161a19c..71767d3793d4 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator (
>    UINT64                                         Mem64ResStatus;
>    UINT64                                         PMem64ResStatus;
>    UINT32                                         MaxOptionRomSize;
> +  PCI_RESOURCE_NODE                              **ChildResources;
> +  UINTN                                          ChildResourceCount;
>    PCI_RESOURCE_NODE                              *IoBridge;
>    PCI_RESOURCE_NODE                              *Mem32Bridge;
>    PCI_RESOURCE_NODE                              *PMem32Bridge;
> @@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator (
>      // Create the entire system resource map from the information collected by
>      // enumerator. Several resource tree was created
>      //
> -    FindResourceNode (RootBridgeDev, &IoPool, &IoBridge);
> -    FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge);
> -    FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge);
> -    FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge);
> -    FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge);
> -
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]);
> +    IoBridge = ChildResources[0];
>      ASSERT (IoBridge     != NULL);
> +
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]);
> +    Mem32Bridge = ChildResources[0];
>      ASSERT (Mem32Bridge  != NULL);
> +
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]);
> +    PMem32Bridge = ChildResources[0];
>      ASSERT (PMem32Bridge != NULL);
> +
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]);
> +    Mem64Bridge = ChildResources[0];
>      ASSERT (Mem64Bridge  != NULL);
> +
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]);
> +    PMem64Bridge = ChildResources[0];
>      ASSERT (PMem64Bridge != NULL);
>  
>      //

Sorry, but this is terrible.

* First of all, Coverity is *wrong*. The C spec clearly states that, for
the purposes of pointer arithmetic, a singleton object behaves
identically to the first element of an array.

So, immediately, the idea arises that we should just use

  PCI_RESOURCE_NODE *IoBridgeArray[1];

  FindResourceNode (RootBridgeDev, &IoPool, IoBridgeArray)

to shut up Coverity.

* Unfortunately, I expect that would only create a different warning: a
warning about potentially overflowing this single-element array. Which
is in fact a deeper problem in FindResourceNode() -- it happily
overwrites an array that is too small.

* Finally, this generic approach is both ugly (lots of code
duplication!), and worse, it allocates memory without proper error
checking (ASSERT() is not error checking), and then it leaks
ChildResources *multiple times*!

I suggest the following, for solving all of these issues:

- create a function called FindFirstResourceNode(). It should have the
same function prototype as FindResourceNode(), except the last parameter
should be mandatory, not OPTIONAL.

- the internal logic should be the same, except we shouldn't be
counting, the return value should be a BOOLEAN. If we find a match, then
output the first match and return TRUE. Otherwise, set the output to
NULL and return FALSE.

- replace the FindResourceNode calls with FindFirstResourceNode calls

- Those call sites are already followed by ASSERT()s, saying that all
search attempts will succeed. If coverity is happy with them, that's
good; otherwise, we'll have to find a solution for them as well.

Laszlo



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