Re: [PATCH] OvmfPkg: fix dynamic default for oprom verification policy PCD without SB

[Laszlo Ersek <lersek@redhat.com>]

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.justen@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 <kraxel@redhat.com>'s
>> Jenkins CI.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Fixes: 6041ac65ae879389f3ab5c0699f916d3e71c97fe
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@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
>