[PATCH 1/2] SecurityPkg: make PcdOptionRomImageVerificationPolicy dynamic

[PATCH 1/2] SecurityPkg: make PcdOptionRomImageVerificationPolicy dynamic

[Brijesh Singh]

By default the image verification policy for option ROM images is 0x4
(DENY_EXECUTE_ON_SECURITY_VIOLATION) but the following OvmfPkg commit:

1fea9ddb4e3f OvmfPkg: execute option ROM images regardless of Secure Boot

set it to 0x0 (ALWAYS_EXECUTE). This is fine because typically option
ROMs comes from host-side and most of the time cloud provider (i.e
hypervisor) have full access over a guest anyway. But when secure boot
is enabled, we would like to deny the execution of option ROM when
SEV is active. Having a dynamic Pcd will give us flexibility to set the
security policy at the runtime.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=728
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 SecurityPkg/SecurityPkg.dec | 24 ++++++++++----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
index 01bff01ed50a..4e32d172d7d9 100644
--- a/SecurityPkg/SecurityPkg.dec
+++ b/SecurityPkg/SecurityPkg.dec
@@ -230,18 +230,6 @@
 #
 
 [PcdsFixedAtBuild, PcdsPatchableInModule]
-  ## Image verification policy for OptionRom. Only following values are valid:<BR><BR>
-  #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification and has been removed.<BR>
-  #  0x00000000      Always trust the image.<BR>
-  #  0x00000001      Never trust the image.<BR>
-  #  0x00000002      Allow execution when there is security violation.<BR>
-  #  0x00000003      Defer execution when there is security violation.<BR>
-  #  0x00000004      Deny execution when there is security violation.<BR>
-  #  0x00000005      Query user when there is security violation.<BR>
-  # @Prompt Set policy for the image from OptionRom.
-  # @ValidRange 0x80000001 | 0x00000000 - 0x00000005
-  gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04|UINT32|0x00000001
-
   ## Image verification policy for removable media which includes CD-ROM, Floppy, USB and network.
   #  Only following values are valid:<BR><BR>
   #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification and has been removed.<BR>
@@ -304,6 +292,18 @@
   gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeSubClassTpmDevice|0x010D0000|UINT32|0x00000007
 
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
+  ## Image verification policy for OptionRom. Only following values are valid:<BR><BR>
+  #  NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification and has been removed.<BR>
+  #  0x00000000      Always trust the image.<BR>
+  #  0x00000001      Never trust the image.<BR>
+  #  0x00000002      Allow execution when there is security violation.<BR>
+  #  0x00000003      Defer execution when there is security violation.<BR>
+  #  0x00000004      Deny execution when there is security violation.<BR>
+  #  0x00000005      Query user when there is security violation.<BR>
+  # @Prompt Set policy for the image from OptionRom.
+  # @ValidRange 0x80000001 | 0x00000000 - 0x00000005
+  gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04|UINT32|0x00000001
+
   ## Indicates the presence or absence of the platform operator during firmware booting.
   #  If platform operator is not physical presence during boot. TPM will be locked and the TPM commands 
   #  that required operator physical presence can not run.<BR><BR>
-- 
2.9.5

[PATCH 2/2] OvmfPkg/PlatformPei: DENY_EXECUTE_ON_SECURITY_VIOLATION when SEV is active

[Brijesh Singh]

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 <chao.b.zhang@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 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.
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.
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);
 }
-- 
2.9.5

Re: [PATCH 2/2] OvmfPkg/PlatformPei: DENY_EXECUTE_ON_SECURITY_VIOLATION when SEV is active

[Laszlo Ersek]

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 <chao.b.zhang@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  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

Re: [PATCH 2/2] OvmfPkg/PlatformPei: DENY_EXECUTE_ON_SECURITY_VIOLATION when SEV is active

[Brijesh Singh]

Hi Laszlo,

On 10/05/2017 02:46 PM, Laszlo Ersek wrote:

....>>   ################################################################################
>>   #
>>   # 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.)
> 

I recently changed by build box and I did carried over my previous gitconfig
setting so that it generate the correct formating per your week but I might
have missed something in the process. I will fix it. sorry about that.


> 
> 
> (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".
> 

Ah good catch. I will fix in v2.

>>
> 
> (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.
> 

Yep, I had similar concern but as you said SecurityPkg.dec spells out
very well hence I did not bother to move the header file from internal
to somewhere its accessible by other modules.


> 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.
>