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 DC73320347150 for ; Thu, 19 Oct 2017 01:50:31 -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 9473A13A49; Thu, 19 Oct 2017 08:54:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9473A13A49 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-85.rdu2.redhat.com [10.10.120.85]) by smtp.corp.redhat.com (Postfix) with ESMTP id 17F4EA749D; Thu, 19 Oct 2017 08:54:07 +0000 (UTC) To: Jordan Justen , edk2-devel-01 Cc: Brijesh Singh , Ard Biesheuvel References: <20171017222321.15381-1-lersek@redhat.com> <150837926088.13913.2401594087131779316@jljusten-skl> From: Laszlo Ersek Message-ID: <80827e7a-a2ce-386b-fe41-e2c2e738c4c9@redhat.com> Date: Thu, 19 Oct 2017 10:54:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <150837926088.13913.2401594087131779316@jljusten-skl> 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.29]); Thu, 19 Oct 2017 08:54:09 +0000 (UTC) Subject: Re: [PATCH] OvmfPkg: fix dynamic default for oprom verification policy PCD without SB 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, 19 Oct 2017 08:50:32 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/19/17 04:14, Jordan Justen wrote: > On 2017-10-17 15:23:21, Laszlo Ersek wrote: >> I missed the following, both while reviewing and while testing commit >> 6041ac65ae87 ("OvmfPkg/PlatformPei: DENY_EXECUTE_ON_SECURITY_VIOLATION >> when SEV is active", 2017-10-05): >> >> If "-D SECURE_BOOT_ENABLE" is not passed on the "build" command line, then >> OVMF has no dynamic default at all for >> "PcdOptionRomImageVerificationPolicy". This means that the PcdSet32S() >> call added in the subject commit doesn't even compile: >> >>> OvmfPkg/PlatformPei/AmdSev.c: In function 'AmdSevInitialize': >>> OvmfPkg/PlatformPei/AmdSev.c:67:3: error: implicit declaration of >>> function '_PCD_SET_MODE_32_S_PcdOptionRomImageVerificationPolicy' >>> [-Werror=implicit-function-declaration] >>> PcdStatus = PcdSet32S (PcdOptionRomImageVerificationPolicy, 0x4); >>> ^ >>> cc1: all warnings being treated as errors >> >> There are three ways to fix the error: >> >> (1) Make the current, SB-only, 0x00 dynamic default unconditional. > > Maybe you should say something like, "As implemented in this patch..." > for item (1). Or, just drop (2) and (3) entirely in the commit > message. I'll leave that up to you. Thank you, Jordan; I dropped (2) and (3). Also added a last-minute reference to TianoCore BZ#737, in which the same issue has now been reported, independently. > > Reviewed-by: Jordan Justen Commit 1958124a6cb0. Thank you! Laszlo > >> >> This is the simplest approach, and it reflects the intent of original >> commit 1fea9ddb4e3f ("OvmfPkg: execute option ROM images regardless of >> Secure Boot", 2016-01-07). Without SECURE_BOOT_ENABLE, >> "SecurityPkg/Library/DxeImageVerificationLib" is not used anyway, so >> the PCD is never read. >> >> (2) Add an !else branch that explicitly sets the SecurityPkg.dec default >> (0x04) as dynamic default, if SECURE_BOOT_ENABLE is FALSE. >> >> This looks awkward because it explicitly sets a dynamic default that >> is then never read. >> >> (3) Set the SecurityPkg.dec default (0x04) as unconditional dynamic >> default, and invert the logic in AmdSevInitialize() >> [OvmfPkg/PlatformPei/AmdSev.c] -- set the PCD to 0x00 if SEV is >> disabled; don't touch it otherwise. >> >> I think this sends the wrong message -- for the time being anyway, SEV >> is the exception, not the rule. We shouldn't rely on the PCD getting >> its most commonly used value in a function called AmdSevInitialize(). >> >> This issue was caught and reported by Gerd Hoffmann 's >> Jenkins CI. >> >> Cc: Ard Biesheuvel >> Cc: Brijesh Singh >> Cc: Jordan Justen >> Fixes: 6041ac65ae879389f3ab5c0699f916d3e71c97fe >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Laszlo Ersek >> --- >> >> Notes: >> Repo: https://github.com/lersek/edk2.git >> Branch: oprom_policy_build_fix >> >> OvmfPkg/OvmfPkgIa32.dsc | 3 --- >> OvmfPkg/OvmfPkgIa32X64.dsc | 3 --- >> OvmfPkg/OvmfPkgX64.dsc | 3 --- >> 3 files changed, 9 deletions(-) >> >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >> index 7fb557b7c9cd..c2f534fdbf3b 100644 >> --- a/OvmfPkg/OvmfPkgIa32.dsc >> +++ b/OvmfPkg/OvmfPkgIa32.dsc >> @@ -540,10 +540,7 @@ [PcdsDynamicDefault] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 >> !endif >> >> -!if $(SECURE_BOOT_ENABLE) == TRUE >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 >> -!endif >> - >> >> ################################################################################ >> # >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >> index 4bcbddb95768..9f300a2e6f32 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >> @@ -548,10 +548,7 @@ [PcdsDynamicDefault] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 >> !endif >> >> -!if $(SECURE_BOOT_ENABLE) == TRUE >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 >> -!endif >> - >> >> ################################################################################ >> # >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index e52a3bd4db9b..1ffcf37f8b92 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -547,10 +547,7 @@ [PcdsDynamicDefault] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 >> !endif >> >> -!if $(SECURE_BOOT_ENABLE) == TRUE >> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00 >> -!endif >> - >> >> ################################################################################ >> # >> -- >> 2.14.1.3.gb7cf6e02401b >> > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >