From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BA36221E11D22 for ; Mon, 28 Aug 2017 22:08:36 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Aug 2017 22:11:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,443,1498546800"; d="scan'208";a="895041631" Received: from ylim19-mobl1.gar.corp.intel.com (HELO localhost) ([10.255.138.150]) by FMSMGA003.fm.intel.com with ESMTP; 28 Aug 2017 22:11:05 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <150398346137.25288.10980860099496106545@jljusten-skl> From: Jordan Justen In-Reply-To: <20170828123928.14627-2-lersek@redhat.com> Cc: Brijesh Singh , Tom Lendacky References: <20170828123928.14627-1-lersek@redhat.com> <20170828123928.14627-2-lersek@redhat.com> User-Agent: alot/0.6 Date: Tue, 29 Aug 2017 01:11:03 -0400 Subject: Re: [PATCH 1/1] OvmfPkg/QemuFwCfgDxeLib: SEV: zero FW_CFG_DMA_ACCESS before decrypting it X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Aug 2017 05:08:36 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jordan Justen 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 > Cc: Jordan Justen > Cc: Tom Lendacky > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 7 +++++++ > 1 file changed, 7 insertions(+) > = > diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Librar= y/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.
> Copyright (c) 2017, Advanced Micro Devices. All rights reserved.
> = > This program and the accompanying materials are licensed and made avai= lable > under the terms and conditions of the BSD License which accompanies th= is > 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 > = > #include > = > #include > +#include > #include > #include > #include > #include > #include > = > #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 =3D sizeof (FW_CFG_DMA_ACCESS); > NumPages =3D 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 IOM= MU > // AllocateBuffer() > // > Status =3D 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 ma= pping > + // it. > + // > + ZeroMem (HostAddress, Size); > + > // > // Map the host buffer with BusMasterCommonBuffer64 > // > Status =3D 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", gEfiCallerB= aseName, > __FUNCTION__, (UINT64)sizeof (FW_CFG_DMA_ACCESS), (UINT64)Size)); > ASSERT (FALSE); > CpuDeadLoop (); > } > = > *Access =3D HostAddress; > *MapInfo =3D Mapping; > } > = > /** > Function is to used for freeing the Access buffer allocated using > AllocFwCfgDmaAccessBuffer() > = > **/ > -- = > 2.14.1.3.gb7cf6e02401b >=20