From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D7F5F8213B for ; Fri, 17 Feb 2017 11:34:50 -0800 (PST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP; 17 Feb 2017 11:34:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,173,1484035200"; d="scan'208";a="66038121" Received: from rwanders-mobl.amr.corp.intel.com (HELO localhost) ([10.252.138.193]) by orsmga005.jf.intel.com with ESMTP; 17 Feb 2017 11:34:50 -0800 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <148736008994.16092.5271721175250835615@jljusten-ivb> From: Jordan Justen In-Reply-To: <20170216204137.30221-5-lersek@redhat.com> References: <20170216204137.30221-1-lersek@redhat.com> <20170216204137.30221-5-lersek@redhat.com> User-Agent: alot/0.5.1 Date: Fri, 17 Feb 2017 11:34:49 -0800 Subject: Re: [PATCH 4/5] OvmfPkg/AcpiPlatformDxe: implement the QEMU_LOADER_WRITE_POINTER command X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Feb 2017 19:34:51 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D359 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek > --- > OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 171 +++++++++++++++++++- > 1 file changed, 168 insertions(+), 3 deletions(-) > = > diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatfo= rmDxe/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 pro= cess. > + > + @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 fo= und 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 ini= tial > + relative value. Or, the pointer value to s= tore > + 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] !=3D '\0' || > + WritePointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] !=3D '\0') { > + DEBUG ((DEBUG_ERROR, "%a: malformed file name\n", __FUNCTION__)); > + return EFI_PROTOCOL_ERROR; > + } > + > + Status =3D QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile, > + &PointerItem, &PointerItemSize); > + PointeeEntry =3D OrderedCollectionFind (Tracker, WritePointer->Pointee= File); > + if (RETURN_ERROR (Status) || PointeeEntry =3D=3D 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 !=3D 1 && WritePointer->PointerSize != =3D 2 && > + WritePointer->PointerSize !=3D 4 && WritePointer->PointerSize != =3D 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 =3D OrderedCollectionUserStruct (PointeeEntry); > + PointerValue =3D WritePointer->PointeeOffset; > + if (PointerValue >=3D 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 p= ast the > + // last byte of any allocated object is expressible (no wraparound). > + // > + ASSERT ((UINTN)PointeeBlob->Base <=3D MAX_ADDRESS - PointeeBlob->Size); > + > + PointerValue +=3D (UINT64)(UINTN)PointeeBlob->Base; > + if (RShiftU64 ( > + RShiftU64 (PointerValue, WritePointer->PointerSize * 8 - 1), 1) = !=3D 0) { What do you think of this instead? if (WritePointer->PointerSize < 8 && RShiftU64 (PointerValue, WritePointer->PointerSize * 8) !=3D 0) { Patches 1-4 Reviewed-by: Jordan Justen > + 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 Pointe= eBlob > + // as unreleasable, for the case when the whole linker/loader script is > + // handled successfully. > + // > + PointeeBlob->HostsOnlyTableData =3D FALSE; > + > + DEBUG ((DEBUG_VERBOSE, "%a: PointerFile=3D\"%a\" PointeeFile=3D\"%a\" " > + "PointerOffset=3D0x%x PointeeOffset=3D0x%x PointerSize=3D%d\n", __FU= NCTION__, > + 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 communicat= ed to > + QEMU earlier. The caller is responsible for invoking this function onl= y on > + such QEMU_LOADER_WRITE_POINTER commands that have been successfully pr= ocessed > + 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 =3D QemuFwCfgFindFile ((CONST CHAR8 *)WritePointer->PointerFile, > + &PointerItem, &PointerItemSize); > + ASSERT_RETURN_ERROR (Status); > + > + PointerValue =3D 0; > + QemuFwCfgSelectItem (PointerItem); > + QemuFwCfgSkipBytes (WritePointer->PointerOffset); > + QemuFwCfgWriteBytes (WritePointer->PointerSize, &PointerValue); > + > + DEBUG ((DEBUG_VERBOSE, > + "%a: PointerFile=3D\"%a\" PointerOffset=3D0x%x PointerSize=3D%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 th= e first > + // pass, no such command has been encountered yet. > + // > + WritePointerSubsetEnd =3D LoaderStart; > for (LoaderEntry =3D LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEnt= ry) { > switch (LoaderEntry->Type) { > case QemuLoaderCmdAllocate: > Status =3D ProcessCmdAllocate (&LoaderEntry->Command.Allocate, Tra= cker); > break; > @@ -611,25 +754,33 @@ InstallQemuFwCfgTables ( > case QemuLoaderCmdAddChecksum: > Status =3D ProcessCmdAddChecksum (&LoaderEntry->Command.AddChecksu= m, > Tracker); > break; > = > + case QemuLoaderCmdWritePointer: > + Status =3D ProcessCmdWritePointer (&LoaderEntry->Command.WritePo= inter, > + Tracker); > + if (!EFI_ERROR (Status)) { > + WritePointerSubsetEnd =3D 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 =3D AllocatePool (INSTALLED_TABLES_MAX * sizeof *Installe= dKey); > if (InstalledKey =3D=3D NULL) { > Status =3D 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__, Insta= lled)); > } > = > FreePool (InstalledKey); > = > -FreeTracker: > +RollbackWritePointersAndFreeTracker: > + // > + // In case of failure, revoke any allocation addresses that were commu= nicated > + // to QEMU previously, before we release all the blobs. > + // > + if (EFI_ERROR (Status)) { > + LoaderEntry =3D WritePointerSubsetEnd; > + while (LoaderEntry > LoaderStart) { > + --LoaderEntry; > + if (LoaderEntry->Type =3D=3D 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 th= at is > // not directly part of some ACPI table. > // > -- = > 2.9.3 > = >=20