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

[Jordan Justen <jordan.l.justen@intel.com>]

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

On 2017-08-28 08:39:28, Laszlo Ersek wrote:
> 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
>