The QEMU_LOADER_WRITE_POINTER command instructs the firmware to write the
address of a field within a previously allocated/downloaded fw_cfg blob
into another (writeable) fw_cfg file at a specific offset.

Put differently, QEMU_LOADER_WRITE_POINTER propagates, to QEMU, the
address that QEMU_LOADER_ALLOCATE placed the designated fw_cfg blob at, as
adjusted for the given field inside the allocated blob.

The implementation is similar to that of QEMU_LOADER_ADD_POINTER. Since
here we "patch" a pointer object in "fw_cfg file space", not guest memory
space, we utilize the QemuFwCfgSkipBytes() and QemuFwCfgWriteBytes() APIs
completed in commit range 465663e9f128..7fcb73541299.

An interesting aspect is that QEMU_LOADER_WRITE_POINTER creates a
host-level reference to a guest memory location. Therefore, if we fail to
process the linker/loader script for any reason, we have to clear out
those references first, before we release the guest memory allocations in
response to the error.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 171 +++++++++++++++++++-
 1 file changed, 168 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 404589cad0b7..de827c2df204 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -350,10 +350,147 @@ ProcessCmdAddChecksum (
     AddChecksum->ResultOffset, AddChecksum->Start, AddChecksum->Length));
   return EFI_SUCCESS;
 }
 
 
+/**
+  Process a QEMU_LOADER_WRITE_POINTER command.
+
+  @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
+
+  @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
+                            structures created thus far.
+
+  @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
+                              WritePointer. Or, the WritePointer command
+                              references a file unknown to Tracker or the
+                              fw_cfg directory. Or, the pointer object to
+                              rewrite has invalid location, size, or initial
+                              relative value. Or, the pointer value to store
+                              does not fit in the given pointer size.
+
+  @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
+                              file has been written.
+**/
+STATIC
+EFI_STATUS
+ProcessCmdWritePointer (
+  IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
+  IN     CONST ORDERED_COLLECTION        *Tracker
+  )
+{
+  RETURN_STATUS            Status;
+  FIRMWARE_CONFIG_ITEM     PointerItem;
+  UINTN                    PointerItemSize;
+  ORDERED_COLLECTION_ENTRY *PointeeEntry;
+  BLOB                     *PointeeBlob;
+  UINT64                   PointerValue;
+
+  if (WritePointer->PointerFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0' ||
+      WritePointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') {
+    DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
+             &PointerItem, &PointerItemSize);
+  PointeeEntry = OrderedCollectionFind (Tracker, WritePointer->PointeeFile);
+  if (RETURN_ERROR (Status) || PointeeEntry == NULL) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: invalid fw_cfg file or blob reference \"%a\" / \"%a\"\n",
+      __FUNCTION__, WritePointer->PointerFile, WritePointer->PointeeFile));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  if ((WritePointer->PointerSize != 1 && WritePointer->PointerSize != 2 &&
+       WritePointer->PointerSize != 4 && WritePointer->PointerSize != 8) ||
+      (PointerItemSize < WritePointer->PointerSize) ||
+      (PointerItemSize - WritePointer->PointerSize <
+       WritePointer->PointerOffset)) {
+    DEBUG ((DEBUG_ERROR, "%a: invalid pointer location or size in \"%a\"\n",
+      __FUNCTION__, WritePointer->PointerFile));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  PointeeBlob = OrderedCollectionUserStruct (PointeeEntry);
+  PointerValue = WritePointer->PointeeOffset;
+  if (PointerValue >= PointeeBlob->Size) {
+    DEBUG ((DEBUG_ERROR, "%a: invalid PointeeOffset\n", __FUNCTION__));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  //
+  // The memory allocation system ensures that the address of the byte past the
+  // last byte of any allocated object is expressible (no wraparound).
+  //
+  ASSERT ((UINTN)PointeeBlob->Base <= MAX_ADDRESS - PointeeBlob->Size);
+
+  PointerValue += (UINT64)(UINTN)PointeeBlob->Base;
+  if (RShiftU64 (
+        RShiftU64 (PointerValue, WritePointer->PointerSize * 8 - 1), 1) != 0) {
+    DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
+      __FUNCTION__, WritePointer->PointerFile));
+    return EFI_PROTOCOL_ERROR;
+  }
+
+  QemuFwCfgSelectItem (PointerItem);
+  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
+  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
+
+  //
+  // Because QEMU has now learned PointeeBlob->Base, we must mark PointeeBlob
+  // as unreleasable, for the case when the whole linker/loader script is
+  // handled successfully.
+  //
+  PointeeBlob->HostsOnlyTableData = FALSE;
+
+  DEBUG ((DEBUG_VERBOSE, "%a: PointerFile=\"%a\" PointeeFile=\"%a\" "
+    "PointerOffset=0x%x PointeeOffset=0x%x PointerSize=%d\n", __FUNCTION__,
+    WritePointer->PointerFile, WritePointer->PointeeFile,
+    WritePointer->PointerOffset, WritePointer->PointeeOffset,
+    WritePointer->PointerSize));
+  return EFI_SUCCESS;
+}
+
+
+/**
+  Undo a QEMU_LOADER_WRITE_POINTER command.
+
+  This function revokes (zeroes out) a guest memory reference communicated to
+  QEMU earlier. The caller is responsible for invoking this function only on
+  such QEMU_LOADER_WRITE_POINTER commands that have been successfully processed
+  by ProcessCmdWritePointer().
+
+  @param[in] WritePointer  The QEMU_LOADER_WRITE_POINTER command to undo.
+**/
+STATIC
+VOID
+UndoCmdWritePointer (
+  IN CONST QEMU_LOADER_WRITE_POINTER *WritePointer
+  )
+{
+  RETURN_STATUS        Status;
+  FIRMWARE_CONFIG_ITEM PointerItem;
+  UINTN                PointerItemSize;
+  UINT64               PointerValue;
+
+  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
+             &PointerItem, &PointerItemSize);
+  ASSERT_RETURN_ERROR (Status);
+
+  PointerValue = 0;
+  QemuFwCfgSelectItem (PointerItem);
+  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
+  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
+
+  DEBUG ((DEBUG_VERBOSE,
+    "%a: PointerFile=\"%a\" PointerOffset=0x%x PointerSize=%d\n", __FUNCTION__,
+    WritePointer->PointerFile, WritePointer->PointerOffset,
+    WritePointer->PointerSize));
+}
+
+
 //
 // We'll be saving the keys of installed tables so that we can roll them back
 // in case of failure. 128 tables should be enough for anyone (TM).
 //
 #define INSTALLED_TABLES_MAX 128
@@ -559,10 +696,11 @@ InstallQemuFwCfgTables (
   EFI_STATUS               Status;
   FIRMWARE_CONFIG_ITEM     FwCfgItem;
   UINTN                    FwCfgSize;
   QEMU_LOADER_ENTRY        *LoaderStart;
   CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
+  CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
   ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
   UINTN                    OriginalPciAttributesCount;
   ORDERED_COLLECTION       *Tracker;
   UINTN                    *InstalledKey;
   INT32                    Installed;
@@ -595,10 +733,15 @@ InstallQemuFwCfgTables (
   }
 
   //
   // first pass: process the commands
   //
+  // "WritePointerSubsetEnd" points one past the last successful
+  // QEMU_LOADER_WRITE_POINTER command. Now when we're about to start the first
+  // pass, no such command has been encountered yet.
+  //
+  WritePointerSubsetEnd = LoaderStart;
   for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
     switch (LoaderEntry->Type) {
     case QemuLoaderCmdAllocate:
       Status = ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tracker);
       break;
@@ -611,25 +754,33 @@ InstallQemuFwCfgTables (
     case QemuLoaderCmdAddChecksum:
       Status = ProcessCmdAddChecksum (&LoaderEntry->Command.AddChecksum,
                  Tracker);
       break;
 
+    case QemuLoaderCmdWritePointer:
+        Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
+                   Tracker);
+        if (!EFI_ERROR (Status)) {
+          WritePointerSubsetEnd = LoaderEntry + 1;
+        }
+        break;
+
     default:
       DEBUG ((EFI_D_VERBOSE, "%a: unknown loader command: 0x%x\n",
         __FUNCTION__, LoaderEntry->Type));
       break;
     }
 
     if (EFI_ERROR (Status)) {
-      goto FreeTracker;
+      goto RollbackWritePointersAndFreeTracker;
     }
   }
 
   InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
   if (InstalledKey == NULL) {
     Status = EFI_OUT_OF_RESOURCES;
-    goto FreeTracker;
+    goto RollbackWritePointersAndFreeTracker;
   }
 
   //
   // second pass: identify and install ACPI tables
   //
@@ -656,11 +807,25 @@ InstallQemuFwCfgTables (
     DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
   }
 
   FreePool (InstalledKey);
 
-FreeTracker:
+RollbackWritePointersAndFreeTracker:
+  //
+  // In case of failure, revoke any allocation addresses that were communicated
+  // to QEMU previously, before we release all the blobs.
+  //
+  if (EFI_ERROR (Status)) {
+    LoaderEntry = WritePointerSubsetEnd;
+    while (LoaderEntry > LoaderStart) {
+      --LoaderEntry;
+      if (LoaderEntry->Type == QemuLoaderCmdWritePointer) {
+        UndoCmdWritePointer (&LoaderEntry->Command.WritePointer);
+      }
+    }
+  }
+
   //
   // Tear down the tracker infrastructure. Each fw_cfg blob will be left in
   // place only if we're exiting with success and the blob hosts data that is
   // not directly part of some ACPI table.
   //
-- 
2.9.3


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 2017-02-16 12:41:36, Laszlo Ersek wrote:
> The QEMU_LOADER_WRITE_POINTER command instructs the firmware to write the
> address of a field within a previously allocated/downloaded fw_cfg blob
> into another (writeable) fw_cfg file at a specific offset.
> 
> Put differently, QEMU_LOADER_WRITE_POINTER propagates, to QEMU, the
> address that QEMU_LOADER_ALLOCATE placed the designated fw_cfg blob at, as
> adjusted for the given field inside the allocated blob.
> 
> The implementation is similar to that of QEMU_LOADER_ADD_POINTER. Since
> here we "patch" a pointer object in "fw_cfg file space", not guest memory
> space, we utilize the QemuFwCfgSkipBytes() and QemuFwCfgWriteBytes() APIs
> completed in commit range 465663e9f128..7fcb73541299.
> 
> An interesting aspect is that QEMU_LOADER_WRITE_POINTER creates a
> host-level reference to a guest memory location. Therefore, if we fail to
> process the linker/loader script for any reason, we have to clear out
> those references first, before we release the guest memory allocations in
> response to the error.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 171 +++++++++++++++++++-
>  1 file changed, 168 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index 404589cad0b7..de827c2df204 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -350,10 +350,147 @@ ProcessCmdAddChecksum (
>      AddChecksum->ResultOffset, AddChecksum->Start, AddChecksum->Length));
>    return EFI_SUCCESS;
>  }
>  
>  
> +/**
> +  Process a QEMU_LOADER_WRITE_POINTER command.
> +
> +  @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
> +
> +  @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
> +                            structures created thus far.
> +
> +  @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
> +                              WritePointer. Or, the WritePointer command
> +                              references a file unknown to Tracker or the
> +                              fw_cfg directory. Or, the pointer object to
> +                              rewrite has invalid location, size, or initial
> +                              relative value. Or, the pointer value to store
> +                              does not fit in the given pointer size.
> +
> +  @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
> +                              file has been written.
> +**/
> +STATIC
> +EFI_STATUS
> +ProcessCmdWritePointer (
> +  IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
> +  IN     CONST ORDERED_COLLECTION        *Tracker
> +  )
> +{
> +  RETURN_STATUS            Status;
> +  FIRMWARE_CONFIG_ITEM     PointerItem;
> +  UINTN                    PointerItemSize;
> +  ORDERED_COLLECTION_ENTRY *PointeeEntry;
> +  BLOB                     *PointeeBlob;
> +  UINT64                   PointerValue;
> +
> +  if (WritePointer->PointerFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0' ||
> +      WritePointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') {
> +    DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
> +             &PointerItem, &PointerItemSize);
> +  PointeeEntry = OrderedCollectionFind (Tracker, WritePointer->PointeeFile);
> +  if (RETURN_ERROR (Status) || PointeeEntry == NULL) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: invalid fw_cfg file or blob reference \"%a\" / \"%a\"\n",
> +      __FUNCTION__, WritePointer->PointerFile, WritePointer->PointeeFile));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  if ((WritePointer->PointerSize != 1 && WritePointer->PointerSize != 2 &&
> +       WritePointer->PointerSize != 4 && WritePointer->PointerSize != 8) ||
> +      (PointerItemSize < WritePointer->PointerSize) ||
> +      (PointerItemSize - WritePointer->PointerSize <
> +       WritePointer->PointerOffset)) {
> +    DEBUG ((DEBUG_ERROR, "%a: invalid pointer location or size in \"%a\"\n",
> +      __FUNCTION__, WritePointer->PointerFile));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  PointeeBlob = OrderedCollectionUserStruct (PointeeEntry);
> +  PointerValue = WritePointer->PointeeOffset;
> +  if (PointerValue >= PointeeBlob->Size) {
> +    DEBUG ((DEBUG_ERROR, "%a: invalid PointeeOffset\n", __FUNCTION__));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  //
> +  // The memory allocation system ensures that the address of the byte past the
> +  // last byte of any allocated object is expressible (no wraparound).
> +  //
> +  ASSERT ((UINTN)PointeeBlob->Base <= MAX_ADDRESS - PointeeBlob->Size);
> +
> +  PointerValue += (UINT64)(UINTN)PointeeBlob->Base;
> +  if (RShiftU64 (
> +        RShiftU64 (PointerValue, WritePointer->PointerSize * 8 - 1), 1) != 0) {

What do you think of this instead?

  if (WritePointer->PointerSize < 8 &&
      RShiftU64 (PointerValue, WritePointer->PointerSize * 8) != 0) {

Patches 1-4 Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

> +    DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
> +      __FUNCTION__, WritePointer->PointerFile));
> +    return EFI_PROTOCOL_ERROR;
> +  }
> +
> +  QemuFwCfgSelectItem (PointerItem);
> +  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
> +  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
> +
> +  //
> +  // Because QEMU has now learned PointeeBlob->Base, we must mark PointeeBlob
> +  // as unreleasable, for the case when the whole linker/loader script is
> +  // handled successfully.
> +  //
> +  PointeeBlob->HostsOnlyTableData = FALSE;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: PointerFile=\"%a\" PointeeFile=\"%a\" "
> +    "PointerOffset=0x%x PointeeOffset=0x%x PointerSize=%d\n", __FUNCTION__,
> +    WritePointer->PointerFile, WritePointer->PointeeFile,
> +    WritePointer->PointerOffset, WritePointer->PointeeOffset,
> +    WritePointer->PointerSize));
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Undo a QEMU_LOADER_WRITE_POINTER command.
> +
> +  This function revokes (zeroes out) a guest memory reference communicated to
> +  QEMU earlier. The caller is responsible for invoking this function only on
> +  such QEMU_LOADER_WRITE_POINTER commands that have been successfully processed
> +  by ProcessCmdWritePointer().
> +
> +  @param[in] WritePointer  The QEMU_LOADER_WRITE_POINTER command to undo.
> +**/
> +STATIC
> +VOID
> +UndoCmdWritePointer (
> +  IN CONST QEMU_LOADER_WRITE_POINTER *WritePointer
> +  )
> +{
> +  RETURN_STATUS        Status;
> +  FIRMWARE_CONFIG_ITEM PointerItem;
> +  UINTN                PointerItemSize;
> +  UINT64               PointerValue;
> +
> +  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
> +             &PointerItem, &PointerItemSize);
> +  ASSERT_RETURN_ERROR (Status);
> +
> +  PointerValue = 0;
> +  QemuFwCfgSelectItem (PointerItem);
> +  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
> +  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
> +
> +  DEBUG ((DEBUG_VERBOSE,
> +    "%a: PointerFile=\"%a\" PointerOffset=0x%x PointerSize=%d\n", __FUNCTION__,
> +    WritePointer->PointerFile, WritePointer->PointerOffset,
> +    WritePointer->PointerSize));
> +}
> +
> +
>  //
>  // We'll be saving the keys of installed tables so that we can roll them back
>  // in case of failure. 128 tables should be enough for anyone (TM).
>  //
>  #define INSTALLED_TABLES_MAX 128
> @@ -559,10 +696,11 @@ InstallQemuFwCfgTables (
>    EFI_STATUS               Status;
>    FIRMWARE_CONFIG_ITEM     FwCfgItem;
>    UINTN                    FwCfgSize;
>    QEMU_LOADER_ENTRY        *LoaderStart;
>    CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
> +  CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
>    ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
>    UINTN                    OriginalPciAttributesCount;
>    ORDERED_COLLECTION       *Tracker;
>    UINTN                    *InstalledKey;
>    INT32                    Installed;
> @@ -595,10 +733,15 @@ InstallQemuFwCfgTables (
>    }
>  
>    //
>    // first pass: process the commands
>    //
> +  // "WritePointerSubsetEnd" points one past the last successful
> +  // QEMU_LOADER_WRITE_POINTER command. Now when we're about to start the first
> +  // pass, no such command has been encountered yet.
> +  //
> +  WritePointerSubsetEnd = LoaderStart;
>    for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
>      switch (LoaderEntry->Type) {
>      case QemuLoaderCmdAllocate:
>        Status = ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tracker);
>        break;
> @@ -611,25 +754,33 @@ InstallQemuFwCfgTables (
>      case QemuLoaderCmdAddChecksum:
>        Status = ProcessCmdAddChecksum (&LoaderEntry->Command.AddChecksum,
>                   Tracker);
>        break;
>  
> +    case QemuLoaderCmdWritePointer:
> +        Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
> +                   Tracker);
> +        if (!EFI_ERROR (Status)) {
> +          WritePointerSubsetEnd = LoaderEntry + 1;
> +        }
> +        break;
> +
>      default:
>        DEBUG ((EFI_D_VERBOSE, "%a: unknown loader command: 0x%x\n",
>          __FUNCTION__, LoaderEntry->Type));
>        break;
>      }
>  
>      if (EFI_ERROR (Status)) {
> -      goto FreeTracker;
> +      goto RollbackWritePointersAndFreeTracker;
>      }
>    }
>  
>    InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
>    if (InstalledKey == NULL) {
>      Status = EFI_OUT_OF_RESOURCES;
> -    goto FreeTracker;
> +    goto RollbackWritePointersAndFreeTracker;
>    }
>  
>    //
>    // second pass: identify and install ACPI tables
>    //
> @@ -656,11 +807,25 @@ InstallQemuFwCfgTables (
>      DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>    }
>  
>    FreePool (InstalledKey);
>  
> -FreeTracker:
> +RollbackWritePointersAndFreeTracker:
> +  //
> +  // In case of failure, revoke any allocation addresses that were communicated
> +  // to QEMU previously, before we release all the blobs.
> +  //
> +  if (EFI_ERROR (Status)) {
> +    LoaderEntry = WritePointerSubsetEnd;
> +    while (LoaderEntry > LoaderStart) {
> +      --LoaderEntry;
> +      if (LoaderEntry->Type == QemuLoaderCmdWritePointer) {
> +        UndoCmdWritePointer (&LoaderEntry->Command.WritePointer);
> +      }
> +    }
> +  }
> +
>    //
>    // Tear down the tracker infrastructure. Each fw_cfg blob will be left in
>    // place only if we're exiting with success and the blob hosts data that is
>    // not directly part of some ACPI table.
>    //
> -- 
> 2.9.3
> 
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 02/17/17 20:34, Jordan Justen wrote:
> On 2017-02-16 12:41:36, Laszlo Ersek wrote:
>> The QEMU_LOADER_WRITE_POINTER command instructs the firmware to write the
>> address of a field within a previously allocated/downloaded fw_cfg blob
>> into another (writeable) fw_cfg file at a specific offset.
>>
>> Put differently, QEMU_LOADER_WRITE_POINTER propagates, to QEMU, the
>> address that QEMU_LOADER_ALLOCATE placed the designated fw_cfg blob at, as
>> adjusted for the given field inside the allocated blob.
>>
>> The implementation is similar to that of QEMU_LOADER_ADD_POINTER. Since
>> here we "patch" a pointer object in "fw_cfg file space", not guest memory
>> space, we utilize the QemuFwCfgSkipBytes() and QemuFwCfgWriteBytes() APIs
>> completed in commit range 465663e9f128..7fcb73541299.
>>
>> An interesting aspect is that QEMU_LOADER_WRITE_POINTER creates a
>> host-level reference to a guest memory location. Therefore, if we fail to
>> process the linker/loader script for any reason, we have to clear out
>> those references first, before we release the guest memory allocations in
>> response to the error.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=359
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 171 +++++++++++++++++++-
>>  1 file changed, 168 insertions(+), 3 deletions(-)
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> index 404589cad0b7..de827c2df204 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> @@ -350,10 +350,147 @@ ProcessCmdAddChecksum (
>>      AddChecksum->ResultOffset, AddChecksum->Start, AddChecksum->Length));
>>    return EFI_SUCCESS;
>>  }
>>  
>>  
>> +/**
>> +  Process a QEMU_LOADER_WRITE_POINTER command.
>> +
>> +  @param[in] WritePointer   The QEMU_LOADER_WRITE_POINTER command to process.
>> +
>> +  @param[in] Tracker        The ORDERED_COLLECTION tracking the BLOB user
>> +                            structures created thus far.
>> +
>> +  @retval EFI_PROTOCOL_ERROR  Malformed fw_cfg file name(s) have been found in
>> +                              WritePointer. Or, the WritePointer command
>> +                              references a file unknown to Tracker or the
>> +                              fw_cfg directory. Or, the pointer object to
>> +                              rewrite has invalid location, size, or initial
>> +                              relative value. Or, the pointer value to store
>> +                              does not fit in the given pointer size.
>> +
>> +  @retval EFI_SUCCESS         The pointer object inside the writeable fw_cfg
>> +                              file has been written.
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +ProcessCmdWritePointer (
>> +  IN     CONST QEMU_LOADER_WRITE_POINTER *WritePointer,
>> +  IN     CONST ORDERED_COLLECTION        *Tracker
>> +  )
>> +{
>> +  RETURN_STATUS            Status;
>> +  FIRMWARE_CONFIG_ITEM     PointerItem;
>> +  UINTN                    PointerItemSize;
>> +  ORDERED_COLLECTION_ENTRY *PointeeEntry;
>> +  BLOB                     *PointeeBlob;
>> +  UINT64                   PointerValue;
>> +
>> +  if (WritePointer->PointerFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0' ||
>> +      WritePointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') {
>> +    DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
>> +             &PointerItem, &PointerItemSize);
>> +  PointeeEntry = OrderedCollectionFind (Tracker, WritePointer->PointeeFile);
>> +  if (RETURN_ERROR (Status) || PointeeEntry == NULL) {
>> +    DEBUG ((DEBUG_ERROR,
>> +      "%a: invalid fw_cfg file or blob reference \"%a\" / \"%a\"\n",
>> +      __FUNCTION__, WritePointer->PointerFile, WritePointer->PointeeFile));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  if ((WritePointer->PointerSize != 1 && WritePointer->PointerSize != 2 &&
>> +       WritePointer->PointerSize != 4 && WritePointer->PointerSize != 8) ||
>> +      (PointerItemSize < WritePointer->PointerSize) ||
>> +      (PointerItemSize - WritePointer->PointerSize <
>> +       WritePointer->PointerOffset)) {
>> +    DEBUG ((DEBUG_ERROR, "%a: invalid pointer location or size in \"%a\"\n",
>> +      __FUNCTION__, WritePointer->PointerFile));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  PointeeBlob = OrderedCollectionUserStruct (PointeeEntry);
>> +  PointerValue = WritePointer->PointeeOffset;
>> +  if (PointerValue >= PointeeBlob->Size) {
>> +    DEBUG ((DEBUG_ERROR, "%a: invalid PointeeOffset\n", __FUNCTION__));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  //
>> +  // The memory allocation system ensures that the address of the byte past the
>> +  // last byte of any allocated object is expressible (no wraparound).
>> +  //
>> +  ASSERT ((UINTN)PointeeBlob->Base <= MAX_ADDRESS - PointeeBlob->Size);
>> +
>> +  PointerValue += (UINT64)(UINTN)PointeeBlob->Base;
>> +  if (RShiftU64 (
>> +        RShiftU64 (PointerValue, WritePointer->PointerSize * 8 - 1), 1) != 0) {
> 
> What do you think of this instead?
> 
>   if (WritePointer->PointerSize < 8 &&
>       RShiftU64 (PointerValue, WritePointer->PointerSize * 8) != 0) {

Entirely valid, but then I should update ProcessCmdAddPointer()
similarly :) Also, I'll feel less smart about myself! ;)

If that's OK with you, I'll fix it up later. (I'll collect these warts
into another BZ, as long as they aren't critical.)

BTW, today I had a successful end-to-end (VM migration) test with Ben's
QEMU v8 series, and this one for OVMF; that is, VMGENID worked "in
action" with a Windows Server 2012 R2 guest. In the guest, I used a
small Windows application from a colleague to wait for the VMGENID to
change (and to report / query it).

> 
> Patches 1-4 Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thank you very much!
Laszlo

> 
>> +    DEBUG ((DEBUG_ERROR, "%a: pointer value unrepresentable in \"%a\"\n",
>> +      __FUNCTION__, WritePointer->PointerFile));
>> +    return EFI_PROTOCOL_ERROR;
>> +  }
>> +
>> +  QemuFwCfgSelectItem (PointerItem);
>> +  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
>> +  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
>> +
>> +  //
>> +  // Because QEMU has now learned PointeeBlob->Base, we must mark PointeeBlob
>> +  // as unreleasable, for the case when the whole linker/loader script is
>> +  // handled successfully.
>> +  //
>> +  PointeeBlob->HostsOnlyTableData = FALSE;
>> +
>> +  DEBUG ((DEBUG_VERBOSE, "%a: PointerFile=\"%a\" PointeeFile=\"%a\" "
>> +    "PointerOffset=0x%x PointeeOffset=0x%x PointerSize=%d\n", __FUNCTION__,
>> +    WritePointer->PointerFile, WritePointer->PointeeFile,
>> +    WritePointer->PointerOffset, WritePointer->PointeeOffset,
>> +    WritePointer->PointerSize));
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +
>> +/**
>> +  Undo a QEMU_LOADER_WRITE_POINTER command.
>> +
>> +  This function revokes (zeroes out) a guest memory reference communicated to
>> +  QEMU earlier. The caller is responsible for invoking this function only on
>> +  such QEMU_LOADER_WRITE_POINTER commands that have been successfully processed
>> +  by ProcessCmdWritePointer().
>> +
>> +  @param[in] WritePointer  The QEMU_LOADER_WRITE_POINTER command to undo.
>> +**/
>> +STATIC
>> +VOID
>> +UndoCmdWritePointer (
>> +  IN CONST QEMU_LOADER_WRITE_POINTER *WritePointer
>> +  )
>> +{
>> +  RETURN_STATUS        Status;
>> +  FIRMWARE_CONFIG_ITEM PointerItem;
>> +  UINTN                PointerItemSize;
>> +  UINT64               PointerValue;
>> +
>> +  Status = QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile,
>> +             &PointerItem, &PointerItemSize);
>> +  ASSERT_RETURN_ERROR (Status);
>> +
>> +  PointerValue = 0;
>> +  QemuFwCfgSelectItem (PointerItem);
>> +  QemuFwCfgSkipBytes (WritePointer->PointerOffset);
>> +  QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue);
>> +
>> +  DEBUG ((DEBUG_VERBOSE,
>> +    "%a: PointerFile=\"%a\" PointerOffset=0x%x PointerSize=%d\n", __FUNCTION__,
>> +    WritePointer->PointerFile, WritePointer->PointerOffset,
>> +    WritePointer->PointerSize));
>> +}
>> +
>> +
>>  //
>>  // We'll be saving the keys of installed tables so that we can roll them back
>>  // in case of failure. 128 tables should be enough for anyone (TM).
>>  //
>>  #define INSTALLED_TABLES_MAX 128
>> @@ -559,10 +696,11 @@ InstallQemuFwCfgTables (
>>    EFI_STATUS               Status;
>>    FIRMWARE_CONFIG_ITEM     FwCfgItem;
>>    UINTN                    FwCfgSize;
>>    QEMU_LOADER_ENTRY        *LoaderStart;
>>    CONST QEMU_LOADER_ENTRY  *LoaderEntry, *LoaderEnd;
>> +  CONST QEMU_LOADER_ENTRY  *WritePointerSubsetEnd;
>>    ORIGINAL_ATTRIBUTES      *OriginalPciAttributes;
>>    UINTN                    OriginalPciAttributesCount;
>>    ORDERED_COLLECTION       *Tracker;
>>    UINTN                    *InstalledKey;
>>    INT32                    Installed;
>> @@ -595,10 +733,15 @@ InstallQemuFwCfgTables (
>>    }
>>  
>>    //
>>    // first pass: process the commands
>>    //
>> +  // "WritePointerSubsetEnd" points one past the last successful
>> +  // QEMU_LOADER_WRITE_POINTER command. Now when we're about to start the first
>> +  // pass, no such command has been encountered yet.
>> +  //
>> +  WritePointerSubsetEnd = LoaderStart;
>>    for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
>>      switch (LoaderEntry->Type) {
>>      case QemuLoaderCmdAllocate:
>>        Status = ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tracker);
>>        break;
>> @@ -611,25 +754,33 @@ InstallQemuFwCfgTables (
>>      case QemuLoaderCmdAddChecksum:
>>        Status = ProcessCmdAddChecksum (&LoaderEntry->Command.AddChecksum,
>>                   Tracker);
>>        break;
>>  
>> +    case QemuLoaderCmdWritePointer:
>> +        Status = ProcessCmdWritePointer (&LoaderEntry->Command.WritePointer,
>> +                   Tracker);
>> +        if (!EFI_ERROR (Status)) {
>> +          WritePointerSubsetEnd = LoaderEntry + 1;
>> +        }
>> +        break;
>> +
>>      default:
>>        DEBUG ((EFI_D_VERBOSE, "%a: unknown loader command: 0x%x\n",
>>          __FUNCTION__, LoaderEntry->Type));
>>        break;
>>      }
>>  
>>      if (EFI_ERROR (Status)) {
>> -      goto FreeTracker;
>> +      goto RollbackWritePointersAndFreeTracker;
>>      }
>>    }
>>  
>>    InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
>>    if (InstalledKey == NULL) {
>>      Status = EFI_OUT_OF_RESOURCES;
>> -    goto FreeTracker;
>> +    goto RollbackWritePointersAndFreeTracker;
>>    }
>>  
>>    //
>>    // second pass: identify and install ACPI tables
>>    //
>> @@ -656,11 +807,25 @@ InstallQemuFwCfgTables (
>>      DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>>    }
>>  
>>    FreePool (InstalledKey);
>>  
>> -FreeTracker:
>> +RollbackWritePointersAndFreeTracker:
>> +  //
>> +  // In case of failure, revoke any allocation addresses that were communicated
>> +  // to QEMU previously, before we release all the blobs.
>> +  //
>> +  if (EFI_ERROR (Status)) {
>> +    LoaderEntry = WritePointerSubsetEnd;
>> +    while (LoaderEntry > LoaderStart) {
>> +      --LoaderEntry;
>> +      if (LoaderEntry->Type == QemuLoaderCmdWritePointer) {
>> +        UndoCmdWritePointer (&LoaderEntry->Command.WritePointer);
>> +      }
>> +    }
>> +  }
>> +
>>    //
>>    // Tear down the tracker infrastructure. Each fw_cfg blob will be left in
>>    // place only if we're exiting with success and the blob hosts data that is
>>    // not directly part of some ACPI table.
>>    //
>> -- 
>> 2.9.3
>>
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel