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

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

[Laszlo Ersek]

This crossed my mind previously in
<http://mid.mail-archive.com/1d6bade7-578e-c0e7-5aa6-2ca6af1185e6@redhat.com>,
point (4), and now I thought I should search for
EdkiiIoMmuOperationBusMasterCommonBuffer too, not just
VirtioOperationBusMasterCommonBuffer.

Repo:   https://github.com/lersek/edk2.git
Branch: fwcfg_sev_commonbuffer

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

Thanks
Laszlo

Laszlo Ersek (1):
  OvmfPkg/QemuFwCfgDxeLib: SEV: zero FW_CFG_DMA_ACCESS before decrypting
    it

 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.14.1.3.gb7cf6e02401b

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

[Laszlo Ersek]

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

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

[Jordan Justen]

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
> 

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

[Brijesh Singh]

On 08/28/2017 07:39 AM, 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>


Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

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

[Laszlo Ersek]

On 08/29/17 16:02, Brijesh Singh wrote:
> 
> 
> On 08/28/2017 07:39 AM, 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>
> 
> 
> Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

Thank you guys for the reviews, pushed as commit d431d8339e8b.

Laszlo