1. Patchew
  2. EDK2
  3. View series

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

Laszlo Ersek posted 1 patch 6 years, 6 months ago
Failed in applying to current master (apply log)
OvmfPkg/OvmfPkgIa32.dsc    | 3 ---
OvmfPkg/OvmfPkgIa32X64.dsc | 3 ---
OvmfPkg/OvmfPkgX64.dsc     | 3 ---
3 files changed, 9 deletions(-)
[edk2] [PATCH] OvmfPkg: fix dynamic default for oprom verification policy PCD without SB
Posted by Laszlo Ersek 6 years, 6 months ago 
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.

    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
Re: [edk2] [PATCH] OvmfPkg: fix dynamic default for oprom verification policy PCD without SB
Posted by Jordan Justen 6 years, 6 months ago 
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.

Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

> 
>     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
Re: [edk2] [PATCH] OvmfPkg: fix dynamic default for oprom verification policy PCD without SB
Posted by Laszlo Ersek 6 years, 6 months ago 
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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.