Reviewed-by: Jordan Justen <[email protected]>
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 <[email protected]> > Cc: Jordan Justen <[email protected]> > Cc: Tom Lendacky <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek <[email protected]> > --- > 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 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
