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

["Ranbir Singh" <rsingh@ventanamicro.com>]

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

I was apprehensive about this from the very beginning.

Anyway, for now I will be dropping this issue from the series.

On Tue, Nov 7, 2023 at 10:11 PM Laszlo Ersek <lersek@redhat.com> wrote:

> 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 (#111016): https://edk2.groups.io/g/devel/message/111016
Mute This Topic: https://groups.io/mt/102438300/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: 8824 bytes --]