[edk2-devel] [Staging/Bug_1525_FmpDevicePkg_MultipleControllers][PATCH] MdeModulePkg/EsrtFmpDxe: Detect duplicate GUID/HardwareInstance

Eric Jin posted 1 patch 4 years, 12 months ago
Failed in applying to current master (apply log)
MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c | 461 ++++++++++----------
1 file changed, 238 insertions(+), 223 deletions(-)
[edk2-devel] [Staging/Bug_1525_FmpDevicePkg_MultipleControllers][PATCH] MdeModulePkg/EsrtFmpDxe: Detect duplicate GUID/HardwareInstance
Posted by Eric Jin 4 years, 12 months ago
https://bugzilla.tianocore.org/show_bug.cgi?id=1525

Fix the following issues identified in #1525:
    EsrtFmpDxe - The check for more than one instance of the same
    GUID/HardwareInstance is not being done at the right point.
    It is being done as EFI_FIRMWARE_IMAGE_DESCRIPTORs are being merged
    into an ESRT Table Entry.  This means a case will be missed if
    there are 3 EFI_FIRMWARE_IMAGE_DESCRIPTORs with the first and 3rd
    one being having the same GUID/HardwareInstance.  Instead, this check
    should be performed across all EFI_FIRMWARE_IMAGE_DESCRIPTORs in the
    entire handle database.

Build a table of GUID/HardwareInstance pairs from all the
EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP instances.  If a duplicate
is found, then generate a DEBUG_ERROR message, generate an ASSERT(),
and ignore the duplicate EFI_FIRMWARE_IMAGE_DESCRIPTOR.

Add an internal worker function called FmpGetFirmwareImageDescriptor()
that retrieves the list of EFI_FIRMWARE_IMAGE_DESCRIPTORs from a single
FMP instance and returns the descriptors in an allocated buffer.  This
function is used to get the descriptors used to build the table of
unique GUID/HardwareInstance pairs.  It is then used again to generate
the ESRT Table from all the EFI_FIRMWARE_IMAGE_DESCRIPTORs from all the
FMP instances.  2 passes are performed so the total number of
descriptors is known.  This allows the correct sized buffers to always
be allocated and the extra logic to grow tables is removed.

CC: Eric Jin <eric.jin@intel.com>
CC: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Jin <eric.jin@intel.com>
---
 MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c | 461 ++++++++++----------
 1 file changed, 238 insertions(+), 223 deletions(-)

diff --git a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
index 848bd44e9d..9c273fe79e 100644
--- a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
+++ b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
@@ -38,6 +38,22 @@
 #include <Guid/EventGroup.h>
 #include <Guid/SystemResourceTable.h>
 
+///
+/// Structure for array of unique GUID/HardwareInstance pairs from the
+/// current set of EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP Protocols.
+///
+typedef struct {
+  ///
+  /// A unique GUID identifying the firmware image type.
+  ///
+  EFI_GUID                      ImageTypeGuid;
+  ///
+  /// An optional number to identify the unique hardware instance within the
+  /// system for devices that may have multiple instances whenever possible.
+  ///
+  UINT64                        HardwareInstance;
+} GUID_HARDWAREINSTANCE_PAIR;
+
 /**
  Print ESRT to debug console.
 
@@ -50,11 +66,6 @@ PrintTable (
   IN EFI_SYSTEM_RESOURCE_TABLE  *Table
   );
 
-//
-// Number of ESRT entries to grow by each time we run out of room
-//
-#define GROWTH_STEP  10
-
 /**
   Install EFI System Resource Table into the UEFI Configuration Table
 
@@ -118,172 +129,129 @@ IsSystemFmp (
 }
 
 /**
-  Function to create a single ESRT Entry and add it to the ESRT
-  given a FMP descriptor.  If the guid is already in the ESRT, then the ESRT
-  entry is updated.  The ESRT will grow if it does not have enough room.
-
-  @param[in, out] Table             On input, pointer to the pointer to the ESRT.
-                                    On output, same as input or pointer to the pointer
-                                    to new enlarged ESRT.
-  @param[in]      FmpImageInfoBuf   Pointer to the EFI_FIRMWARE_IMAGE_DESCRIPTOR.
-  @param[in]      FmpVersion        FMP Version.
-
-  @return  Status code.
+  Function to create a single ESRT Entry and add it to the ESRT with
+  a given FMP descriptor.  If the GUID is already in the ESRT, then the ESRT
+  entry is updated.
+
+  @param[in,out] Table                Pointer to the ESRT Table.
+  @param[in,out] HardwareInstances    Pointer to the GUID_HARDWAREINSTANCE_PAIR.
+  @param[in,out] NumberOfDescriptors  The number of EFI_FIRMWARE_IMAGE_DESCRIPTORs.
+  @param[in]     FmpImageInfoBuf      Pointer to the EFI_FIRMWARE_IMAGE_DESCRIPTOR.
+  @param[in]     FmpVersion           FMP Version.
+
+  @retval  EFI_SUCCESS     FmpImageInfoBuf was use to fill in a new ESRT entry
+                           in Table.
+  @retval  EFI_SUCCESS     The ImageTypeId GUID in FmpImageInfoBuf matches an
+                           existing ESRT entry in Table, and the information
+                           from FmpImageInfoBuf was merged into the the existing
+                           ESRT entry.
+  @retval  EFI_UNSPOORTED  The GUID/HardareInstance in FmpImageInfoBuf has is a
+                           duplicate.
 
 **/
 EFI_STATUS
 CreateEsrtEntry (
-  IN OUT EFI_SYSTEM_RESOURCE_TABLE  **Table,
-  IN OUT UINT64                     **HardwareInstances,
-  IN EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf,
-  IN UINT32                         FmpVersion
+  IN OUT EFI_SYSTEM_RESOURCE_TABLE      *Table,
+  IN OUT GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances,
+  IN OUT UINT32                         *NumberOfDescriptors,
+  IN     EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf,
+  IN     UINT32                         FmpVersion
   )
 {
   UINTN                      Index;
   EFI_SYSTEM_RESOURCE_ENTRY  *Entry;
-  UINTN                      NewSize;
-  EFI_SYSTEM_RESOURCE_TABLE  *NewTable;
-  UINT64                     *NewHardwareInstances;
   UINT64                     FmpHardwareInstance;
 
-  Index = 0;
-  Entry = NULL;
+  FmpHardwareInstance = 0;
+  if (FmpVersion >= 3) {
+    FmpHardwareInstance = FmpImageInfoBuf->HardwareInstance;
+  }
 
-  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)((*Table) + 1);
   //
-  // Check to see if GUID is already in the table
+  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
   //
-  for (Index = 0; Index < (*Table)->FwResourceCount; Index++) {
-    if (CompareGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId)) {
-      //
-      // If HardwareInstance in ESRT and FmpImageInfoBuf are the same value
-      // for the same ImageTypeId GUID, then there is more than one FMP
-      // instance for the same FW device, which is an error condition.
-      // If FmpVersion is less than 3, then assume HardwareInstance is 0.
-      //
-      FmpHardwareInstance = 0;
-      if (FmpVersion >= 3) {
-        FmpHardwareInstance = FmpImageInfoBuf->HardwareInstance;
-      }
-      if ((*HardwareInstances)[Index] == FmpHardwareInstance) {
-        DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: ESRT Entry already exists for FMP Instance with GUID %g and HardwareInstance %016lx\n", &Entry->FwClass, (*HardwareInstances)[Index]));
-        ASSERT ((*HardwareInstances)[Index] != FmpHardwareInstance);
+  for (Index = 0; Index < *NumberOfDescriptors; Index++) {
+    if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImageInfoBuf->ImageTypeId)) {
+      if (HardwareInstances[Index].HardwareInstance == FmpHardwareInstance) {
+        DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware image descriptor with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, FmpHardwareInstance));
+        ASSERT (
+          !CompareGuid (&HardwareInstances[Index].ImageTypeGuid, &FmpImageInfoBuf->ImageTypeId) ||
+          HardwareInstances[Index].HardwareInstance != FmpHardwareInstance
+          );
         return EFI_UNSUPPORTED;
       }
-
-      DEBUG ((DEBUG_INFO, "EsrtFmpDxe: ESRT Entry already exists for FMP Instance with GUID %g\n", &Entry->FwClass));
-
-      //
-      // Set ESRT FwVersion to the smaller of the two values
-      //
-      Entry->FwVersion = MIN (FmpImageInfoBuf->Version, Entry->FwVersion);
-
-      //
-      // VERSION 2 has Lowest Supported
-      //
-      if (FmpVersion >= 2) {
-        //
-        // Set ESRT LowestSupportedFwVersion to the smaller of the two values
-        //
-        Entry->LowestSupportedFwVersion =
-          MIN (
-            FmpImageInfoBuf->LowestSupportedImageVersion,
-            Entry->LowestSupportedFwVersion
-            );
-      }
-
-      //
-      // VERSION 3 supports last attempt values
-      //
-      if (FmpVersion >= 3) {
-        //
-        // Update the ESRT entry with the last attempt status and last attempt
-        // version from the first FMP instance whose last attempt status is not
-        // SUCCESS.  If all FMP instances are SUCCESS, then set version to the 
-        // smallest value from all FMP instances.
-        //
-        if (Entry->LastAttemptStatus == LAST_ATTEMPT_STATUS_SUCCESS) {
-          if (FmpImageInfoBuf->LastAttemptStatus != LAST_ATTEMPT_STATUS_SUCCESS) {
-            Entry->LastAttemptStatus = FmpImageInfoBuf->LastAttemptStatus;
-            Entry->LastAttemptVersion = FmpImageInfoBuf->LastAttemptVersion;
-          } else {
-            Entry->LastAttemptVersion =
-              MIN (
-                FmpImageInfoBuf->LastAttemptVersion,
-                Entry->LastAttemptVersion
-                );
-          }
-        }
-      }
-
-      return EFI_SUCCESS;
     }
-    Entry++;
   }
 
   //
-  // Grow table if needed
+  // Record new GUID/HardwareInstance pair
   //
-  if ((*Table)->FwResourceCount >= (*Table)->FwResourceCountMax) {
-    NewSize  = (((*Table)->FwResourceCountMax + GROWTH_STEP) * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE);
-    NewTable = AllocateZeroPool (NewSize);
-    if (NewTable == NULL) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory larger table for ESRT. \n"));
-      return EFI_OUT_OF_RESOURCES;
-    }
-    //
-    // Copy the whole old table into new table buffer
-    //
-    CopyMem (
-      NewTable,
-      (*Table),
-      (((*Table)->FwResourceCountMax) * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE)
-      );
-    //
-    // Update max
-    //
-    NewTable->FwResourceCountMax = NewTable->FwResourceCountMax + GROWTH_STEP;
-    //
-    // Free old table
-    //
-    FreePool ((*Table));
-    //
-    // Reassign pointer to new table.
-    //
-    (*Table) = NewTable;
+  CopyGuid (&HardwareInstances[*NumberOfDescriptors].ImageTypeGuid, &FmpImageInfoBuf->ImageTypeId);
+  HardwareInstances[*NumberOfDescriptors].HardwareInstance = FmpHardwareInstance;
+  *NumberOfDescriptors = *NumberOfDescriptors + 1;
+
+  DEBUG ((DEBUG_INFO, "EsrtFmpDxe: Add new image descriptor with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, FmpHardwareInstance));
 
-    NewSize  = ((*Table)->FwResourceCountMax) * sizeof (UINT64);
-    NewHardwareInstances = AllocateZeroPool (NewSize);
-    if (NewHardwareInstances == NULL) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory larger table for Hardware Instances.\n"));
-      return EFI_OUT_OF_RESOURCES;
+  //
+  // Check to see if GUID is already in the ESRT table
+  //
+  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1);
+  for (Index = 0; Index < Table->FwResourceCount; Index++, Entry++) {
+    if (!CompareGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId)) {
+      continue;
     }
+    DEBUG ((DEBUG_INFO, "EsrtFmpDxe: ESRT Entry already exists for FMP Instance with GUID %g\n", &Entry->FwClass));
+
     //
-    // Copy the whole old table into new table buffer
+    // Set ESRT FwVersion to the smaller of the two values
     //
-    CopyMem (
-      NewHardwareInstances,
-      (*HardwareInstances),
-      ((*Table)->FwResourceCountMax) * sizeof (UINT64)
-      );
+    Entry->FwVersion = MIN (FmpImageInfoBuf->Version, Entry->FwVersion);
+
     //
-    // Free old table
+    // VERSION 2 has Lowest Supported
     //
-    FreePool ((*HardwareInstances));
+    if (FmpVersion >= 2) {
+      //
+      // Set ESRT LowestSupportedFwVersion to the smaller of the two values
+      //
+      Entry->LowestSupportedFwVersion =
+        MIN (
+          FmpImageInfoBuf->LowestSupportedImageVersion,
+          Entry->LowestSupportedFwVersion
+          );
+    }
+
     //
-    // Reassign pointer to new table.
+    // VERSION 3 supports last attempt values
     //
-    (*HardwareInstances) = NewHardwareInstances;
+    if (FmpVersion >= 3) {
+      //
+      // Update the ESRT entry with the last attempt status and last attempt
+      // version from the first FMP instance whose last attempt status is not
+      // SUCCESS.  If all FMP instances are SUCCESS, then set version to the
+      // smallest value from all FMP instances.
+      //
+      if (Entry->LastAttemptStatus == LAST_ATTEMPT_STATUS_SUCCESS) {
+        if (FmpImageInfoBuf->LastAttemptStatus != LAST_ATTEMPT_STATUS_SUCCESS) {
+          Entry->LastAttemptStatus = FmpImageInfoBuf->LastAttemptStatus;
+          Entry->LastAttemptVersion = FmpImageInfoBuf->LastAttemptVersion;
+        } else {
+          Entry->LastAttemptVersion =
+            MIN (
+              FmpImageInfoBuf->LastAttemptVersion,
+              Entry->LastAttemptVersion
+              );
+        }
+      }
+    }
+
+    return EFI_SUCCESS;
   }
 
   //
-  // ESRT table has enough room for the new entry so add new entry
-  //
-  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(((UINT8 *)(*Table)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE));
+  // Add a new ESRT Table Entry
   //
-  // Move to the location of new entry
-  //
-  Entry = Entry + (*Table)->FwResourceCount;
+  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1) + Table->FwResourceCount;
 
   CopyGuid (&Entry->FwClass, &FmpImageInfoBuf->ImageTypeId);
 
@@ -316,19 +284,87 @@ CreateEsrtEntry (
   }
 
   //
-  // VERSION 3 supports hardware instance
+  // Increment the number of active ESRT Table Entries
   //
-  (*HardwareInstances)[(*Table)->FwResourceCount] = 0;
-  if (FmpVersion >= 3) {
-    (*HardwareInstances)[(*Table)->FwResourceCount] = FmpImageInfoBuf->HardwareInstance;
+  Table->FwResourceCount++;
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Function to retrieve the EFI_FIRMWARE_IMAGE_DESCRIPTOR from an FMP Instance.
+  The returned buffer is allocated using AllocatePool() and must be freed by the
+  caller using FreePool().
+
+  @param[in]  Fmp                        Pointer to an EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
+  @param[out] FmpImageInfoDescriptorVer  Pointer to the version number associated
+                                         with the returned EFI_FIRMWARE_IMAGE_DESCRIPTOR.
+  @param[out] FmpImageInfoCount          Pointer to the number of the returned
+                                         EFI_FIRMWARE_IMAGE_DESCRIPTORs.
+  @param[out] DescriptorSize             Pointer to the size, in bytes, of each
+                                         returned EFI_FIRMWARE_IMAGE_DESCRIPTOR.
+
+  @return  Pointer to the retrieved EFI_FIRMWARE_IMAGE_DESCRIPTOR.  If the
+           descriptor can not be retrieved, then NULL is returned.
+
+**/
+EFI_FIRMWARE_IMAGE_DESCRIPTOR *
+FmpGetFirmwareImageDescriptor (
+  IN  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp,
+  OUT UINT32                            *FmpImageInfoDescriptorVer,
+  OUT UINT8                             *FmpImageInfoCount,
+  OUT UINTN                             *DescriptorSize
+  )
+{
+  EFI_STATUS                     Status;
+  UINTN                          ImageInfoSize;
+  UINT32                         PackageVersion;
+  CHAR16                         *PackageVersionName;
+  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
+
+  ImageInfoSize = 0;
+  Status = Fmp->GetImageInfo (
+                  Fmp,                        // FMP Pointer
+                  &ImageInfoSize,             // Buffer Size (in this case 0)
+                  NULL,                       // NULL so we can get size
+                  FmpImageInfoDescriptorVer,  // DescriptorVersion
+                  FmpImageInfoCount,          // DescriptorCount
+                  DescriptorSize,             // DescriptorSize
+                  &PackageVersion,            // PackageVersion
+                  &PackageVersionName         // PackageVersionName
+                  );
+  if (Status != EFI_BUFFER_TOO_SMALL) {
+    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected Failure in GetImageInfo.  Status = %r\n", Status));
+    return NULL;
   }
 
-  //
-  // Increment resource count
-  //
-  (*Table)->FwResourceCount++;
+  FmpImageInfoBuf = AllocateZeroPool (ImageInfoSize);
+  if (FmpImageInfoBuf == NULL) {
+    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get memory for FMP descriptor.\n"));
+    return NULL;
+  }
 
-  return EFI_SUCCESS;
+  PackageVersionName = NULL;
+  Status = Fmp->GetImageInfo (
+                  Fmp,                        // FMP Pointer
+                  &ImageInfoSize,             // ImageInfoSize
+                  FmpImageInfoBuf,            // ImageInfo
+                  FmpImageInfoDescriptorVer,  // DescriptorVersion
+                  FmpImageInfoCount,          // DescriptorCount
+                  DescriptorSize,             // DescriptorSize
+                  &PackageVersion,            // PackageVersion
+                  &PackageVersionName         // PackageVersionName
+                  );
+  if (PackageVersionName != NULL) {
+    FreePool (PackageVersionName);
+  }
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in GetImageInfo.  Status = %r\n", Status));
+    FreePool (FmpImageInfoBuf);
+    return NULL;
+  }
+
+  return FmpImageInfoBuf;
 }
 
 /**
@@ -344,31 +380,26 @@ CreateFmpBasedEsrt (
   VOID
   )
 {
-  EFI_STATUS                        Status;
-  EFI_SYSTEM_RESOURCE_TABLE         *Table;
-  UINT64                            *HardwareInstances;
-  UINTN                             NoProtocols;
-  VOID                              **Buffer;
-  UINTN                             Index;
-  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp;
-  UINTN                             DescriptorSize;
-  EFI_FIRMWARE_IMAGE_DESCRIPTOR     *FmpImageInfoBuf;
-  EFI_FIRMWARE_IMAGE_DESCRIPTOR     *FmpImageInfoBufOrg;
-  UINT8                             FmpImageInfoCount;
-  UINT32                            FmpImageInfoDescriptorVer;
-  UINTN                             ImageInfoSize;
-  UINT32                            PackageVersion;
-  CHAR16                            *PackageVersionName;
+  EFI_STATUS                     Status;
+  UINTN                          NoProtocols;
+  VOID                           **Buffer;
+  UINTN                          Index;
+  UINT32                         FmpImageInfoDescriptorVer;
+  UINT8                          FmpImageInfoCount;
+  UINTN                          DescriptorSize;
+  UINT32                         NumberOfDescriptors;
+  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
+  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *OrgFmpImageInfoBuf;
+  EFI_SYSTEM_RESOURCE_TABLE      *Table;
+  GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances;
 
   Status             = EFI_SUCCESS;
-  Table              = NULL;
-  HardwareInstances  = NULL;
   NoProtocols        = 0;
   Buffer             = NULL;
-  PackageVersionName = NULL;
   FmpImageInfoBuf    = NULL;
-  FmpImageInfoBufOrg = NULL;
-  Fmp                = NULL;
+  OrgFmpImageInfoBuf = NULL;
+  Table              = NULL;
+  HardwareInstances  = NULL;
 
   Status = EfiLocateProtocolBuffer (
              &gEfiFirmwareManagementProtocolGuid,
@@ -380,77 +411,64 @@ CreateFmpBasedEsrt (
   }
 
   //
-  // Allocate Memory for tables
+  // Count the total number of EFI_FIRMWARE_IMAGE_DESCRIPTORs
+  //
+  for (Index = 0, NumberOfDescriptors = 0; Index < NoProtocols; Index++) {
+    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor (
+                        (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
+                        &FmpImageInfoDescriptorVer,
+                        &FmpImageInfoCount,
+                        &DescriptorSize
+                        );
+    if (FmpImageInfoBuf != NULL) {
+      NumberOfDescriptors += FmpImageInfoCount;
+      FreePool (FmpImageInfoBuf);
+    }
+  }
+
+  //
+  // Allocate ESRT Table and GUID/HardwareInstance table
   //
   Table = AllocateZeroPool (
-             (GROWTH_STEP * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE)
+             (NumberOfDescriptors * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE)
              );
   if (Table == NULL) {
     DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory for ESRT.\n"));
-    gBS->FreePool (Buffer);
+    FreePool (Buffer);
     return NULL;
   }
 
-  HardwareInstances = AllocateZeroPool (GROWTH_STEP * sizeof (UINT64));
+  HardwareInstances = AllocateZeroPool (NumberOfDescriptors * sizeof (GUID_HARDWAREINSTANCE_PAIR));
   if (HardwareInstances == NULL) {
     DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to allocate memory for HW Instance Table.\n"));
-    gBS->FreePool (Table);
-    gBS->FreePool (Buffer);
+    FreePool (Table);
+    FreePool (Buffer);
     return NULL;
   }
 
+  //
+  // Initialize ESRT Table
+  //
   Table->FwResourceCount    = 0;
-  Table->FwResourceCountMax = GROWTH_STEP;
+  Table->FwResourceCountMax = NumberOfDescriptors;
   Table->FwResourceVersion  = EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION;
 
+  NumberOfDescriptors = 0;
   for (Index = 0; Index < NoProtocols; Index++) {
-    Fmp = (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index];
-
-    ImageInfoSize = 0;
-    Status = Fmp->GetImageInfo (
-                    Fmp,                         // FMP Pointer
-                    &ImageInfoSize,              // Buffer Size (in this case 0)
-                    NULL,                        // NULL so we can get size
-                    &FmpImageInfoDescriptorVer,  // DescriptorVersion
-                    &FmpImageInfoCount,          // DescriptorCount
-                    &DescriptorSize,             // DescriptorSize
-                    &PackageVersion,             // PackageVersion
-                    &PackageVersionName          // PackageVersionName
-                    );
-
-    if (Status != EFI_BUFFER_TOO_SMALL) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected Failure in GetImageInfo.  Status = %r\n", Status));
-      continue;
-    }
-
-    FmpImageInfoBuf = AllocateZeroPool (ImageInfoSize);
+    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor (
+                        (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
+                        &FmpImageInfoDescriptorVer,
+                        &FmpImageInfoCount,
+                        &DescriptorSize
+                        );
     if (FmpImageInfoBuf == NULL) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get memory for descriptors.\n"));
-      continue;
-    }
-
-    FmpImageInfoBufOrg = FmpImageInfoBuf;
-    PackageVersionName = NULL;
-    Status = Fmp->GetImageInfo (
-                    Fmp,
-                    &ImageInfoSize,              // ImageInfoSize
-                    FmpImageInfoBuf,             // ImageInfo
-                    &FmpImageInfoDescriptorVer,  // DescriptorVersion
-                    &FmpImageInfoCount,          // DescriptorCount
-                    &DescriptorSize,             // DescriptorSize
-                    &PackageVersion,             // PackageVersion
-                    &PackageVersionName          // PackageVersionName
-                    );
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in GetImageInfo.  Status = %r\n", Status));
-      FreePool (FmpImageInfoBufOrg);
-      FmpImageInfoBufOrg = NULL;
       continue;
     }
 
     //
     // Check each descriptor and read from the one specified
     //
+    OrgFmpImageInfoBuf = FmpImageInfoBuf;
     while (FmpImageInfoCount > 0) {
       //
       // If the descriptor has the IN USE bit set, create ESRT entry otherwise ignore.
@@ -459,7 +477,7 @@ CreateFmpBasedEsrt (
         //
         // Create ESRT entry
         //
-        CreateEsrtEntry (&Table, &HardwareInstances, FmpImageInfoBuf, FmpImageInfoDescriptorVer);
+        CreateEsrtEntry (Table, HardwareInstances, &NumberOfDescriptors, FmpImageInfoBuf, FmpImageInfoDescriptorVer);
       }
       FmpImageInfoCount--;
       //
@@ -468,15 +486,12 @@ CreateFmpBasedEsrt (
       FmpImageInfoBuf = (EFI_FIRMWARE_IMAGE_DESCRIPTOR *)(((UINT8 *)FmpImageInfoBuf) + DescriptorSize);
     }
 
-    if (PackageVersionName != NULL) {
-      FreePool (PackageVersionName);
-      PackageVersionName = NULL;
-    }
-    FreePool (FmpImageInfoBufOrg);
-    FmpImageInfoBufOrg = NULL;
+    FreePool (OrgFmpImageInfoBuf);
+    OrgFmpImageInfoBuf = NULL;
   }
 
-  gBS->FreePool (Buffer);
+  FreePool (Buffer);
+  FreePool (HardwareInstances);
   return Table;
 }
 
-- 
2.20.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#39358): https://edk2.groups.io/g/devel/message/39358
Mute This Topic: https://groups.io/mt/31271286/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Staging/Bug_1525_FmpDevicePkg_MultipleControllers][PATCH] MdeModulePkg/EsrtFmpDxe: Detect duplicate GUID/HardwareInstance
Posted by Michael D Kinney 4 years, 12 months ago
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: Jin, Eric
> Sent: Sunday, April 21, 2019 10:48 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject:
> [Staging/Bug_1525_FmpDevicePkg_MultipleControllers][PAT
> CH] MdeModulePkg/EsrtFmpDxe: Detect duplicate
> GUID/HardwareInstance
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1525
> 
> Fix the following issues identified in #1525:
>     EsrtFmpDxe - The check for more than one instance
> of the same
>     GUID/HardwareInstance is not being done at the
> right point.
>     It is being done as EFI_FIRMWARE_IMAGE_DESCRIPTORs
> are being merged
>     into an ESRT Table Entry.  This means a case will
> be missed if
>     there are 3 EFI_FIRMWARE_IMAGE_DESCRIPTORs with the
> first and 3rd
>     one being having the same GUID/HardwareInstance.
> Instead, this check
>     should be performed across all
> EFI_FIRMWARE_IMAGE_DESCRIPTORs in the
>     entire handle database.
> 
> Build a table of GUID/HardwareInstance pairs from all
> the
> EFI_FIRMWARE_IMAGE_DESCRIPTORs from all FMP instances.
> If a duplicate
> is found, then generate a DEBUG_ERROR message, generate
> an ASSERT(),
> and ignore the duplicate EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> 
> Add an internal worker function called
> FmpGetFirmwareImageDescriptor()
> that retrieves the list of
> EFI_FIRMWARE_IMAGE_DESCRIPTORs from a single
> FMP instance and returns the descriptors in an
> allocated buffer.  This
> function is used to get the descriptors used to build
> the table of
> unique GUID/HardwareInstance pairs.  It is then used
> again to generate
> the ESRT Table from all the
> EFI_FIRMWARE_IMAGE_DESCRIPTORs from all the
> FMP instances.  2 passes are performed so the total
> number of
> descriptors is known.  This allows the correct sized
> buffers to always
> be allocated and the extra logic to grow tables is
> removed.
> 
> CC: Eric Jin <eric.jin@intel.com>
> CC: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Jin <eric.jin@intel.com>
> ---
>  MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c | 461
> ++++++++++----------
>  1 file changed, 238 insertions(+), 223 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> index 848bd44e9d..9c273fe79e 100644
> --- a/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> +++ b/MdeModulePkg/Universal/EsrtFmpDxe/EsrtFmp.c
> @@ -38,6 +38,22 @@
>  #include <Guid/EventGroup.h>
>  #include <Guid/SystemResourceTable.h>
> 
> +///
> +/// Structure for array of unique
> GUID/HardwareInstance pairs from the
> +/// current set of EFI_FIRMWARE_IMAGE_DESCRIPTORs from
> all FMP Protocols.
> +///
> +typedef struct {
> +  ///
> +  /// A unique GUID identifying the firmware image
> type.
> +  ///
> +  EFI_GUID                      ImageTypeGuid;
> +  ///
> +  /// An optional number to identify the unique
> hardware instance within the
> +  /// system for devices that may have multiple
> instances whenever possible.
> +  ///
> +  UINT64                        HardwareInstance;
> +} GUID_HARDWAREINSTANCE_PAIR;
> +
>  /**
>   Print ESRT to debug console.
> 
> @@ -50,11 +66,6 @@ PrintTable (
>    IN EFI_SYSTEM_RESOURCE_TABLE  *Table
>    );
> 
> -//
> -// Number of ESRT entries to grow by each time we run
> out of room
> -//
> -#define GROWTH_STEP  10
> -
>  /**
>    Install EFI System Resource Table into the UEFI
> Configuration Table
> 
> @@ -118,172 +129,129 @@ IsSystemFmp (
>  }
> 
>  /**
> -  Function to create a single ESRT Entry and add it to
> the ESRT
> -  given a FMP descriptor.  If the guid is already in
> the ESRT, then the ESRT
> -  entry is updated.  The ESRT will grow if it does not
> have enough room.
> -
> -  @param[in, out] Table             On input, pointer
> to the pointer to the ESRT.
> -                                    On output, same as
> input or pointer to the pointer
> -                                    to new enlarged
> ESRT.
> -  @param[in]      FmpImageInfoBuf   Pointer to the
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> -  @param[in]      FmpVersion        FMP Version.
> -
> -  @return  Status code.
> +  Function to create a single ESRT Entry and add it to
> the ESRT with
> +  a given FMP descriptor.  If the GUID is already in
> the ESRT, then the ESRT
> +  entry is updated.
> +
> +  @param[in,out] Table                Pointer to the
> ESRT Table.
> +  @param[in,out] HardwareInstances    Pointer to the
> GUID_HARDWAREINSTANCE_PAIR.
> +  @param[in,out] NumberOfDescriptors  The number of
> EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> +  @param[in]     FmpImageInfoBuf      Pointer to the
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> +  @param[in]     FmpVersion           FMP Version.
> +
> +  @retval  EFI_SUCCESS     FmpImageInfoBuf was use to
> fill in a new ESRT entry
> +                           in Table.
> +  @retval  EFI_SUCCESS     The ImageTypeId GUID in
> FmpImageInfoBuf matches an
> +                           existing ESRT entry in
> Table, and the information
> +                           from FmpImageInfoBuf was
> merged into the the existing
> +                           ESRT entry.
> +  @retval  EFI_UNSPOORTED  The GUID/HardareInstance in
> FmpImageInfoBuf has is a
> +                           duplicate.
> 
>  **/
>  EFI_STATUS
>  CreateEsrtEntry (
> -  IN OUT EFI_SYSTEM_RESOURCE_TABLE  **Table,
> -  IN OUT UINT64
> **HardwareInstances,
> -  IN EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf,
> -  IN UINT32                         FmpVersion
> +  IN OUT EFI_SYSTEM_RESOURCE_TABLE      *Table,
> +  IN OUT GUID_HARDWAREINSTANCE_PAIR
> *HardwareInstances,
> +  IN OUT UINT32
> *NumberOfDescriptors,
> +  IN     EFI_FIRMWARE_IMAGE_DESCRIPTOR
> *FmpImageInfoBuf,
> +  IN     UINT32                         FmpVersion
>    )
>  {
>    UINTN                      Index;
>    EFI_SYSTEM_RESOURCE_ENTRY  *Entry;
> -  UINTN                      NewSize;
> -  EFI_SYSTEM_RESOURCE_TABLE  *NewTable;
> -  UINT64                     *NewHardwareInstances;
>    UINT64                     FmpHardwareInstance;
> 
> -  Index = 0;
> -  Entry = NULL;
> +  FmpHardwareInstance = 0;
> +  if (FmpVersion >= 3) {
> +    FmpHardwareInstance = FmpImageInfoBuf-
> >HardwareInstance;
> +  }
> 
> -  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)((*Table) + 1);
>    //
> -  // Check to see if GUID is already in the table
> +  // Check to see of FmpImageInfoBuf
> GUID/HardwareInstance is unique
>    //
> -  for (Index = 0; Index < (*Table)->FwResourceCount;
> Index++) {
> -    if (CompareGuid (&Entry->FwClass,
> &FmpImageInfoBuf->ImageTypeId)) {
> -      //
> -      // If HardwareInstance in ESRT and
> FmpImageInfoBuf are the same value
> -      // for the same ImageTypeId GUID, then there is
> more than one FMP
> -      // instance for the same FW device, which is an
> error condition.
> -      // If FmpVersion is less than 3, then assume
> HardwareInstance is 0.
> -      //
> -      FmpHardwareInstance = 0;
> -      if (FmpVersion >= 3) {
> -        FmpHardwareInstance = FmpImageInfoBuf-
> >HardwareInstance;
> -      }
> -      if ((*HardwareInstances)[Index] ==
> FmpHardwareInstance) {
> -        DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: ESRT Entry
> already exists for FMP Instance with GUID %g and
> HardwareInstance %016lx\n", &Entry->FwClass,
> (*HardwareInstances)[Index]));
> -        ASSERT ((*HardwareInstances)[Index] !=
> FmpHardwareInstance);
> +  for (Index = 0; Index < *NumberOfDescriptors;
> Index++) {
> +    if (CompareGuid
> (&HardwareInstances[Index].ImageTypeGuid,
> &FmpImageInfoBuf->ImageTypeId)) {
> +      if (HardwareInstances[Index].HardwareInstance ==
> FmpHardwareInstance) {
> +        DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate
> firmware image descriptor with GUID %g
> HardwareInstance:0x%x\n", &FmpImageInfoBuf-
> >ImageTypeId, FmpHardwareInstance));
> +        ASSERT (
> +          !CompareGuid
> (&HardwareInstances[Index].ImageTypeGuid,
> &FmpImageInfoBuf->ImageTypeId) ||
> +          HardwareInstances[Index].HardwareInstance !=
> FmpHardwareInstance
> +          );
>          return EFI_UNSUPPORTED;
>        }
> -
> -      DEBUG ((DEBUG_INFO, "EsrtFmpDxe: ESRT Entry
> already exists for FMP Instance with GUID %g\n",
> &Entry->FwClass));
> -
> -      //
> -      // Set ESRT FwVersion to the smaller of the two
> values
> -      //
> -      Entry->FwVersion = MIN (FmpImageInfoBuf-
> >Version, Entry->FwVersion);
> -
> -      //
> -      // VERSION 2 has Lowest Supported
> -      //
> -      if (FmpVersion >= 2) {
> -        //
> -        // Set ESRT LowestSupportedFwVersion to the
> smaller of the two values
> -        //
> -        Entry->LowestSupportedFwVersion =
> -          MIN (
> -            FmpImageInfoBuf-
> >LowestSupportedImageVersion,
> -            Entry->LowestSupportedFwVersion
> -            );
> -      }
> -
> -      //
> -      // VERSION 3 supports last attempt values
> -      //
> -      if (FmpVersion >= 3) {
> -        //
> -        // Update the ESRT entry with the last attempt
> status and last attempt
> -        // version from the first FMP instance whose
> last attempt status is not
> -        // SUCCESS.  If all FMP instances are SUCCESS,
> then set version to the
> -        // smallest value from all FMP instances.
> -        //
> -        if (Entry->LastAttemptStatus ==
> LAST_ATTEMPT_STATUS_SUCCESS) {
> -          if (FmpImageInfoBuf->LastAttemptStatus !=
> LAST_ATTEMPT_STATUS_SUCCESS) {
> -            Entry->LastAttemptStatus =
> FmpImageInfoBuf->LastAttemptStatus;
> -            Entry->LastAttemptVersion =
> FmpImageInfoBuf->LastAttemptVersion;
> -          } else {
> -            Entry->LastAttemptVersion =
> -              MIN (
> -                FmpImageInfoBuf->LastAttemptVersion,
> -                Entry->LastAttemptVersion
> -                );
> -          }
> -        }
> -      }
> -
> -      return EFI_SUCCESS;
>      }
> -    Entry++;
>    }
> 
>    //
> -  // Grow table if needed
> +  // Record new GUID/HardwareInstance pair
>    //
> -  if ((*Table)->FwResourceCount >= (*Table)-
> >FwResourceCountMax) {
> -    NewSize  = (((*Table)->FwResourceCountMax +
> GROWTH_STEP) * sizeof (EFI_SYSTEM_RESOURCE_ENTRY)) +
> sizeof (EFI_SYSTEM_RESOURCE_TABLE);
> -    NewTable = AllocateZeroPool (NewSize);
> -    if (NewTable == NULL) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> allocate memory larger table for ESRT. \n"));
> -      return EFI_OUT_OF_RESOURCES;
> -    }
> -    //
> -    // Copy the whole old table into new table buffer
> -    //
> -    CopyMem (
> -      NewTable,
> -      (*Table),
> -      (((*Table)->FwResourceCountMax) * sizeof
> (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> (EFI_SYSTEM_RESOURCE_TABLE)
> -      );
> -    //
> -    // Update max
> -    //
> -    NewTable->FwResourceCountMax = NewTable-
> >FwResourceCountMax + GROWTH_STEP;
> -    //
> -    // Free old table
> -    //
> -    FreePool ((*Table));
> -    //
> -    // Reassign pointer to new table.
> -    //
> -    (*Table) = NewTable;
> +  CopyGuid
> (&HardwareInstances[*NumberOfDescriptors].ImageTypeGuid
> , &FmpImageInfoBuf->ImageTypeId);
> +
> HardwareInstances[*NumberOfDescriptors].HardwareInstanc
> e = FmpHardwareInstance;
> +  *NumberOfDescriptors = *NumberOfDescriptors + 1;
> +
> +  DEBUG ((DEBUG_INFO, "EsrtFmpDxe: Add new image
> descriptor with GUID %g HardwareInstance:0x%x\n",
> &FmpImageInfoBuf->ImageTypeId, FmpHardwareInstance));
> 
> -    NewSize  = ((*Table)->FwResourceCountMax) * sizeof
> (UINT64);
> -    NewHardwareInstances = AllocateZeroPool (NewSize);
> -    if (NewHardwareInstances == NULL) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> allocate memory larger table for Hardware
> Instances.\n"));
> -      return EFI_OUT_OF_RESOURCES;
> +  //
> +  // Check to see if GUID is already in the ESRT table
> +  //
> +  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1);
> +  for (Index = 0; Index < Table->FwResourceCount;
> Index++, Entry++) {
> +    if (!CompareGuid (&Entry->FwClass,
> &FmpImageInfoBuf->ImageTypeId)) {
> +      continue;
>      }
> +    DEBUG ((DEBUG_INFO, "EsrtFmpDxe: ESRT Entry
> already exists for FMP Instance with GUID %g\n",
> &Entry->FwClass));
> +
>      //
> -    // Copy the whole old table into new table buffer
> +    // Set ESRT FwVersion to the smaller of the two
> values
>      //
> -    CopyMem (
> -      NewHardwareInstances,
> -      (*HardwareInstances),
> -      ((*Table)->FwResourceCountMax) * sizeof (UINT64)
> -      );
> +    Entry->FwVersion = MIN (FmpImageInfoBuf->Version,
> Entry->FwVersion);
> +
>      //
> -    // Free old table
> +    // VERSION 2 has Lowest Supported
>      //
> -    FreePool ((*HardwareInstances));
> +    if (FmpVersion >= 2) {
> +      //
> +      // Set ESRT LowestSupportedFwVersion to the
> smaller of the two values
> +      //
> +      Entry->LowestSupportedFwVersion =
> +        MIN (
> +          FmpImageInfoBuf-
> >LowestSupportedImageVersion,
> +          Entry->LowestSupportedFwVersion
> +          );
> +    }
> +
>      //
> -    // Reassign pointer to new table.
> +    // VERSION 3 supports last attempt values
>      //
> -    (*HardwareInstances) = NewHardwareInstances;
> +    if (FmpVersion >= 3) {
> +      //
> +      // Update the ESRT entry with the last attempt
> status and last attempt
> +      // version from the first FMP instance whose
> last attempt status is not
> +      // SUCCESS.  If all FMP instances are SUCCESS,
> then set version to the
> +      // smallest value from all FMP instances.
> +      //
> +      if (Entry->LastAttemptStatus ==
> LAST_ATTEMPT_STATUS_SUCCESS) {
> +        if (FmpImageInfoBuf->LastAttemptStatus !=
> LAST_ATTEMPT_STATUS_SUCCESS) {
> +          Entry->LastAttemptStatus = FmpImageInfoBuf-
> >LastAttemptStatus;
> +          Entry->LastAttemptVersion = FmpImageInfoBuf-
> >LastAttemptVersion;
> +        } else {
> +          Entry->LastAttemptVersion =
> +            MIN (
> +              FmpImageInfoBuf->LastAttemptVersion,
> +              Entry->LastAttemptVersion
> +              );
> +        }
> +      }
> +    }
> +
> +    return EFI_SUCCESS;
>    }
> 
>    //
> -  // ESRT table has enough room for the new entry so
> add new entry
> -  //
> -  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(((UINT8
> *)(*Table)) + sizeof (EFI_SYSTEM_RESOURCE_TABLE));
> +  // Add a new ESRT Table Entry
>    //
> -  // Move to the location of new entry
> -  //
> -  Entry = Entry + (*Table)->FwResourceCount;
> +  Entry = (EFI_SYSTEM_RESOURCE_ENTRY *)(Table + 1) +
> Table->FwResourceCount;
> 
>    CopyGuid (&Entry->FwClass, &FmpImageInfoBuf-
> >ImageTypeId);
> 
> @@ -316,19 +284,87 @@ CreateEsrtEntry (
>    }
> 
>    //
> -  // VERSION 3 supports hardware instance
> +  // Increment the number of active ESRT Table Entries
>    //
> -  (*HardwareInstances)[(*Table)->FwResourceCount] = 0;
> -  if (FmpVersion >= 3) {
> -    (*HardwareInstances)[(*Table)->FwResourceCount] =
> FmpImageInfoBuf->HardwareInstance;
> +  Table->FwResourceCount++;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Function to retrieve the
> EFI_FIRMWARE_IMAGE_DESCRIPTOR from an FMP Instance.
> +  The returned buffer is allocated using
> AllocatePool() and must be freed by the
> +  caller using FreePool().
> +
> +  @param[in]  Fmp                        Pointer to an
> EFI_FIRMWARE_MANAGEMENT_PROTOCOL.
> +  @param[out] FmpImageInfoDescriptorVer  Pointer to
> the version number associated
> +                                         with the
> returned EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> +  @param[out] FmpImageInfoCount          Pointer to
> the number of the returned
> +
> EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> +  @param[out] DescriptorSize             Pointer to
> the size, in bytes, of each
> +                                         returned
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> +
> +  @return  Pointer to the retrieved
> EFI_FIRMWARE_IMAGE_DESCRIPTOR.  If the
> +           descriptor can not be retrieved, then NULL
> is returned.
> +
> +**/
> +EFI_FIRMWARE_IMAGE_DESCRIPTOR *
> +FmpGetFirmwareImageDescriptor (
> +  IN  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp,
> +  OUT UINT32
> *FmpImageInfoDescriptorVer,
> +  OUT UINT8
> *FmpImageInfoCount,
> +  OUT UINTN
> *DescriptorSize
> +  )
> +{
> +  EFI_STATUS                     Status;
> +  UINTN                          ImageInfoSize;
> +  UINT32                         PackageVersion;
> +  CHAR16                         *PackageVersionName;
> +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
> +
> +  ImageInfoSize = 0;
> +  Status = Fmp->GetImageInfo (
> +                  Fmp,                        // FMP
> Pointer
> +                  &ImageInfoSize,             //
> Buffer Size (in this case 0)
> +                  NULL,                       // NULL
> so we can get size
> +                  FmpImageInfoDescriptorVer,  //
> DescriptorVersion
> +                  FmpImageInfoCount,          //
> DescriptorCount
> +                  DescriptorSize,             //
> DescriptorSize
> +                  &PackageVersion,            //
> PackageVersion
> +                  &PackageVersionName         //
> PackageVersionName
> +                  );
> +  if (Status != EFI_BUFFER_TOO_SMALL) {
> +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected
> Failure in GetImageInfo.  Status = %r\n", Status));
> +    return NULL;
>    }
> 
> -  //
> -  // Increment resource count
> -  //
> -  (*Table)->FwResourceCount++;
> +  FmpImageInfoBuf = AllocateZeroPool (ImageInfoSize);
> +  if (FmpImageInfoBuf == NULL) {
> +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get
> memory for FMP descriptor.\n"));
> +    return NULL;
> +  }
> 
> -  return EFI_SUCCESS;
> +  PackageVersionName = NULL;
> +  Status = Fmp->GetImageInfo (
> +                  Fmp,                        // FMP
> Pointer
> +                  &ImageInfoSize,             //
> ImageInfoSize
> +                  FmpImageInfoBuf,            //
> ImageInfo
> +                  FmpImageInfoDescriptorVer,  //
> DescriptorVersion
> +                  FmpImageInfoCount,          //
> DescriptorCount
> +                  DescriptorSize,             //
> DescriptorSize
> +                  &PackageVersion,            //
> PackageVersion
> +                  &PackageVersionName         //
> PackageVersionName
> +                  );
> +  if (PackageVersionName != NULL) {
> +    FreePool (PackageVersionName);
> +  }
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in
> GetImageInfo.  Status = %r\n", Status));
> +    FreePool (FmpImageInfoBuf);
> +    return NULL;
> +  }
> +
> +  return FmpImageInfoBuf;
>  }
> 
>  /**
> @@ -344,31 +380,26 @@ CreateFmpBasedEsrt (
>    VOID
>    )
>  {
> -  EFI_STATUS                        Status;
> -  EFI_SYSTEM_RESOURCE_TABLE         *Table;
> -  UINT64
> *HardwareInstances;
> -  UINTN                             NoProtocols;
> -  VOID                              **Buffer;
> -  UINTN                             Index;
> -  EFI_FIRMWARE_MANAGEMENT_PROTOCOL  *Fmp;
> -  UINTN                             DescriptorSize;
> -  EFI_FIRMWARE_IMAGE_DESCRIPTOR     *FmpImageInfoBuf;
> -  EFI_FIRMWARE_IMAGE_DESCRIPTOR
> *FmpImageInfoBufOrg;
> -  UINT8                             FmpImageInfoCount;
> -  UINT32
> FmpImageInfoDescriptorVer;
> -  UINTN                             ImageInfoSize;
> -  UINT32                            PackageVersion;
> -  CHAR16
> *PackageVersionName;
> +  EFI_STATUS                     Status;
> +  UINTN                          NoProtocols;
> +  VOID                           **Buffer;
> +  UINTN                          Index;
> +  UINT32
> FmpImageInfoDescriptorVer;
> +  UINT8                          FmpImageInfoCount;
> +  UINTN                          DescriptorSize;
> +  UINT32                         NumberOfDescriptors;
> +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *FmpImageInfoBuf;
> +  EFI_FIRMWARE_IMAGE_DESCRIPTOR  *OrgFmpImageInfoBuf;
> +  EFI_SYSTEM_RESOURCE_TABLE      *Table;
> +  GUID_HARDWAREINSTANCE_PAIR     *HardwareInstances;
> 
>    Status             = EFI_SUCCESS;
> -  Table              = NULL;
> -  HardwareInstances  = NULL;
>    NoProtocols        = 0;
>    Buffer             = NULL;
> -  PackageVersionName = NULL;
>    FmpImageInfoBuf    = NULL;
> -  FmpImageInfoBufOrg = NULL;
> -  Fmp                = NULL;
> +  OrgFmpImageInfoBuf = NULL;
> +  Table              = NULL;
> +  HardwareInstances  = NULL;
> 
>    Status = EfiLocateProtocolBuffer (
>               &gEfiFirmwareManagementProtocolGuid,
> @@ -380,77 +411,64 @@ CreateFmpBasedEsrt (
>    }
> 
>    //
> -  // Allocate Memory for tables
> +  // Count the total number of
> EFI_FIRMWARE_IMAGE_DESCRIPTORs
> +  //
> +  for (Index = 0, NumberOfDescriptors = 0; Index <
> NoProtocols; Index++) {
> +    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor (
> +
> (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
> +                        &FmpImageInfoDescriptorVer,
> +                        &FmpImageInfoCount,
> +                        &DescriptorSize
> +                        );
> +    if (FmpImageInfoBuf != NULL) {
> +      NumberOfDescriptors += FmpImageInfoCount;
> +      FreePool (FmpImageInfoBuf);
> +    }
> +  }
> +
> +  //
> +  // Allocate ESRT Table and GUID/HardwareInstance
> table
>    //
>    Table = AllocateZeroPool (
> -             (GROWTH_STEP * sizeof
> (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> (EFI_SYSTEM_RESOURCE_TABLE)
> +             (NumberOfDescriptors * sizeof
> (EFI_SYSTEM_RESOURCE_ENTRY)) + sizeof
> (EFI_SYSTEM_RESOURCE_TABLE)
>               );
>    if (Table == NULL) {
>      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> allocate memory for ESRT.\n"));
> -    gBS->FreePool (Buffer);
> +    FreePool (Buffer);
>      return NULL;
>    }
> 
> -  HardwareInstances = AllocateZeroPool (GROWTH_STEP *
> sizeof (UINT64));
> +  HardwareInstances = AllocateZeroPool
> (NumberOfDescriptors * sizeof
> (GUID_HARDWAREINSTANCE_PAIR));
>    if (HardwareInstances == NULL) {
>      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to
> allocate memory for HW Instance Table.\n"));
> -    gBS->FreePool (Table);
> -    gBS->FreePool (Buffer);
> +    FreePool (Table);
> +    FreePool (Buffer);
>      return NULL;
>    }
> 
> +  //
> +  // Initialize ESRT Table
> +  //
>    Table->FwResourceCount    = 0;
> -  Table->FwResourceCountMax = GROWTH_STEP;
> +  Table->FwResourceCountMax = NumberOfDescriptors;
>    Table->FwResourceVersion  =
> EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION;
> 
> +  NumberOfDescriptors = 0;
>    for (Index = 0; Index < NoProtocols; Index++) {
> -    Fmp = (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *)
> Buffer[Index];
> -
> -    ImageInfoSize = 0;
> -    Status = Fmp->GetImageInfo (
> -                    Fmp,                         //
> FMP Pointer
> -                    &ImageInfoSize,              //
> Buffer Size (in this case 0)
> -                    NULL,                        //
> NULL so we can get size
> -                    &FmpImageInfoDescriptorVer,  //
> DescriptorVersion
> -                    &FmpImageInfoCount,          //
> DescriptorCount
> -                    &DescriptorSize,             //
> DescriptorSize
> -                    &PackageVersion,             //
> PackageVersion
> -                    &PackageVersionName          //
> PackageVersionName
> -                    );
> -
> -    if (Status != EFI_BUFFER_TOO_SMALL) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Unexpected
> Failure in GetImageInfo.  Status = %r\n", Status));
> -      continue;
> -    }
> -
> -    FmpImageInfoBuf = AllocateZeroPool
> (ImageInfoSize);
> +    FmpImageInfoBuf = FmpGetFirmwareImageDescriptor (
> +
> (EFI_FIRMWARE_MANAGEMENT_PROTOCOL *) Buffer[Index],
> +                        &FmpImageInfoDescriptorVer,
> +                        &FmpImageInfoCount,
> +                        &DescriptorSize
> +                        );
>      if (FmpImageInfoBuf == NULL) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failed to get
> memory for descriptors.\n"));
> -      continue;
> -    }
> -
> -    FmpImageInfoBufOrg = FmpImageInfoBuf;
> -    PackageVersionName = NULL;
> -    Status = Fmp->GetImageInfo (
> -                    Fmp,
> -                    &ImageInfoSize,              //
> ImageInfoSize
> -                    FmpImageInfoBuf,             //
> ImageInfo
> -                    &FmpImageInfoDescriptorVer,  //
> DescriptorVersion
> -                    &FmpImageInfoCount,          //
> DescriptorCount
> -                    &DescriptorSize,             //
> DescriptorSize
> -                    &PackageVersion,             //
> PackageVersion
> -                    &PackageVersionName          //
> PackageVersionName
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Failure in
> GetImageInfo.  Status = %r\n", Status));
> -      FreePool (FmpImageInfoBufOrg);
> -      FmpImageInfoBufOrg = NULL;
>        continue;
>      }
> 
>      //
>      // Check each descriptor and read from the one
> specified
>      //
> +    OrgFmpImageInfoBuf = FmpImageInfoBuf;
>      while (FmpImageInfoCount > 0) {
>        //
>        // If the descriptor has the IN USE bit set,
> create ESRT entry otherwise ignore.
> @@ -459,7 +477,7 @@ CreateFmpBasedEsrt (
>          //
>          // Create ESRT entry
>          //
> -        CreateEsrtEntry (&Table, &HardwareInstances,
> FmpImageInfoBuf, FmpImageInfoDescriptorVer);
> +        CreateEsrtEntry (Table, HardwareInstances,
> &NumberOfDescriptors, FmpImageInfoBuf,
> FmpImageInfoDescriptorVer);
>        }
>        FmpImageInfoCount--;
>        //
> @@ -468,15 +486,12 @@ CreateFmpBasedEsrt (
>        FmpImageInfoBuf = (EFI_FIRMWARE_IMAGE_DESCRIPTOR
> *)(((UINT8 *)FmpImageInfoBuf) + DescriptorSize);
>      }
> 
> -    if (PackageVersionName != NULL) {
> -      FreePool (PackageVersionName);
> -      PackageVersionName = NULL;
> -    }
> -    FreePool (FmpImageInfoBufOrg);
> -    FmpImageInfoBufOrg = NULL;
> +    FreePool (OrgFmpImageInfoBuf);
> +    OrgFmpImageInfoBuf = NULL;
>    }
> 
> -  gBS->FreePool (Buffer);
> +  FreePool (Buffer);
> +  FreePool (HardwareInstances);
>    return Table;
>  }
> 
> --
> 2.20.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#39370): https://edk2.groups.io/g/devel/message/39370
Mute This Topic: https://groups.io/mt/31271286/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-