public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Jordan Justen <jordan.l.justen@intel.com>,
	Tom Lendacky <thomas.lendacky@amd.com>
Subject: [PATCH 11/12] OvmfPkg/IoMmuDxe: abort harder on memory encryption mask failures
Date: Wed,  2 Aug 2017 23:24:52 +0200	[thread overview]
Message-ID: <20170802212453.19221-12-lersek@redhat.com> (raw)
In-Reply-To: <20170802212453.19221-1-lersek@redhat.com>

Upon a MemEncryptSevClearPageEncMask() failure in Map(), it wouldn't be
difficult to release the bounce buffer that was implicitly allocated for
BusMasterRead[64] and BusMasterWrite[64] operations. However, undoing any
partial memory encryption mask changes -- partial page splitting and PTE
modifications -- is practically impossible. (For example, restoring the
encryption mask on the entire range has no reason to fare any better than
the MemEncryptSevClearPageEncMask() call itself.)

For this reason, keep ASSERT_EFI_ERROR(), but hang in RELEASE builds too,
if MemEncryptSevClearPageEncMask() or MemEncryptSevSetPageEncMask() fails.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 5049d19e9cb7..ee94cd4efbe2 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -89,175 +89,178 @@ EFIAPI
 IoMmuMap (
   IN     EDKII_IOMMU_PROTOCOL                       *This,
   IN     EDKII_IOMMU_OPERATION                      Operation,
   IN     VOID                                       *HostAddress,
   IN OUT UINTN                                      *NumberOfBytes,
   OUT    EFI_PHYSICAL_ADDRESS                       *DeviceAddress,
   OUT    VOID                                       **Mapping
   )
 {
   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) {
     return EFI_INVALID_PARAMETER;
   }
 
   //
   // Allocate a MAP_INFO structure to remember the mapping when Unmap() is
   // called later.
   //
   MapInfo = AllocatePool (sizeof (MAP_INFO));
   if (MapInfo == NULL) {
     Status = EFI_OUT_OF_RESOURCES;
     goto Failed;
   }
 
   //
   // Initialize the MAP_INFO structure, except the PlainTextAddress field
   //
   MapInfo->Operation         = Operation;
   MapInfo->NumberOfBytes     = *NumberOfBytes;
   MapInfo->NumberOfPages     = EFI_SIZE_TO_PAGES (MapInfo->NumberOfBytes);
   MapInfo->CryptedAddress    = (UINTN)HostAddress;
 
   //
   // In the switch statement below, we point "MapInfo->PlainTextAddress" to the
   // 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
   // is necessary regardless of whether the original (crypted) buffer crosses
   // the 4GB limit or not -- we have to allocate a separate plaintext buffer.
   // The only variable is whether the plaintext buffer should be under 4GB.
   //
   case EdkiiIoMmuOperationBusMasterRead:
   case EdkiiIoMmuOperationBusMasterWrite:
     MapInfo->PlainTextAddress = BASE_4GB - 1;
     AllocateType = AllocateMaxAddress;
     //
     // fall through
     //
   case EdkiiIoMmuOperationBusMasterRead64:
   case EdkiiIoMmuOperationBusMasterWrite64:
     //
     // Allocate the implicit plaintext bounce buffer.
     //
     Status = gBS->AllocatePages (
                     AllocateType,
                     EfiBootServicesData,
                     MapInfo->NumberOfPages,
                     &MapInfo->PlainTextAddress
                     );
     if (EFI_ERROR (Status)) {
       goto FreeMapInfo;
     }
     break;
 
   //
   // 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) ||
         (EFI_PAGES_TO_SIZE (MapInfo->NumberOfPages) >
          BASE_4GB - MapInfo->CryptedAddress)) {
       //
       // CommonBuffer operations cannot be remapped. If the common buffer is
       // above 4GB, then it is not possible to generate a mapping, so return an
       // error.
       //
       Status = EFI_UNSUPPORTED;
       goto FreeMapInfo;
     }
     //
     // fall through
     //
   case EdkiiIoMmuOperationBusMasterCommonBuffer64:
     //
     // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
     //
     MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
     //
     // Stash the crypted data.
     //
     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:
     //
     // Operation is invalid
     //
     Status = EFI_INVALID_PARAMETER;
     goto FreeMapInfo;
   }
 
   //
   // Clear the memory encryption mask on the plaintext buffer.
   //
   Status = MemEncryptSevClearPageEncMask (
              0,
              MapInfo->PlainTextAddress,
              MapInfo->NumberOfPages,
              TRUE
              );
-  ASSERT_EFI_ERROR(Status);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    CpuDeadLoop ();
+  }
 
   //
   // If this is a read operation from the Bus Master's point of view,
   // 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 == EdkiiIoMmuOperationBusMasterCommonBuffer ||
       Operation == EdkiiIoMmuOperationBusMasterCommonBuffer64) {
     CopyMem (
       (VOID *) (UINTN) MapInfo->PlainTextAddress,
       DecryptionSource,
       MapInfo->NumberOfBytes
       );
   }
 
   //
   // Populate output parameters.
   //
   *DeviceAddress = MapInfo->PlainTextAddress;
   *Mapping       = MapInfo;
 
   DEBUG ((
     DEBUG_VERBOSE,
     "%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
     __FUNCTION__,
     MapInfo->PlainTextAddress,
     MapInfo->CryptedAddress,
     (UINT64)MapInfo->NumberOfPages,
     (UINT64)MapInfo->NumberOfBytes
     ));
 
   return EFI_SUCCESS;
@@ -287,129 +290,132 @@ EFIAPI
 IoMmuUnmap (
   IN  EDKII_IOMMU_PROTOCOL                     *This,
   IN  VOID                                     *Mapping
   )
 {
   MAP_INFO                 *MapInfo;
   EFI_STATUS               Status;
   COMMON_BUFFER_HEADER     *CommonBufferHeader;
   VOID                     *EncryptionTarget;
 
   if (Mapping == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
   MapInfo = (MAP_INFO *)Mapping;
 
   //
   // 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 (
       EncryptionTarget,
       (VOID *) (UINTN) MapInfo->PlainTextAddress,
       MapInfo->NumberOfBytes
       );
     break;
 
   default:
     //
     // nothing to encrypt after BusMasterRead[64] operations
     //
     break;
   }
 
   DEBUG ((
     DEBUG_VERBOSE,
     "%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
     __FUNCTION__,
     MapInfo->PlainTextAddress,
     MapInfo->CryptedAddress,
     (UINT64)MapInfo->NumberOfPages,
     (UINT64)MapInfo->NumberOfBytes
     ));
 
   //
   // Restore the memory encryption mask on the area we used to hold the
   // plaintext.
   //
   Status = MemEncryptSevSetPageEncMask (
              0,
              MapInfo->PlainTextAddress,
              MapInfo->NumberOfPages,
              TRUE
              );
-  ASSERT_EFI_ERROR(Status);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    CpuDeadLoop ();
+  }
 
   //
   // 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.
   //
   FreePool (Mapping);
   return EFI_SUCCESS;
 }
 
 /**
   Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
   OperationBusMasterCommonBuffer64 mapping.
 
   @param  This                  The protocol instance pointer.
   @param  Type                  This parameter is not used and must be ignored.
   @param  MemoryType            The type of memory to allocate,
                                 EfiBootServicesData or EfiRuntimeServicesData.
   @param  Pages                 The number of pages to allocate.
   @param  HostAddress           A pointer to store the base system memory
                                 address of the allocated range.
   @param  Attributes            The requested bit mask of attributes for the
                                 allocated range.
 
   @retval EFI_SUCCESS           The requested memory pages were allocated.
   @retval EFI_UNSUPPORTED       Attributes is unsupported. The only legal
                                 attribute bits are MEMORY_WRITE_COMBINE and
                                 MEMORY_CACHED.
   @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
   @retval EFI_OUT_OF_RESOURCES  The memory pages could not be allocated.
 
 **/
-- 
2.13.1.3.g8be5a757fa67




  parent reply	other threads:[~2017-08-02 21:23 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-02 21:24 [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
2017-08-02 21:24 ` [PATCH 01/12] OvmfPkg/IoMmuDxe: rewrap source code to 79 characters Laszlo Ersek
2017-08-02 21:24 ` [PATCH 02/12] OvmfPkg/IoMmuDxe: rename DeviceAddress to PlainTextAddress in MAP_INFO Laszlo Ersek
2017-08-02 21:24 ` [PATCH 03/12] OvmfPkg/IoMmuDxe: rename HostAddress to CryptedAddress " Laszlo Ersek
2017-08-02 21:24 ` [PATCH 04/12] OvmfPkg/IoMmuDxe: convert UINTN arguments to UINT64 for the %Lx fmt spec Laszlo Ersek
2017-08-02 21:24 ` [PATCH 05/12] OvmfPkg/IoMmuDxe: don't initialize local variables Laszlo Ersek
2017-08-02 21:24 ` [PATCH 06/12] OvmfPkg/IoMmuDxe: propagate errors from AmdSevInstallIoMmuProtocol() Laszlo Ersek
2017-08-02 21:24 ` [PATCH 07/12] OvmfPkg/IoMmuDxe: clean up used library classes Laszlo Ersek
2017-08-02 21:24 ` [PATCH 08/12] OvmfPkg/IoMmuDxe: zero out pages before releasing them Laszlo Ersek
2017-08-02 21:24 ` [PATCH 09/12] OvmfPkg/IoMmuDxe: rework setup of "MapInfo->PlainTextAddress" in Map() Laszlo Ersek
2017-08-02 21:24 ` [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap Laszlo Ersek
2017-08-02 23:01   ` Brijesh Singh
2017-08-03  0:13     ` Laszlo Ersek
2017-08-03  1:09       ` Brijesh Singh
2017-08-03 14:35         ` Brijesh Singh
2017-08-03 14:40           ` Laszlo Ersek
2017-08-02 21:24 ` Laszlo Ersek [this message]
2017-08-02 21:24 ` [PATCH 12/12] OvmfPkg/IoMmuDxe: Unmap(): recycle MAP_INFO after BusMasterCommonBuffer[64] Laszlo Ersek
2017-08-02 21:31 ` [PATCH 00/12] OvmfPkg/IoMmuDxe: cleanups and fixes Laszlo Ersek
2017-08-03 14:10 ` Brijesh Singh
2017-08-03 14:15   ` Laszlo Ersek
2017-08-05  1:25   ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170802212453.19221-12-lersek@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox