From 58e681406f8fd59fad3f20ec3737549d4237e847 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Wed, 2 Aug 2017 18:26:59 +0200 Subject: [PATCH] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap At the moment, we have the following distribution of actions between the IOMMU protocol member functions: - AllocateBuffer() allocates pages and clears the memory encryption mask. - FreeBuffer() re-sets the memory encryption mask, and deallocates pages. - Map() does nothing at all when BusMasterCommonBuffer[64] is requested (and AllocateBuffer() was called previously). Otherwise, Map() allocates pages, and clears the memory encryption mask. - Unmap() does nothing when cleaning up a BusMasterCommonBuffer[64] operation. Otherwise, Unmap() clears the encryption mask, and frees the pages. This is wrong: the AllocateBuffer() protocol member is not expected to produce a buffer that is immediately usable, and client code is required to call Map() unconditionally, even if BusMasterCommonBuffer[64] is the desired operation. Implement the right distribution of actions as follows: - AllocateBuffer() allocates pages and does not touch the encryption mask. - FreeBuffer() deallocates pages and does not touch the encryption mask. - Map() does not allocate pages when BusMasterCommonBuffer[64] is requested, and it allocates pages (bounce buffer) otherwise. Regardless of the BusMaster operation, Map() (and Map() only) clears the memory encryption mask. - Unmap() restores the encryption mask unconditionally. If the operation was BusMasterCommonBuffer[64], then Unmap() does not release the pages. Otherwise, the pages (bounce buffer) are released. This approach also ensures that Unmap() can be called from ExitBootServices() event handlers, for cleaning up BusMasterCommonBuffer[64] operations. (More specifically, for restoring the SEV encryption mask on any in-flight buffers, after resetting any referring devices.) ExitBootServices() event handlers must not change the UEFI memory map, thus any memory allocation or freeing in Unmap() would disqualify Unmap() from being called in such a context. Map()-ing and Unmap()-ing memory for a BusMasterCommonBuffer[64] operation effectively means in-place decryption and encryption in a SEV context. As an additional hurdle, section "7.10.8 Encrypt-in-Place" of AMD publication Nr.24593 implies that we need a separate temporary buffer for decryption and encryption that will eventually land in-place. Allocating said temporary buffer in the straightforward way would violate the above allocation/freeing restrictions on Map()/Unmap(), therefore pre-allocate this "stash buffer" too in AllocateBuffer(), and free it in FreeBuffer(). To completely rid Unmap() of dynamic memory impact, for BusMasterCommonBuffer[64] operations, we're going to rework the lifecycle of the MAP_INFO structures in a later patch. (The MemEncryptSevSetPageEncMask() call in Unmap() could theoretically allocate memory internally for page splitting, however this won't happen in practice: in Unmap() we only restore the memory encryption mask, and don't genuinely set it. Any page splitting will have occurred in Map()'s MemEncryptSevClearPageEncMask() call first.) Cc: Ard Biesheuvel Cc: Brijesh Singh Cc: Jordan Justen Cc: Tom Lendacky Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek Tested-by: Brijesh Singh Reviewed-by: Brijesh Singh --- OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 247 +++++++++++++++++++++++++-------- 1 file changed, 190 insertions(+), 57 deletions(-) diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c index 0a85ee6559..1dafe0df11 100644 --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c @@ -28,7 +28,31 @@ typedef struct { EFI_PHYSICAL_ADDRESS PlainTextAddress; } MAP_INFO; -#define NO_MAPPING (VOID *) (UINTN) -1 +#define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R') + +// +// The following structure enables Map() and Unmap() to perform in-place +// decryption and encryption, respectively, for BusMasterCommonBuffer[64] +// operations, without dynamic memory allocation or release. +// +// Both COMMON_BUFFER_HEADER and COMMON_BUFFER_HEADER.StashBuffer are allocated +// by AllocateBuffer() and released by FreeBuffer(). +// +#pragma pack (1) +typedef struct { + UINT64 Signature; + + // + // Always allocated from EfiBootServicesData type memory, and always + // encrypted. + // + VOID *StashBuffer; + + // + // Followed by the actual common buffer, starting at the next page. + // +} COMMON_BUFFER_HEADER; +#pragma pack () /** Provides the controller-specific addresses required to access system memory @@ -74,6 +98,8 @@ IoMmuMap ( EFI_STATUS Status; MAP_INFO *MapInfo; EFI_ALLOCATE_TYPE AllocateType; + COMMON_BUFFER_HEADER *CommonBufferHeader; + VOID *DecryptionSource; if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL || Mapping == NULL) { @@ -100,10 +126,11 @@ IoMmuMap ( // // In the switch statement below, we point "MapInfo->PlainTextAddress" to the - // plaintext buffer, according to Operation. + // plaintext buffer, according to Operation. We also set "DecryptionSource". // MapInfo->PlainTextAddress = MAX_ADDRESS; AllocateType = AllocateAnyPages; + DecryptionSource = (VOID *)(UINTN)MapInfo->CryptedAddress; switch (Operation) { // // For BusMasterRead[64] and BusMasterWrite[64] operations, a bounce buffer @@ -135,9 +162,10 @@ IoMmuMap ( break; // - // For BusMasterCommonBuffer[64] operations, a plaintext buffer has been - // allocated already, with AllocateBuffer(). We only check whether the - // address is low enough for the requested operation. + // For BusMasterCommonBuffer[64] operations, a to-be-plaintext buffer and a + // stash buffer (for in-place decryption) have been allocated already, with + // AllocateBuffer(). We only check whether the address of the to-be-plaintext + // buffer is low enough for the requested operation. // case EdkiiIoMmuOperationBusMasterCommonBuffer: if ((MapInfo->CryptedAddress > BASE_4GB) || @@ -156,18 +184,27 @@ IoMmuMap ( // case EdkiiIoMmuOperationBusMasterCommonBuffer64: // - // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer(), - // and it is already decrypted. + // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer(). // MapInfo->PlainTextAddress = MapInfo->CryptedAddress; - // - // Therefore no mapping is necessary. + // Stash the crypted data. // - *DeviceAddress = MapInfo->PlainTextAddress; - *Mapping = NO_MAPPING; - FreePool (MapInfo); - return EFI_SUCCESS; + CommonBufferHeader = (COMMON_BUFFER_HEADER *)( + (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE + ); + ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG); + CopyMem ( + CommonBufferHeader->StashBuffer, + (VOID *)(UINTN)MapInfo->CryptedAddress, + MapInfo->NumberOfBytes + ); + // + // Point "DecryptionSource" to the stash buffer so that we decrypt + // it to the original location, after the switch statement. + // + DecryptionSource = CommonBufferHeader->StashBuffer; + break; default: // @@ -193,11 +230,16 @@ IoMmuMap ( // then copy the contents of the real buffer into the mapped buffer // so the Bus Master can read the contents of the real buffer. // + // For BusMasterCommonBuffer[64] operations, the CopyMem() below will decrypt + // the original data (from the stash buffer) back to the original location. + // if (Operation == EdkiiIoMmuOperationBusMasterRead || - Operation == EdkiiIoMmuOperationBusMasterRead64) { + Operation == EdkiiIoMmuOperationBusMasterRead64 || + Operation == EdkiiIoMmuOperationBusMasterCommonBuffer || + Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) { CopyMem ( (VOID *) (UINTN) MapInfo->PlainTextAddress, - (VOID *) (UINTN) MapInfo->CryptedAddress, + DecryptionSource, MapInfo->NumberOfBytes ); } @@ -249,34 +291,58 @@ IoMmuUnmap ( { MAP_INFO *MapInfo; EFI_STATUS Status; + COMMON_BUFFER_HEADER *CommonBufferHeader; + VOID *EncryptionTarget; if (Mapping == NULL) { return EFI_INVALID_PARAMETER; } - // - // See if the Map() operation associated with this Unmap() required a mapping - // buffer. If a mapping buffer was not required, then this function simply - // buffer. If a mapping buffer was not required, then this function simply - // - if (Mapping == NO_MAPPING) { - return EFI_SUCCESS; - } - MapInfo = (MAP_INFO *)Mapping; // - // If this is a write operation from the Bus Master's point of view, - // then copy the contents of the mapped buffer into the real buffer - // so the processor can read the contents of the real buffer. + // set CommonBufferHeader to suppress incorrect compiler/analyzer warnings // - if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite || - MapInfo->Operation == EdkiiIoMmuOperationBusMasterWrite64) { + CommonBufferHeader = NULL; + + // + // For BusMasterWrite[64] operations and BusMasterCommonBuffer[64] operations + // we have to encrypt the results, ultimately to the original place (i.e., + // "MapInfo->CryptedAddress"). + // + // For BusMasterCommonBuffer[64] operations however, this encryption has to + // land in-place, so divert the encryption to the stash buffer first. + // + EncryptionTarget = (VOID *)(UINTN)MapInfo->CryptedAddress; + + switch (MapInfo->Operation) { + case EdkiiIoMmuOperationBusMasterCommonBuffer: + case EdkiiIoMmuOperationBusMasterCommonBuffer64: + ASSERT (MapInfo->PlainTextAddress == MapInfo->CryptedAddress); + + CommonBufferHeader = (COMMON_BUFFER_HEADER *)( + (UINTN)MapInfo->PlainTextAddress - EFI_PAGE_SIZE + ); + ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG); + EncryptionTarget = CommonBufferHeader->StashBuffer; + // + // fall through + // + + case EdkiiIoMmuOperationBusMasterWrite: + case EdkiiIoMmuOperationBusMasterWrite64: CopyMem ( - (VOID *) (UINTN) MapInfo->CryptedAddress, + EncryptionTarget, (VOID *) (UINTN) MapInfo->PlainTextAddress, MapInfo->NumberOfBytes ); + break; + + default: + // + // nothing to encrypt after BusMasterRead[64] operations + // + break; } DEBUG (( @@ -288,8 +354,10 @@ IoMmuUnmap ( (UINT64)MapInfo->NumberOfPages, (UINT64)MapInfo->NumberOfBytes )); + // - // Restore the memory encryption mask + // Restore the memory encryption mask on the area we used to hold the + // plaintext. // Status = MemEncryptSevSetPageEncMask ( 0, @@ -298,15 +366,32 @@ IoMmuUnmap ( TRUE ); ASSERT_EFI_ERROR(Status); - ZeroMem ( - (VOID*)(UINTN)MapInfo->PlainTextAddress, - EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages) - ); // - // Free the mapped buffer and the MAP_INFO structure. + // For BusMasterCommonBuffer[64] operations, copy the stashed data to the + // original (now encrypted) location. + // + // For all other operations, fill the late bounce buffer (which existed as + // plaintext at some point) with zeros, and then release it. + // + if (MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer || + MapInfo->Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) { + CopyMem ( + (VOID *)(UINTN)MapInfo->CryptedAddress, + CommonBufferHeader->StashBuffer, + MapInfo->NumberOfBytes + ); + } else { + ZeroMem ( + (VOID *)(UINTN)MapInfo->PlainTextAddress, + EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages) + ); + gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages); + } + + // + // Free the MAP_INFO structure. // - gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages); FreePool (Mapping); return EFI_SUCCESS; } @@ -346,6 +431,9 @@ IoMmuAllocateBuffer ( { EFI_STATUS Status; EFI_PHYSICAL_ADDRESS PhysicalAddress; + VOID *StashBuffer; + UINTN CommonBufferPages; + COMMON_BUFFER_HEADER *CommonBufferHeader; // // Validate Attributes @@ -370,6 +458,32 @@ IoMmuAllocateBuffer ( return EFI_INVALID_PARAMETER; } + // + // We'll need a header page for the COMMON_BUFFER_HEADER structure. + // + if (Pages > MAX_UINTN - 1) { + return EFI_OUT_OF_RESOURCES; + } + CommonBufferPages = Pages + 1; + + // + // Allocate the stash in EfiBootServicesData type memory. + // + // Map() will temporarily save encrypted data in the stash for + // BusMasterCommonBuffer[64] operations, so the data can be decrypted to the + // original location. + // + // Unmap() will temporarily save plaintext data in the stash for + // BusMasterCommonBuffer[64] operations, so the data can be encrypted to the + // original location. + // + // StashBuffer always resides in encrypted memory. + // + StashBuffer = AllocatePages (Pages); + if (StashBuffer == NULL) { + return EFI_OUT_OF_RESOURCES; + } + PhysicalAddress = (UINTN)-1; if ((Attributes & EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) { // @@ -380,19 +494,21 @@ IoMmuAllocateBuffer ( Status = gBS->AllocatePages ( AllocateMaxAddress, MemoryType, - Pages, + CommonBufferPages, &PhysicalAddress ); - if (!EFI_ERROR (Status)) { - *HostAddress = (VOID *) (UINTN) PhysicalAddress; - - // - // Clear memory encryption mask - // - Status = MemEncryptSevClearPageEncMask (0, PhysicalAddress, Pages, TRUE); - ASSERT_EFI_ERROR(Status); + if (EFI_ERROR (Status)) { + goto FreeStashBuffer; } + CommonBufferHeader = (VOID *)(UINTN)PhysicalAddress; + PhysicalAddress += EFI_PAGE_SIZE; + + CommonBufferHeader->Signature = COMMON_BUFFER_SIG; + CommonBufferHeader->StashBuffer = StashBuffer; + + *HostAddress = (VOID *)(UINTN)PhysicalAddress; + DEBUG (( DEBUG_VERBOSE, "%a Address 0x%Lx Pages 0x%Lx\n", @@ -400,6 +516,10 @@ IoMmuAllocateBuffer ( PhysicalAddress, (UINT64)Pages )); + return EFI_SUCCESS; + +FreeStashBuffer: + FreePages (StashBuffer, Pages); return Status; } @@ -424,19 +544,27 @@ IoMmuFreeBuffer ( IN VOID *HostAddress ) { - EFI_STATUS Status; + UINTN CommonBufferPages; + COMMON_BUFFER_HEADER *CommonBufferHeader; + + CommonBufferPages = Pages + 1; + CommonBufferHeader = (COMMON_BUFFER_HEADER *)( + (UINTN)HostAddress - EFI_PAGE_SIZE + ); // - // Set memory encryption mask + // Check the signature. // - Status = MemEncryptSevSetPageEncMask ( - 0, - (EFI_PHYSICAL_ADDRESS)(UINTN)HostAddress, - Pages, - TRUE - ); - ASSERT_EFI_ERROR(Status); - ZeroMem (HostAddress, EFI_PAGES_TO_SIZE (Pages)); + ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG); + if (CommonBufferHeader->Signature != COMMON_BUFFER_SIG) { + return EFI_INVALID_PARAMETER; + } + + // + // Free the stash buffer. This buffer was always encrypted, so no need to + // zero it. + // + FreePages (CommonBufferHeader->StashBuffer, Pages); DEBUG (( DEBUG_VERBOSE, @@ -445,7 +573,12 @@ IoMmuFreeBuffer ( (UINT64)(UINTN)HostAddress, (UINT64)Pages )); - return gBS->FreePages ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress, Pages); + + // + // Release the common buffer itself. Unmap() has re-encrypted it in-place, so + // no need to zero it. + // + return gBS->FreePages ((UINTN)CommonBufferHeader, CommonBufferPages); }