[PATCH 1/1] OvmfPkg/QemuFwCfgDxeLib: SEV: zero FW_CFG_DMA_ACCESS before decrypting it

[Laszlo Ersek <lersek@redhat.com>]

There's a small window between

- AllocFwCfgDmaAccessBuffer() mapping the new FW_CFG_DMA_ACCESS object for
  common buffer operation (i.e., decrypting it), and

- InternalQemuFwCfgDmaBytes() setting the fields of the object.

In this window, earlier garbage in the object is "leaked" to the
hypervisor. So zero the object before we decrypt it.

(This commit message references AMD SEV directly, because QemuFwCfgDxeLib
is not *generally* enabled for IOMMU operation just yet, unlike our goal
for the virtio infrastructure. Instead, QemuFwCfgDxeLib uses
MemEncryptSevLib explicitly to detect SEV, and then relies on IOMMU
protocol behavior that is specific to SEV. At this point, this is by
design.)

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.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
index 8b98208591e6..22077851a40c 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c
@@ -1,29 +1,30 @@
 /** @file
 
   Stateful and implicitly initialized fw_cfg library implementation.
 
   Copyright (C) 2013, Red Hat, Inc.
   Copyright (c) 2011 - 2013, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2017, Advanced Micro Devices. All rights reserved.<BR>
 
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License which accompanies this
   distribution.  The full text of the license may be found at
   http://opensource.org/licenses/bsd-license.php
 
   THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
   WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 **/
 
 #include <Uefi.h>
 
 #include <Protocol/IoMmu.h>
 
 #include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
 #include <Library/IoLib.h>
 #include <Library/DebugLib.h>
 #include <Library/QemuFwCfgLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/MemEncryptSevLib.h>
 
 #include "QemuFwCfgLibInternal.h"
@@ -155,75 +156,81 @@ VOID
 AllocFwCfgDmaAccessBuffer (
   OUT   VOID     **Access,
   OUT   VOID     **MapInfo
   )
 {
   UINTN                 Size;
   UINTN                 NumPages;
   EFI_STATUS            Status;
   VOID                  *HostAddress;
   EFI_PHYSICAL_ADDRESS  DmaAddress;
   VOID                  *Mapping;
 
   Size = sizeof (FW_CFG_DMA_ACCESS);
   NumPages = EFI_SIZE_TO_PAGES (Size);
 
   //
   // As per UEFI spec, in order to map a host address with
   // BusMasterCommomBuffer64, the buffer must be allocated using the IOMMU
   // AllocateBuffer()
   //
   Status = mIoMmuProtocol->AllocateBuffer (
                              mIoMmuProtocol,
                              AllocateAnyPages,
                              EfiBootServicesData,
                              NumPages,
                              &HostAddress,
                              EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE
                              );
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
       "%a:%a failed to allocate FW_CFG_DMA_ACCESS\n", gEfiCallerBaseName,
       __FUNCTION__));
     ASSERT (FALSE);
     CpuDeadLoop ();
   }
 
+  //
+  // Avoid exposing stale data even temporarily: zero the area before mapping
+  // it.
+  //
+  ZeroMem (HostAddress, Size);
+
   //
   // Map the host buffer with BusMasterCommonBuffer64
   //
   Status = mIoMmuProtocol->Map (
                              mIoMmuProtocol,
                              EdkiiIoMmuOperationBusMasterCommonBuffer64,
                              HostAddress,
                              &Size,
                              &DmaAddress,
                              &Mapping
                              );
   if (EFI_ERROR (Status)) {
     mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress);
     DEBUG ((DEBUG_ERROR,
       "%a:%a failed to Map() FW_CFG_DMA_ACCESS\n", gEfiCallerBaseName,
       __FUNCTION__));
     ASSERT (FALSE);
     CpuDeadLoop ();
   }
 
   if (Size < sizeof (FW_CFG_DMA_ACCESS)) {
     mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping);
     mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress);
     DEBUG ((DEBUG_ERROR,
       "%a:%a failed to Map() - requested 0x%Lx got 0x%Lx\n", gEfiCallerBaseName,
       __FUNCTION__, (UINT64)sizeof (FW_CFG_DMA_ACCESS), (UINT64)Size));
     ASSERT (FALSE);
     CpuDeadLoop ();
   }
 
   *Access = HostAddress;
   *MapInfo = Mapping;
 }
 
 /**
   Function is to used for freeing the Access buffer allocated using
   AllocFwCfgDmaAccessBuffer()
 
 **/
-- 
2.14.1.3.gb7cf6e02401b