From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 8D1BE21CF58AF for ; Thu, 5 Oct 2017 12:42:41 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C1B367EA8A; Thu, 5 Oct 2017 19:46:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C1B367EA8A Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-16.rdu2.redhat.com [10.10.120.16]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1B18E60C99; Thu, 5 Oct 2017 19:46:01 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Chao Zhang References: <20171005184848.94432-1-brijesh.singh@amd.com> <20171005184848.94432-2-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <0383fa57-dd89-eb73-abaa-3248e1e7269f@redhat.com> Date: Thu, 5 Oct 2017 21:46:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171005184848.94432-2-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 05 Oct 2017 19:46:03 +0000 (UTC) Subject: Re: [PATCH 2/2] OvmfPkg/PlatformPei: DENY_EXECUTE_ON_SECURITY_VIOLATION when SEV is active 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: Thu, 05 Oct 2017 19:42:41 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Brijesh, I've got three comments: On 10/05/17 20:48, Brijesh Singh wrote: > The following commit: > > 1fea9ddb4e3f OvmfPkg: execute option ROM images regardless of Secure Boot > > sets the OptionRomImageVerificationPolicy to ALWAYS_EXECUTE the expansion > ROMs attached to the emulated PCI devices. A expansion ROM constitute > another channel through which a cloud provider (i.e hypervisor) can > inject a code in guest boot flow to compromise it. > > When SEV is enabled, the bios code has been verified by the guest owner > via the SEV guest launch sequence before its executed. When secure boot, > is enabled, lets make sure that we do not allow guest bios to execute a > code which is not signed by the guest owner. > > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=728 > Cc: Chao Zhang > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Tom Lendacky > Signed-off-by: Brijesh Singh > --- > OvmfPkg/OvmfPkgIa32X64.dsc | 9 +++++---- > OvmfPkg/OvmfPkgX64.dsc | 9 +++++---- > OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++ > OvmfPkg/PlatformPei/AmdSev.c | 7 +++++++ > 4 files changed, 19 insertions(+), 8 deletions(-) > > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 7f9220ccb90a..4bcbddb95768 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -489,10 +489,6 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000 > !endif > > -!if $(SECURE_BOOT_ENABLE) == TRUE > - gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 > -!endif > - > # IRQs 5, 9, 10, 11 are level-triggered > gPcAtChipsetPkgTokenSpaceGuid.Pcd8259LegacyModeEdgeLevel|0x0E20 > > @@ -552,6 +548,11 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 > !endif > > +!if $(SECURE_BOOT_ENABLE) == TRUE > + gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 > +!endif > + > + > ################################################################################ > # > # Components Section - list of all EDK II Modules needed by this Platform. (1) It's hard to see what section of the DSC file the lines are moved from, and to what section. Git can be configured so that it display the section names in the diff hunk header (following the "@@" marks). Please refer to the following: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05 https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09 In particular, see the "diff.ini.xfuncname" config option, and the matching "diff=ini" attributes. (This remark does not affect the patch, only how it is formatted. Showing the DSC section names in the diff hunk headers helps quite a bit with review.) > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 36c60fc19c40..e52a3bd4db9b 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -488,10 +488,6 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackSize|0x4000 > !endif > > -!if $(SECURE_BOOT_ENABLE) == TRUE > - gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 > -!endif > - > # IRQs 5, 9, 10, 11 are level-triggered > gPcAtChipsetPkgTokenSpaceGuid.Pcd8259LegacyModeEdgeLevel|0x0E20 > > @@ -551,6 +547,11 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 > !endif > > +!if $(SECURE_BOOT_ENABLE) == TRUE > + gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 > +!endif > + > + > ################################################################################ > # > # Components Section - list of all EDK II Modules needed by this Platform. (2) Please apply the same code movement to "OvmfPkg/OvmfPkgIa32.dsc". As written, the PcdSet32S() macro invocation in PlatformPei below will not even compile for OvmfPkgIa32. Namely, the PCD will remain fixed-at-build, and because of that, the replacement text of the PcdSet32S() macro, i.e. _PCD_SET_MODE_32_S_##TokenName ((Value)) from MdePkg/Include/Library/PcdLib.h will not have any *further* replacement text, from "AutoGen.h". > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index 16a8db7b0bd2..de7434d93dc0 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -41,6 +41,7 @@ > IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > + SecurityPkg/SecurityPkg.dec > UefiCpuPkg/UefiCpuPkg.dec > OvmfPkg/OvmfPkg.dec > > @@ -96,6 +97,7 @@ > gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask > + gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy > gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c > index 26f7c3fdbb13..1539e5b5cdce 100644 > --- a/OvmfPkg/PlatformPei/AmdSev.c > +++ b/OvmfPkg/PlatformPei/AmdSev.c > @@ -59,4 +59,11 @@ AmdSevInitialize ( > ASSERT_RETURN_ERROR (PcdStatus); > > DEBUG ((DEBUG_INFO, "SEV is enabled (mask 0x%lx)\n", EncryptionMask)); > + > + // > + // Set Pcd to Deny the execution of option ROM when security > + // violation. > + // > + PcdStatus = PcdSet32S (PcdOptionRomImageVerificationPolicy, 0x4); > + ASSERT_RETURN_ERROR (PcdStatus); > } > (3) I find it sub-optimal that the DENY_EXECUTE_ON_SECURITY_VIOLATION macro, from SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h is internal to that library instance, and isn't exposed in some library class header. Anyway, I guess we can't do much about it here, and at least SecurityPkg.dec spells out the meanings of the values. So this hunk looks fine to me. Thanks! Laszlo