On 11/7/23 07:19, Ranbir Singh wrote: > From: Ranbir Singh <ranbir.sin...@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...@intel.com> > Co-authored-by: Veeresh Sangolli <veeresh.sango...@dellteam.com> > Signed-off-by: Ranbir Singh <ranbir.sin...@dell.com> > Signed-off-by: Ranbir Singh <rsi...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-