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 <jordan.l.jus...@intel.com>
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 <kra...@redhat.com>'s
>> Jenkins CI.
>>
>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> Cc: Brijesh Singh <brijesh.si...@amd.com>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Fixes: 6041ac65ae879389f3ab5c0699f916d3e71c97fe
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> 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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
