[PATCH v1 0/4] Don't require self-signed PK in setup mode

[PATCH v1 0/4] Don't require self-signed PK in setup mode

[Jan Bobek]

Hi all,

I'm sending out v1 of my patch series that addresses a UEFI spec
non-compliance when enrolling PK in setup mode. Additional info can be
found in bugzilla [1]; the changes are split into 4 patches as
suggested by Laszlo Ersek in comment #4.

I've based my work on the patch by Matthew Carlson; I've credited him
with co-authorship of the first patch even though in the end I decided
to do the implementation a bit differently.

Comments & reviews welcome!

Cheers,
-Jan

References:
1. https://bugzilla.tianocore.org/show_bug.cgi?id=2506

Jan Bobek (4):
  SecurityPkg: limit verification of enrolled PK in setup mode
  OvmfPkg: require self-signed PK when secure boot is enabled
  ArmVirtPkg: require self-signed PK when secure boot is enabled
  SecurityPkg: don't require PK to be self-signed by default

 SecurityPkg/SecurityPkg.dec                             | 7 +++++++
 ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
 ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
 ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
 OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
 OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
 OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
 OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
 OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
 OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
 OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
 SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
 SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
 13 files changed, 50 insertions(+), 2 deletions(-)

-- 
2.30.2

[PATCH v1 1/4] SecurityPkg: limit verification of enrolled PK in setup mode

[Jan Bobek]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506

Per UEFI spec, enrolling a new PK in setup mode should not require a
self-signature. Introduce a feature PCD called PcdRequireSelfSignedPk
to control this requirement. Default to TRUE in order to preserve the
legacy behavior.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Co-authored-by: Matthew Carlson <macarl@microsoft.com>
Signed-off-by: Jan Bobek <jbobek@nvidia.com>
---
 SecurityPkg/SecurityPkg.dec                             | 7 +++++++
 SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
 SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
index 8257f11d17c7..d3b7ad7ff6fb 100644
--- a/SecurityPkg/SecurityPkg.dec
+++ b/SecurityPkg/SecurityPkg.dec
@@ -580,5 +580,12 @@ [PcdsDynamic, PcdsDynamicEx]
   ## This PCD records LASA field in CC EVENTLOG ACPI table.
   gEfiSecurityPkgTokenSpaceGuid.PcdCcEventlogAcpiTableLasa|0|UINT64|0x00010026
 
+[PcdsFeatureFlag]
+  ## Indicates if the platform requires PK to be self-signed when setting the PK in setup mode.
+  #   TRUE  - Require PK to be self-signed.
+  #   FALSE - Do not require PK to be self-signed.
+  # @Prompt Require PK to be self-signed
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE|BOOLEAN|0x00010027
+
 [UserExtensions.TianoCore."ExtraFiles"]
   SecurityPkgExtra.uni
diff --git a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
index 8eadeebcebd7..e5985c5f8b60 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
+++ b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
@@ -86,3 +86,6 @@ [Guids]
   gEfiCertTypeRsa2048Sha256Guid  ## SOMETIMES_CONSUMES   ## GUID  # Unique ID for the type of the certificate.
   gEfiCertPkcs7Guid              ## SOMETIMES_CONSUMES   ## GUID  # Unique ID for the type of the certificate.
   gEfiCertX509Guid               ## SOMETIMES_CONSUMES   ## GUID  # Unique ID for the type of the signature.
+
+[FeaturePcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk
diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 054ee4d1d988..e9989695626e 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -603,7 +603,10 @@ ProcessVarWithPk (
   // Init state of Del. State may change due to secure check
   //
   Del = FALSE;
-  if ((InCustomMode () && UserPhysicalPresent ()) || ((mPlatformMode == SETUP_MODE) && !IsPk)) {
+  if (  (InCustomMode () && UserPhysicalPresent ())
+     || (  (mPlatformMode == SETUP_MODE)
+        && !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)))
+  {
     Payload     = (UINT8 *)Data + AUTHINFO2_SIZE (Data);
     PayloadSize = DataSize - AUTHINFO2_SIZE (Data);
     if (PayloadSize == 0) {
@@ -627,7 +630,9 @@ ProcessVarWithPk (
       return Status;
     }
 
-    if ((mPlatformMode != SETUP_MODE) || IsPk) {
+    if (  (mPlatformMode != SETUP_MODE)
+       || (FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk))
+    {
       Status = VendorKeyIsModified ();
     }
   } else if (mPlatformMode == USER_MODE) {
-- 
2.30.2

[PATCH v1 2/4] OvmfPkg: require self-signed PK when secure boot is enabled

[Jan Bobek]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506

In all DSC files that define SECURE_BOOT_ENABLE, opt-in into requiring
self-signed PK when SECURE_BOOT_ENABLE is TRUE.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Peter Grehan <grehan@freebsd.org>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Signed-off-by: Jan Bobek <jbobek@nvidia.com>
---
 OvmfPkg/Bhyve/BhyveX64.dsc       | 3 +++
 OvmfPkg/CloudHv/CloudHvX64.dsc   | 3 +++
 OvmfPkg/IntelTdx/IntelTdxX64.dsc | 3 +++
 OvmfPkg/Microvm/MicrovmX64.dsc   | 3 +++
 OvmfPkg/OvmfPkgIa32.dsc          | 3 +++
 OvmfPkg/OvmfPkgIa32X64.dsc       | 3 +++
 OvmfPkg/OvmfPkgX64.dsc           | 3 +++
 7 files changed, 21 insertions(+)

diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index befec670d4f3..66a2ae8868e5 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -422,6 +422,9 @@ [PcdsFeatureFlag]
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|TRUE
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index 7326417eab62..9cb267f98942 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -480,6 +480,9 @@ [PcdsFeatureFlag]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
 !endif
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 0f1e970fbbb3..93918b55b1a5 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -390,6 +390,9 @@ [PcdsFeatureFlag]
 !ifdef $(CSM_ENABLE)
   gUefiOvmfPkgTokenSpaceGuid.PcdCsmEnable|TRUE
 !endif
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
index 2d53b5c2950d..3c988f3e65e0 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -476,6 +476,9 @@ [PcdsFeatureFlag]
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index f232de13a7b6..22dc29330d2d 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -488,6 +488,9 @@ [PcdsFeatureFlag]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
 !endif
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a9d422bd9169..6b539814bdb0 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -493,6 +493,9 @@ [PcdsFeatureFlag]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
 !endif
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 3f970a79a08a..f6b8b342c4ed 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -513,6 +513,9 @@ [PcdsFeatureFlag]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport|TRUE
   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALSE
 !endif
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
 
 [PcdsFixedAtBuild]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
-- 
2.30.2

[PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled

[Jan Bobek]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506

In all DSC files that define SECURE_BOOT_ENABLE, opt-in into requiring
self-signed PK when SECURE_BOOT_ENABLE is TRUE.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Jan Bobek <jbobek@nvidia.com>
---
 ArmVirtPkg/ArmVirtCloudHv.dsc    | 4 ++++
 ArmVirtPkg/ArmVirtQemu.dsc       | 4 ++++
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ++++
 3 files changed, 12 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtCloudHv.dsc b/ArmVirtPkg/ArmVirtCloudHv.dsc
index 7ca7a391d9cf..dc33936d6f03 100644
--- a/ArmVirtPkg/ArmVirtCloudHv.dsc
+++ b/ArmVirtPkg/ArmVirtCloudHv.dsc
@@ -85,6 +85,10 @@ [PcdsFeatureFlag.common]
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
 
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
+
 [PcdsFixedAtBuild.common]
 !if $(ARCH) == AARCH64
   gArmTokenSpaceGuid.PcdVFPEnabled|1
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 0f1c6395488a..31fd0e5279ab 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -145,6 +145,10 @@ [PcdsFeatureFlag.common]
 
   gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
 
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
+
 [PcdsFixedAtBuild.common]
 !if $(ARCH) == AARCH64
   gArmTokenSpaceGuid.PcdVFPEnabled|1
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 807c85d48285..1e0f06c91137 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -114,6 +114,10 @@ [PcdsFeatureFlag.common]
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
 
+!if $(SECURE_BOOT_ENABLE) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
+!endif
+
 [PcdsFixedAtBuild.common]
 !if $(ARCH) == AARCH64
   gArmTokenSpaceGuid.PcdVFPEnabled|1
-- 
2.30.2

Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled

[Yao, Jiewen]

Could ArmVirtPkg maintainer(s) review this patch?

> -----Original Message-----
> From: Jan Bobek <jbobek@nvidia.com>
> Sent: Saturday, January 21, 2023 6:59 AM
> To: devel@edk2.groups.io
> Cc: Jan Bobek <jbobek@nvidia.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Leif Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar
> <sami.mujawar@arm.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is
> enabled
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506
> 
> In all DSC files that define SECURE_BOOT_ENABLE, opt-in into requiring
> self-signed PK when SECURE_BOOT_ENABLE is TRUE.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Jan Bobek <jbobek@nvidia.com>
> ---
>  ArmVirtPkg/ArmVirtCloudHv.dsc    | 4 ++++
>  ArmVirtPkg/ArmVirtQemu.dsc       | 4 ++++
>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtCloudHv.dsc b/ArmVirtPkg/ArmVirtCloudHv.dsc
> index 7ca7a391d9cf..dc33936d6f03 100644
> --- a/ArmVirtPkg/ArmVirtCloudHv.dsc
> +++ b/ArmVirtPkg/ArmVirtCloudHv.dsc
> @@ -85,6 +85,10 @@ [PcdsFeatureFlag.common]
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
> 
> +!if $(SECURE_BOOT_ENABLE) == TRUE
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> +!endif
> +
>  [PcdsFixedAtBuild.common]
>  !if $(ARCH) == AARCH64
>    gArmTokenSpaceGuid.PcdVFPEnabled|1
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 0f1c6395488a..31fd0e5279ab 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -145,6 +145,10 @@ [PcdsFeatureFlag.common]
> 
>    gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
> 
> +!if $(SECURE_BOOT_ENABLE) == TRUE
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> +!endif
> +
>  [PcdsFixedAtBuild.common]
>  !if $(ARCH) == AARCH64
>    gArmTokenSpaceGuid.PcdVFPEnabled|1
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 807c85d48285..1e0f06c91137 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -114,6 +114,10 @@ [PcdsFeatureFlag.common]
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
> 
> +!if $(SECURE_BOOT_ENABLE) == TRUE
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> +!endif
> +
>  [PcdsFixedAtBuild.common]
>  !if $(ARCH) == AARCH64
>    gArmTokenSpaceGuid.PcdVFPEnabled|1
> --
> 2.30.2

Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled

[Ard Biesheuvel]

On Fri, 20 Jan 2023 at 23:59, Jan Bobek <jbobek@nvidia.com> wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506
>
> In all DSC files that define SECURE_BOOT_ENABLE, opt-in into requiring
> self-signed PK when SECURE_BOOT_ENABLE is TRUE.
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Jan Bobek <jbobek@nvidia.com>

I have no problems with this patch, but I wonder if we need it. I
suppose this is intended to retain the previous behavior, but i don't
think that makes sense at all. Secure boot support in ArmVirtPkg is
not production quality in any case, and self-signed PKs are rather
pointless too, so I think we should just go with the new default
behavior of allowing unsigned PKs.


> ---
>  ArmVirtPkg/ArmVirtCloudHv.dsc    | 4 ++++
>  ArmVirtPkg/ArmVirtQemu.dsc       | 4 ++++
>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ++++
>  3 files changed, 12 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtCloudHv.dsc b/ArmVirtPkg/ArmVirtCloudHv.dsc
> index 7ca7a391d9cf..dc33936d6f03 100644
> --- a/ArmVirtPkg/ArmVirtCloudHv.dsc
> +++ b/ArmVirtPkg/ArmVirtCloudHv.dsc
> @@ -85,6 +85,10 @@ [PcdsFeatureFlag.common]
>
>    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
>
> +!if $(SECURE_BOOT_ENABLE) == TRUE
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> +!endif
> +
>  [PcdsFixedAtBuild.common]
>  !if $(ARCH) == AARCH64
>    gArmTokenSpaceGuid.PcdVFPEnabled|1
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 0f1c6395488a..31fd0e5279ab 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -145,6 +145,10 @@ [PcdsFeatureFlag.common]
>
>    gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
>
> +!if $(SECURE_BOOT_ENABLE) == TRUE
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> +!endif
> +
>  [PcdsFixedAtBuild.common]
>  !if $(ARCH) == AARCH64
>    gArmTokenSpaceGuid.PcdVFPEnabled|1
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 807c85d48285..1e0f06c91137 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -114,6 +114,10 @@ [PcdsFeatureFlag.common]
>
>    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
>
> +!if $(SECURE_BOOT_ENABLE) == TRUE
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> +!endif
> +
>  [PcdsFixedAtBuild.common]
>  !if $(ARCH) == AARCH64
>    gArmTokenSpaceGuid.PcdVFPEnabled|1
> --
> 2.30.2
>

Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled

[Yao, Jiewen]

That is fine. This patch is just to maintain the compatibility.

Feel free to drop it, if you think it is not needed for this platform.

I can merge rest patches at first.

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Friday, February 3, 2023 6:49 PM
> To: Jan Bobek <jbobek@nvidia.com>
> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif
> Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar
> <sami.mujawar@arm.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot
> is enabled
> 
> On Fri, 20 Jan 2023 at 23:59, Jan Bobek <jbobek@nvidia.com> wrote:
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506
> >
> > In all DSC files that define SECURE_BOOT_ENABLE, opt-in into requiring
> > self-signed PK when SECURE_BOOT_ENABLE is TRUE.
> >
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Jan Bobek <jbobek@nvidia.com>
> 
> I have no problems with this patch, but I wonder if we need it. I
> suppose this is intended to retain the previous behavior, but i don't
> think that makes sense at all. Secure boot support in ArmVirtPkg is
> not production quality in any case, and self-signed PKs are rather
> pointless too, so I think we should just go with the new default
> behavior of allowing unsigned PKs.
> 
> 
> > ---
> >  ArmVirtPkg/ArmVirtCloudHv.dsc    | 4 ++++
> >  ArmVirtPkg/ArmVirtQemu.dsc       | 4 ++++
> >  ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ++++
> >  3 files changed, 12 insertions(+)
> >
> > diff --git a/ArmVirtPkg/ArmVirtCloudHv.dsc b/ArmVirtPkg/ArmVirtCloudHv.dsc
> > index 7ca7a391d9cf..dc33936d6f03 100644
> > --- a/ArmVirtPkg/ArmVirtCloudHv.dsc
> > +++ b/ArmVirtPkg/ArmVirtCloudHv.dsc
> > @@ -85,6 +85,10 @@ [PcdsFeatureFlag.common]
> >
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
> >
> > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> > +!endif
> > +
> >  [PcdsFixedAtBuild.common]
> >  !if $(ARCH) == AARCH64
> >    gArmTokenSpaceGuid.PcdVFPEnabled|1
> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > index 0f1c6395488a..31fd0e5279ab 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > @@ -145,6 +145,10 @@ [PcdsFeatureFlag.common]
> >
> >    gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
> >
> > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> > +!endif
> > +
> >  [PcdsFixedAtBuild.common]
> >  !if $(ARCH) == AARCH64
> >    gArmTokenSpaceGuid.PcdVFPEnabled|1
> > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > index 807c85d48285..1e0f06c91137 100644
> > --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > @@ -114,6 +114,10 @@ [PcdsFeatureFlag.common]
> >
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
> >
> > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> > +!endif
> > +
> >  [PcdsFixedAtBuild.common]
> >  !if $(ARCH) == AARCH64
> >    gArmTokenSpaceGuid.PcdVFPEnabled|1
> > --
> > 2.30.2
> >

Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled

[Ard Biesheuvel]

On Fri, 3 Feb 2023 at 12:15, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>
> That is fine. This patch is just to maintain the compatibility.
>
> Feel free to drop it, if you think it is not needed for this platform.
>
> I can merge rest patches at first.
>

OK, please go ahead.



> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Friday, February 3, 2023 6:49 PM
> > To: Jan Bobek <jbobek@nvidia.com>
> > Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Leif
> > Lindholm <quic_llindhol@quicinc.com>; Sami Mujawar
> > <sami.mujawar@arm.com>; Gerd Hoffmann <kraxel@redhat.com>
> > Subject: Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot
> > is enabled
> >
> > On Fri, 20 Jan 2023 at 23:59, Jan Bobek <jbobek@nvidia.com> wrote:
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506
> > >
> > > In all DSC files that define SECURE_BOOT_ENABLE, opt-in into requiring
> > > self-signed PK when SECURE_BOOT_ENABLE is TRUE.
> > >
> > > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > Signed-off-by: Jan Bobek <jbobek@nvidia.com>
> >
> > I have no problems with this patch, but I wonder if we need it. I
> > suppose this is intended to retain the previous behavior, but i don't
> > think that makes sense at all. Secure boot support in ArmVirtPkg is
> > not production quality in any case, and self-signed PKs are rather
> > pointless too, so I think we should just go with the new default
> > behavior of allowing unsigned PKs.
> >
> >
> > > ---
> > >  ArmVirtPkg/ArmVirtCloudHv.dsc    | 4 ++++
> > >  ArmVirtPkg/ArmVirtQemu.dsc       | 4 ++++
> > >  ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ++++
> > >  3 files changed, 12 insertions(+)
> > >
> > > diff --git a/ArmVirtPkg/ArmVirtCloudHv.dsc b/ArmVirtPkg/ArmVirtCloudHv.dsc
> > > index 7ca7a391d9cf..dc33936d6f03 100644
> > > --- a/ArmVirtPkg/ArmVirtCloudHv.dsc
> > > +++ b/ArmVirtPkg/ArmVirtCloudHv.dsc
> > > @@ -85,6 +85,10 @@ [PcdsFeatureFlag.common]
> > >
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
> > >
> > > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> > > +!endif
> > > +
> > >  [PcdsFixedAtBuild.common]
> > >  !if $(ARCH) == AARCH64
> > >    gArmTokenSpaceGuid.PcdVFPEnabled|1
> > > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > > index 0f1c6395488a..31fd0e5279ab 100644
> > > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > > @@ -145,6 +145,10 @@ [PcdsFeatureFlag.common]
> > >
> > >    gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled|$(TPM2_ENABLE)
> > >
> > > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> > > +!endif
> > > +
> > >  [PcdsFixedAtBuild.common]
> > >  !if $(ARCH) == AARCH64
> > >    gArmTokenSpaceGuid.PcdVFPEnabled|1
> > > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > > index 807c85d48285..1e0f06c91137 100644
> > > --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > > +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > > @@ -114,6 +114,10 @@ [PcdsFeatureFlag.common]
> > >
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
> > >
> > > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE
> > > +!endif
> > > +
> > >  [PcdsFixedAtBuild.common]
> > >  !if $(ARCH) == AARCH64
> > >    gArmTokenSpaceGuid.PcdVFPEnabled|1
> > > --
> > > 2.30.2
> > >

Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled

[Gerd Hoffmann]

On Fri, Feb 03, 2023 at 11:49:07AM +0100, Ard Biesheuvel wrote:
> On Fri, 20 Jan 2023 at 23:59, Jan Bobek <jbobek@nvidia.com> wrote:
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506
> >
> > In all DSC files that define SECURE_BOOT_ENABLE, opt-in into requiring
> > self-signed PK when SECURE_BOOT_ENABLE is TRUE.
> >
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Jan Bobek <jbobek@nvidia.com>
> 
> I have no problems with this patch, but I wonder if we need it. I
> suppose this is intended to retain the previous behavior, but i don't
> think that makes sense at all. Secure boot support in ArmVirtPkg is
> not production quality in any case, and self-signed PKs are rather
> pointless too, so I think we should just go with the new default
> behavior of allowing unsigned PKs.

Hmm, reading this (and the bugzilla entry) I'm wondering what the point
in requiring a self-signed PK is.  I can't think of a case where this
brings a benefit.  Shouldn't we just relax the requirement everywhere,
especially given that this is what the spec asks for?

take care,
  Gerd

[PATCH v1 4/4] SecurityPkg: don't require PK to be self-signed by default

[Jan Bobek]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2506

Change the default value of PcdRequireSelfSignedPk to FALSE in
accordance with UEFI spec, which states that PK need not be
self-signed when enrolling in setup mode.

Note that this relaxes the legacy behavior, which required the PK to
be self-signed in this case.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Signed-off-by: Jan Bobek <jbobek@nvidia.com>
---
 SecurityPkg/SecurityPkg.dec | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
index d3b7ad7ff6fb..0382090f4e75 100644
--- a/SecurityPkg/SecurityPkg.dec
+++ b/SecurityPkg/SecurityPkg.dec
@@ -585,7 +585,7 @@ [PcdsFeatureFlag]
   #   TRUE  - Require PK to be self-signed.
   #   FALSE - Do not require PK to be self-signed.
   # @Prompt Require PK to be self-signed
-  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|TRUE|BOOLEAN|0x00010027
+  gEfiMdeModulePkgTokenSpaceGuid.PcdRequireSelfSignedPk|FALSE|BOOLEAN|0x00010027
 
 [UserExtensions.TianoCore."ExtraFiles"]
   SecurityPkgExtra.uni
-- 
2.30.2

Re: [PATCH v1 0/4] Don't require self-signed PK in setup mode

[Yao, Jiewen]

Hi Sean
I would like to hear your feedback, since it is a little different from the original MSFT patch.

Would you please take a look?

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Jan Bobek <jbobek@nvidia.com>
> Sent: Saturday, January 21, 2023 6:59 AM
> To: devel@edk2.groups.io
> Cc: Jan Bobek <jbobek@nvidia.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
> 
> Hi all,
> 
> I'm sending out v1 of my patch series that addresses a UEFI spec
> non-compliance when enrolling PK in setup mode. Additional info can be
> found in bugzilla [1]; the changes are split into 4 patches as
> suggested by Laszlo Ersek in comment #4.
> 
> I've based my work on the patch by Matthew Carlson; I've credited him
> with co-authorship of the first patch even though in the end I decided
> to do the implementation a bit differently.
> 
> Comments & reviews welcome!
> 
> Cheers,
> -Jan
> 
> References:
> 1. https://bugzilla.tianocore.org/show_bug.cgi?id=2506
> 
> Jan Bobek (4):
>   SecurityPkg: limit verification of enrolled PK in setup mode
>   OvmfPkg: require self-signed PK when secure boot is enabled
>   ArmVirtPkg: require self-signed PK when secure boot is enabled
>   SecurityPkg: don't require PK to be self-signed by default
> 
>  SecurityPkg/SecurityPkg.dec                             | 7 +++++++
>  ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
>  ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
>  OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
>  OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
>  OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
>  OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
>  OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
>  OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
>  SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
>  SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
>  13 files changed, 50 insertions(+), 2 deletions(-)
> 
> --
> 2.30.2

Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode

[Sean]

[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]

Jan,

 From looking over the patch 1/4 email i have a concern.

In AuthService.c on the conditional change you made.  Aren't we losing a 
case where we are evaluating a nonPK payload signed by the PK?  Given 
the system is in SetupMode that means there is no PK but is this the 
conditional path that is used when installing Secure boot keys in 
reverse (DBX,DX,KEK,PK) order?

Is there testing you have done?  This code should be pretty easy to do 
host based unit testing on.  Any chance you have authored that to 
confirm use cases are not unexpectedly impacted?  Example of host based 
unit test of library is here: 
edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master · 
tianocore/edk2 (github.com) 
<https://github.com/tianocore/edk2/tree/master/SecurityPkg/Library/SecureBootVariableLib/UnitTest>


Thanks

Sean




On 1/22/2023 10:13 PM, Yao, Jiewen wrote:
> Hi Sean
> I would like to hear your feedback, since it is a little different from the original MSFT patch.
>
> Would you please take a look?
>
> Thank you
> Yao, Jiewen
>
>> -----Original Message-----
>> From: Jan Bobek<jbobek@nvidia.com>
>> Sent: Saturday, January 21, 2023 6:59 AM
>> To:devel@edk2.groups.io
>> Cc: Jan Bobek<jbobek@nvidia.com>; Laszlo Ersek<lersek@redhat.com>; Yao,
>> Jiewen<jiewen.yao@intel.com>
>> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
>>
>> Hi all,
>>
>> I'm sending out v1 of my patch series that addresses a UEFI spec
>> non-compliance when enrolling PK in setup mode. Additional info can be
>> found in bugzilla [1]; the changes are split into 4 patches as
>> suggested by Laszlo Ersek in comment #4.
>>
>> I've based my work on the patch by Matthew Carlson; I've credited him
>> with co-authorship of the first patch even though in the end I decided
>> to do the implementation a bit differently.
>>
>> Comments & reviews welcome!
>>
>> Cheers,
>> -Jan
>>
>> References:
>> 1.https://bugzilla.tianocore.org/show_bug.cgi?id=2506
>>
>> Jan Bobek (4):
>>    SecurityPkg: limit verification of enrolled PK in setup mode
>>    OvmfPkg: require self-signed PK when secure boot is enabled
>>    ArmVirtPkg: require self-signed PK when secure boot is enabled
>>    SecurityPkg: don't require PK to be self-signed by default
>>
>>   SecurityPkg/SecurityPkg.dec                             | 7 +++++++
>>   ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
>>   ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
>>   ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
>>   OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
>>   OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
>>   OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
>>   OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
>>   OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
>>   OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
>>   OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
>>   SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
>>   SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
>>   13 files changed, 50 insertions(+), 2 deletions(-)
>>
>> --
>> 2.30.2
>
>
> 
>
>

[-- Attachment #2: Type: text/html, Size: 4292 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode

[Jan Bobek]

Hi Sean,

> From looking over the patch 1/4 email i have a concern.
>
> In AuthService.c on the conditional change you made. Aren't we losing
> a case where we are evaluating a nonPK payload signed by the PK?
> Given the system is in SetupMode that means there is no PK but is this
> the conditional path that is used when installing Secure boot keys in
> reverse (DBX,DX,KEK,PK) order?

Thanks for sharing your concern! They way I think about the change is
that I've replaced the expression

    IsPk

with

    FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk

and nothing else. When you look at it this way, it's fairly easy to
break down how it affects the logic:

1. Assume PcdRequireSelfSignedPk is TRUE. In this case, the two
   expressions are exactly the same and no change in behavior occurs,
   just as we want.

2. Assume IsPk is FALSE. In this case, both expressions evaluate to
   FALSE, no matter what the PCD is configured to. That's also good,
   because we don't want to change how non-PK payloads are handled.

3. In fact, the only change in behavior that occurs is when
   PcdRequireSelfSignedPk is FALSE and IsPk is TRUE; here the former
   expression would be TRUE, whereas the latter is FALSE. That's exactly
   what we want: we wish to enter the first block of the if-else branch
   (which changes the variable similarly to when we're in custom mode)
   rather than falling through to the third block (which checks the
   self-signature).

To directly answer your question, I don't think the behavior changes at
all when processing non-PK payloads, by virtue of IsPk being FALSE and
what I said in point (2.) above.

Additionally, yes, the first block of the if-else branch is exactly the
path that's taken when enrolling KEK/DB/DBX in Setup mode, and one that
has always been available even without Custom mode. It used to be that
you couldn't use this path to enroll PK without Custom mode (precisely
because of !IsPk in the condition), but I'm hoping to enable this path
with my patch.

Let me know if I haven't answered or misunderstood your question.

> Is there testing you have done?  This code should be pretty easy to do
> host based unit testing on. Any chance you have authored that to
> confirm use cases are not unexpectedly impacted?  Example of host
> based unit test of library is here:
> edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master ·
> tianocore/edk2
> (github.com)<https://github.com/tianocore/edk2/tree/master/SecurityPkg/Library/SecureBootVariableLib/UnitTest>

I've done an ad-hoc test by commenting out the switch to/from Custom
mode in EnrollFromDefaultKeysApp from SecurityPkg, booting in Setup mode
and using the modified app to enroll the keys. You could do a similar
test from the OS, but in my case this was more straightforward.

I am aware of the host-based unit testing library in edk2, and I agree
that it would be great to have the code in AuthVariableLib tested for
all the different cases. However, I don't have any such tests right now,
and while I'm willing to potentially look into writing some, I'd have to
do it more or less on the side, meaning it could take a while.

Best,
-Jan

> On 1/22/2023 10:13 PM, Yao, Jiewen wrote:
>
> Hi Sean
> I would like to hear your feedback, since it is a little different from the original MSFT patch.
>
> Would you please take a look?
>
> Thank you
> Yao, Jiewen
>
>
>
> -----Original Message-----
> From: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>
> Sent: Saturday, January 21, 2023 6:59 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>; Laszlo Ersek <lersek@redhat.com><mailto:lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com><mailto:jiewen.yao@intel.com>
> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
>
> Hi all,
>
> I'm sending out v1 of my patch series that addresses a UEFI spec
> non-compliance when enrolling PK in setup mode. Additional info can be
> found in bugzilla [1]; the changes are split into 4 patches as
> suggested by Laszlo Ersek in comment #4.
>
> I've based my work on the patch by Matthew Carlson; I've credited him
> with co-authorship of the first patch even though in the end I decided
> to do the implementation a bit differently.
>
> Comments & reviews welcome!
>
> Cheers,
> -Jan
>
> References:
> 1. https://bugzilla.tianocore.org/show_bug.cgi?id=2506
>
> Jan Bobek (4):
>  SecurityPkg: limit verification of enrolled PK in setup mode
>  OvmfPkg: require self-signed PK when secure boot is enabled
>  ArmVirtPkg: require self-signed PK when secure boot is enabled
>  SecurityPkg: don't require PK to be self-signed by default
>
> SecurityPkg/SecurityPkg.dec                             | 7 +++++++
> ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
> ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
> ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
> OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
> OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
> OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
> OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
> OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
> OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
> OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
> SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
> 13 files changed, 50 insertions(+), 2 deletions(-)

Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode

[Sean]

I read your replacement a little different.

-  if ((InCustomMode () && UserPhysicalPresent ()) || ((mPlatformMode == SETUP_MODE) && !IsPk)) {

with

+  if (  (InCustomMode () && UserPhysicalPresent ())
+     || (  (mPlatformMode == SETUP_MODE)
+        && !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)))
+  {


In the upper part you replaced it says !IsPk.  What am i missing?


If a payload was in this function targeting a KEK change with no PK installed it would go in the original IF condition.
In the new code it would  because device is not in custom mode and the payload is not targeted PK.

Thanks
Sean
  



On 1/25/2023 1:38 PM, Jan Bobek wrote:
> Hi Sean,
>
>>  From looking over the patch 1/4 email i have a concern.
>>
>> In AuthService.c on the conditional change you made. Aren't we losing
>> a case where we are evaluating a nonPK payload signed by the PK?
>> Given the system is in SetupMode that means there is no PK but is this
>> the conditional path that is used when installing Secure boot keys in
>> reverse (DBX,DX,KEK,PK) order?
> Thanks for sharing your concern! They way I think about the change is
> that I've replaced the expression
>
>      IsPk
>
> with
>
>      FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk
>
> and nothing else. When you look at it this way, it's fairly easy to
> break down how it affects the logic:
>
> 1. Assume PcdRequireSelfSignedPk is TRUE. In this case, the two
>     expressions are exactly the same and no change in behavior occurs,
>     just as we want.
>
> 2. Assume IsPk is FALSE. In this case, both expressions evaluate to
>     FALSE, no matter what the PCD is configured to. That's also good,
>     because we don't want to change how non-PK payloads are handled.
>
> 3. In fact, the only change in behavior that occurs is when
>     PcdRequireSelfSignedPk is FALSE and IsPk is TRUE; here the former
>     expression would be TRUE, whereas the latter is FALSE. That's exactly
>     what we want: we wish to enter the first block of the if-else branch
>     (which changes the variable similarly to when we're in custom mode)
>     rather than falling through to the third block (which checks the
>     self-signature).
>
> To directly answer your question, I don't think the behavior changes at
> all when processing non-PK payloads, by virtue of IsPk being FALSE and
> what I said in point (2.) above.
>
> Additionally, yes, the first block of the if-else branch is exactly the
> path that's taken when enrolling KEK/DB/DBX in Setup mode, and one that
> has always been available even without Custom mode. It used to be that
> you couldn't use this path to enroll PK without Custom mode (precisely
> because of !IsPk in the condition), but I'm hoping to enable this path
> with my patch.
>
> Let me know if I haven't answered or misunderstood your question.
>
>> Is there testing you have done?  This code should be pretty easy to do
>> host based unit testing on. Any chance you have authored that to
>> confirm use cases are not unexpectedly impacted?  Example of host
>> based unit test of library is here:
>> edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master ·
>> tianocore/edk2
>> (github.com)<https://github.com/tianocore/edk2/tree/master/SecurityPkg/Library/SecureBootVariableLib/UnitTest>
> I've done an ad-hoc test by commenting out the switch to/from Custom
> mode in EnrollFromDefaultKeysApp from SecurityPkg, booting in Setup mode
> and using the modified app to enroll the keys. You could do a similar
> test from the OS, but in my case this was more straightforward.
>
> I am aware of the host-based unit testing library in edk2, and I agree
> that it would be great to have the code in AuthVariableLib tested for
> all the different cases. However, I don't have any such tests right now,
> and while I'm willing to potentially look into writing some, I'd have to
> do it more or less on the side, meaning it could take a while.
>
> Best,
> -Jan
>
>> On 1/22/2023 10:13 PM, Yao, Jiewen wrote:
>>
>> Hi Sean
>> I would like to hear your feedback, since it is a little different from the original MSFT patch.
>>
>> Would you please take a look?
>>
>> Thank you
>> Yao, Jiewen
>>
>>
>>
>> -----Original Message-----
>> From: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>
>> Sent: Saturday, January 21, 2023 6:59 AM
>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>> Cc: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>; Laszlo Ersek <lersek@redhat.com><mailto:lersek@redhat.com>; Yao,
>> Jiewen <jiewen.yao@intel.com><mailto:jiewen.yao@intel.com>
>> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
>>
>> Hi all,
>>
>> I'm sending out v1 of my patch series that addresses a UEFI spec
>> non-compliance when enrolling PK in setup mode. Additional info can be
>> found in bugzilla [1]; the changes are split into 4 patches as
>> suggested by Laszlo Ersek in comment #4.
>>
>> I've based my work on the patch by Matthew Carlson; I've credited him
>> with co-authorship of the first patch even though in the end I decided
>> to do the implementation a bit differently.
>>
>> Comments & reviews welcome!
>>
>> Cheers,
>> -Jan
>>
>> References:
>> 1. https://bugzilla.tianocore.org/show_bug.cgi?id=2506
>>
>> Jan Bobek (4):
>>   SecurityPkg: limit verification of enrolled PK in setup mode
>>   OvmfPkg: require self-signed PK when secure boot is enabled
>>   ArmVirtPkg: require self-signed PK when secure boot is enabled
>>   SecurityPkg: don't require PK to be self-signed by default
>>
>> SecurityPkg/SecurityPkg.dec                             | 7 +++++++
>> ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
>> ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
>> ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
>> OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
>> OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
>> OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
>> OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
>> OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
>> OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
>> OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
>> SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
>> 13 files changed, 50 insertions(+), 2 deletions(-)

Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode

[Jan Bobek]

> I read your replacement a little different.
>
> -  if ((InCustomMode () && UserPhysicalPresent ()) || ((mPlatformMode == SETUP_MODE) && !IsPk)) {
>
> with
>
> +  if (  (InCustomMode () && UserPhysicalPresent ())
> +     || (  (mPlatformMode == SETUP_MODE)
> +        && !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)))
> +  {
>
>
> In the upper part you replaced it says !IsPk.  What am i missing?
>
>
> If a payload was in this function targeting a KEK change with no PK
> installed it would go in the original IF condition.  In the new code
> it would because device is not in custom mode and the payload is not
> targeted PK.

Is it possible that you're missing the negation (i.e. exclamation mark)
in front of the parethesis? The old code was

  !IsPk

The new code is

  !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)

If IsPk is FALSE, both of these evaluate to TRUE no matter what the PCD
is.

-Jan

> On 1/25/2023 1:38 PM, Jan Bobek wrote:
>> Hi Sean,
>>
>>>  From looking over the patch 1/4 email i have a concern.
>>>
>>> In AuthService.c on the conditional change you made. Aren't we losing
>>> a case where we are evaluating a nonPK payload signed by the PK?
>>> Given the system is in SetupMode that means there is no PK but is this
>>> the conditional path that is used when installing Secure boot keys in
>>> reverse (DBX,DX,KEK,PK) order?
>> Thanks for sharing your concern! They way I think about the change is
>> that I've replaced the expression
>>
>>      IsPk
>>
>> with
>>
>>      FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk
>>
>> and nothing else. When you look at it this way, it's fairly easy to
>> break down how it affects the logic:
>>
>> 1. Assume PcdRequireSelfSignedPk is TRUE. In this case, the two
>>     expressions are exactly the same and no change in behavior occurs,
>>     just as we want.
>>
>> 2. Assume IsPk is FALSE. In this case, both expressions evaluate to
>>     FALSE, no matter what the PCD is configured to. That's also good,
>>     because we don't want to change how non-PK payloads are handled.
>>
>> 3. In fact, the only change in behavior that occurs is when
>>     PcdRequireSelfSignedPk is FALSE and IsPk is TRUE; here the former
>>     expression would be TRUE, whereas the latter is FALSE. That's exactly
>>     what we want: we wish to enter the first block of the if-else branch
>>     (which changes the variable similarly to when we're in custom mode)
>>     rather than falling through to the third block (which checks the
>>     self-signature).
>>
>> To directly answer your question, I don't think the behavior changes at
>> all when processing non-PK payloads, by virtue of IsPk being FALSE and
>> what I said in point (2.) above.
>>
>> Additionally, yes, the first block of the if-else branch is exactly the
>> path that's taken when enrolling KEK/DB/DBX in Setup mode, and one that
>> has always been available even without Custom mode. It used to be that
>> you couldn't use this path to enroll PK without Custom mode (precisely
>> because of !IsPk in the condition), but I'm hoping to enable this path
>> with my patch.
>>
>> Let me know if I haven't answered or misunderstood your question.
>>
>>> Is there testing you have done?  This code should be pretty easy to do
>>> host based unit testing on. Any chance you have authored that to
>>> confirm use cases are not unexpectedly impacted?  Example of host
>>> based unit test of library is here:
>>> edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master ·
>>> tianocore/edk2
>>> (github.com)<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Ftree%2Fmaster%2FSecurityPkg%2FLibrary%2FSecureBootVariableLib%2FUnitTest&data=05%7C01%7Cjbobek%40nvidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=djptmpLUwESbPeqFwFUz5sVbS1MBnD%2FPua9H2y1nxFE%3D&reserved=0>
>> I've done an ad-hoc test by commenting out the switch to/from Custom
>> mode in EnrollFromDefaultKeysApp from SecurityPkg, booting in Setup mode
>> and using the modified app to enroll the keys. You could do a similar
>> test from the OS, but in my case this was more straightforward.
>>
>> I am aware of the host-based unit testing library in edk2, and I agree
>> that it would be great to have the code in AuthVariableLib tested for
>> all the different cases. However, I don't have any such tests right now,
>> and while I'm willing to potentially look into writing some, I'd have to
>> do it more or less on the side, meaning it could take a while.
>>
>> Best,
>> -Jan
>>
>>> On 1/22/2023 10:13 PM, Yao, Jiewen wrote:
>>>
>>> Hi Sean
>>> I would like to hear your feedback, since it is a little different from the original MSFT patch.
>>>
>>> Would you please take a look?
>>>
>>> Thank you
>>> Yao, Jiewen
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>
>>> Sent: Saturday, January 21, 2023 6:59 AM
>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>>> Cc: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>; Laszlo Ersek <lersek@redhat.com><mailto:lersek@redhat.com>; Yao,
>>> Jiewen <jiewen.yao@intel.com><mailto:jiewen.yao@intel.com>
>>> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
>>>
>>> Hi all,
>>>
>>> I'm sending out v1 of my patch series that addresses a UEFI spec
>>> non-compliance when enrolling PK in setup mode. Additional info can be
>>> found in bugzilla [1]; the changes are split into 4 patches as
>>> suggested by Laszlo Ersek in comment #4.
>>>
>>> I've based my work on the patch by Matthew Carlson; I've credited him
>>> with co-authorship of the first patch even though in the end I decided
>>> to do the implementation a bit differently.
>>>
>>> Comments & reviews welcome!
>>>
>>> Cheers,
>>> -Jan
>>>
>>> References:
>>> 1. https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2506&data=05%7C01%7Cjbobek%40nvidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=PXam5BVxMmUwgRdGsq1RcnwC8yb8nmOzcLz3oKica7s%3D&reserved=0
>>>
>>> Jan Bobek (4):
>>>   SecurityPkg: limit verification of enrolled PK in setup mode
>>>   OvmfPkg: require self-signed PK when secure boot is enabled
>>>   ArmVirtPkg: require self-signed PK when secure boot is enabled
>>>   SecurityPkg: don't require PK to be self-signed by default
>>>
>>> SecurityPkg/SecurityPkg.dec                             | 7 +++++++
>>> ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
>>> ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
>>> ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
>>> OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
>>> OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
>>> OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
>>> OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
>>> OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
>>> OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
>>> OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
>>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
>>> SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
>>> 13 files changed, 50 insertions(+), 2 deletions(-)

Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode

[Sean]

Did i mention how much i hate reviewing code over email?  :)

Even after looking at it a few times I missed that change.

I think this change looks fine.  Personally, i think this code has 
terrible readability and high complexity and with high impact.  It would 
be great to at least get unit tests if not a full refactor of the 
library.  I also think the edk2 idea of "custom mode" should be 
removed.  But that feedback isn't for this patch and I don't think this 
patch should be held up just because the surrounding code is less than 
ideal.

For patch 1 and 4.

Reviewed-by: Sean Brogan <sean.brogan@microsoft.com>

Thanks

Sean




On 1/27/2023 2:05 PM, Jan Bobek wrote:
>> I read your replacement a little different.
>>
>> -  if ((InCustomMode () && UserPhysicalPresent ()) || ((mPlatformMode == SETUP_MODE) && !IsPk)) {
>>
>> with
>>
>> +  if (  (InCustomMode () && UserPhysicalPresent ())
>> +     || (  (mPlatformMode == SETUP_MODE)
>> +        && !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)))
>> +  {
>>
>>
>> In the upper part you replaced it says !IsPk.  What am i missing?
>>
>>
>> If a payload was in this function targeting a KEK change with no PK
>> installed it would go in the original IF condition.  In the new code
>> it would because device is not in custom mode and the payload is not
>> targeted PK.
> Is it possible that you're missing the negation (i.e. exclamation mark)
> in front of the parethesis? The old code was
>
>    !IsPk
>
> The new code is
>
>    !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)
>
> If IsPk is FALSE, both of these evaluate to TRUE no matter what the PCD
> is.
>
> -Jan
>
>> On 1/25/2023 1:38 PM, Jan Bobek wrote:
>>> Hi Sean,
>>>
>>>>   From looking over the patch 1/4 email i have a concern.
>>>>
>>>> In AuthService.c on the conditional change you made. Aren't we losing
>>>> a case where we are evaluating a nonPK payload signed by the PK?
>>>> Given the system is in SetupMode that means there is no PK but is this
>>>> the conditional path that is used when installing Secure boot keys in
>>>> reverse (DBX,DX,KEK,PK) order?
>>> Thanks for sharing your concern! They way I think about the change is
>>> that I've replaced the expression
>>>
>>>       IsPk
>>>
>>> with
>>>
>>>       FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk
>>>
>>> and nothing else. When you look at it this way, it's fairly easy to
>>> break down how it affects the logic:
>>>
>>> 1. Assume PcdRequireSelfSignedPk is TRUE. In this case, the two
>>>      expressions are exactly the same and no change in behavior occurs,
>>>      just as we want.
>>>
>>> 2. Assume IsPk is FALSE. In this case, both expressions evaluate to
>>>      FALSE, no matter what the PCD is configured to. That's also good,
>>>      because we don't want to change how non-PK payloads are handled.
>>>
>>> 3. In fact, the only change in behavior that occurs is when
>>>      PcdRequireSelfSignedPk is FALSE and IsPk is TRUE; here the former
>>>      expression would be TRUE, whereas the latter is FALSE. That's exactly
>>>      what we want: we wish to enter the first block of the if-else branch
>>>      (which changes the variable similarly to when we're in custom mode)
>>>      rather than falling through to the third block (which checks the
>>>      self-signature).
>>>
>>> To directly answer your question, I don't think the behavior changes at
>>> all when processing non-PK payloads, by virtue of IsPk being FALSE and
>>> what I said in point (2.) above.
>>>
>>> Additionally, yes, the first block of the if-else branch is exactly the
>>> path that's taken when enrolling KEK/DB/DBX in Setup mode, and one that
>>> has always been available even without Custom mode. It used to be that
>>> you couldn't use this path to enroll PK without Custom mode (precisely
>>> because of !IsPk in the condition), but I'm hoping to enable this path
>>> with my patch.
>>>
>>> Let me know if I haven't answered or misunderstood your question.
>>>
>>>> Is there testing you have done?  This code should be pretty easy to do
>>>> host based unit testing on. Any chance you have authored that to
>>>> confirm use cases are not unexpectedly impacted?  Example of host
>>>> based unit test of library is here:
>>>> edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master ·
>>>> tianocore/edk2
>>>> (github.com)<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Ftree%2Fmaster%2FSecurityPkg%2FLibrary%2FSecureBootVariableLib%2FUnitTest&data=05%7C01%7Cjbobek%40nvidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=djptmpLUwESbPeqFwFUz5sVbS1MBnD%2FPua9H2y1nxFE%3D&reserved=0>
>>> I've done an ad-hoc test by commenting out the switch to/from Custom
>>> mode in EnrollFromDefaultKeysApp from SecurityPkg, booting in Setup mode
>>> and using the modified app to enroll the keys. You could do a similar
>>> test from the OS, but in my case this was more straightforward.
>>>
>>> I am aware of the host-based unit testing library in edk2, and I agree
>>> that it would be great to have the code in AuthVariableLib tested for
>>> all the different cases. However, I don't have any such tests right now,
>>> and while I'm willing to potentially look into writing some, I'd have to
>>> do it more or less on the side, meaning it could take a while.
>>>
>>> Best,
>>> -Jan
>>>
>>>> On 1/22/2023 10:13 PM, Yao, Jiewen wrote:
>>>>
>>>> Hi Sean
>>>> I would like to hear your feedback, since it is a little different from the original MSFT patch.
>>>>
>>>> Would you please take a look?
>>>>
>>>> Thank you
>>>> Yao, Jiewen
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>
>>>> Sent: Saturday, January 21, 2023 6:59 AM
>>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>>>> Cc: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>; Laszlo Ersek <lersek@redhat.com><mailto:lersek@redhat.com>; Yao,
>>>> Jiewen <jiewen.yao@intel.com><mailto:jiewen.yao@intel.com>
>>>> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
>>>>
>>>> Hi all,
>>>>
>>>> I'm sending out v1 of my patch series that addresses a UEFI spec
>>>> non-compliance when enrolling PK in setup mode. Additional info can be
>>>> found in bugzilla [1]; the changes are split into 4 patches as
>>>> suggested by Laszlo Ersek in comment #4.
>>>>
>>>> I've based my work on the patch by Matthew Carlson; I've credited him
>>>> with co-authorship of the first patch even though in the end I decided
>>>> to do the implementation a bit differently.
>>>>
>>>> Comments & reviews welcome!
>>>>
>>>> Cheers,
>>>> -Jan
>>>>
>>>> References:
>>>> 1. https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2506&data=05%7C01%7Cjbobek%40nvidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=PXam5BVxMmUwgRdGsq1RcnwC8yb8nmOzcLz3oKica7s%3D&reserved=0
>>>>
>>>> Jan Bobek (4):
>>>>    SecurityPkg: limit verification of enrolled PK in setup mode
>>>>    OvmfPkg: require self-signed PK when secure boot is enabled
>>>>    ArmVirtPkg: require self-signed PK when secure boot is enabled
>>>>    SecurityPkg: don't require PK to be self-signed by default
>>>>
>>>> SecurityPkg/SecurityPkg.dec                             | 7 +++++++
>>>> ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
>>>> ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
>>>> ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
>>>> OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
>>>> OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
>>>> OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
>>>> OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
>>>> OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
>>>> OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
>>>> OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
>>>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
>>>> SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
>>>> 13 files changed, 50 insertions(+), 2 deletions(-)

Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode

[Yao, Jiewen]

Thanks Sean.

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Sean Brogan <spbrogan@outlook.com>
> Sent: Saturday, January 28, 2023 10:37 AM
> To: Jan Bobek <jbobek@nvidia.com>
> Cc: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup
> mode
> 
> Did i mention how much i hate reviewing code over email?  :)
> 
> Even after looking at it a few times I missed that change.
> 
> I think this change looks fine.  Personally, i think this code has
> terrible readability and high complexity and with high impact.  It would
> be great to at least get unit tests if not a full refactor of the
> library.  I also think the edk2 idea of "custom mode" should be
> removed.  But that feedback isn't for this patch and I don't think this
> patch should be held up just because the surrounding code is less than
> ideal.
> 
> For patch 1 and 4.
> 
> Reviewed-by: Sean Brogan <sean.brogan@microsoft.com>
> 
> Thanks
> 
> Sean
> 
> 
> 
> 
> On 1/27/2023 2:05 PM, Jan Bobek wrote:
> >> I read your replacement a little different.
> >>
> >> -  if ((InCustomMode () && UserPhysicalPresent ()) || ((mPlatformMode ==
> SETUP_MODE) && !IsPk)) {
> >>
> >> with
> >>
> >> +  if (  (InCustomMode () && UserPhysicalPresent ())
> >> +     || (  (mPlatformMode == SETUP_MODE)
> >> +        && !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)))
> >> +  {
> >>
> >>
> >> In the upper part you replaced it says !IsPk.  What am i missing?
> >>
> >>
> >> If a payload was in this function targeting a KEK change with no PK
> >> installed it would go in the original IF condition.  In the new code
> >> it would because device is not in custom mode and the payload is not
> >> targeted PK.
> > Is it possible that you're missing the negation (i.e. exclamation mark)
> > in front of the parethesis? The old code was
> >
> >    !IsPk
> >
> > The new code is
> >
> >    !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)
> >
> > If IsPk is FALSE, both of these evaluate to TRUE no matter what the PCD
> > is.
> >
> > -Jan
> >
> >> On 1/25/2023 1:38 PM, Jan Bobek wrote:
> >>> Hi Sean,
> >>>
> >>>>   From looking over the patch 1/4 email i have a concern.
> >>>>
> >>>> In AuthService.c on the conditional change you made. Aren't we losing
> >>>> a case where we are evaluating a nonPK payload signed by the PK?
> >>>> Given the system is in SetupMode that means there is no PK but is this
> >>>> the conditional path that is used when installing Secure boot keys in
> >>>> reverse (DBX,DX,KEK,PK) order?
> >>> Thanks for sharing your concern! They way I think about the change is
> >>> that I've replaced the expression
> >>>
> >>>       IsPk
> >>>
> >>> with
> >>>
> >>>       FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk
> >>>
> >>> and nothing else. When you look at it this way, it's fairly easy to
> >>> break down how it affects the logic:
> >>>
> >>> 1. Assume PcdRequireSelfSignedPk is TRUE. In this case, the two
> >>>      expressions are exactly the same and no change in behavior occurs,
> >>>      just as we want.
> >>>
> >>> 2. Assume IsPk is FALSE. In this case, both expressions evaluate to
> >>>      FALSE, no matter what the PCD is configured to. That's also good,
> >>>      because we don't want to change how non-PK payloads are handled.
> >>>
> >>> 3. In fact, the only change in behavior that occurs is when
> >>>      PcdRequireSelfSignedPk is FALSE and IsPk is TRUE; here the former
> >>>      expression would be TRUE, whereas the latter is FALSE. That's exactly
> >>>      what we want: we wish to enter the first block of the if-else branch
> >>>      (which changes the variable similarly to when we're in custom mode)
> >>>      rather than falling through to the third block (which checks the
> >>>      self-signature).
> >>>
> >>> To directly answer your question, I don't think the behavior changes at
> >>> all when processing non-PK payloads, by virtue of IsPk being FALSE and
> >>> what I said in point (2.) above.
> >>>
> >>> Additionally, yes, the first block of the if-else branch is exactly the
> >>> path that's taken when enrolling KEK/DB/DBX in Setup mode, and one that
> >>> has always been available even without Custom mode. It used to be that
> >>> you couldn't use this path to enroll PK without Custom mode (precisely
> >>> because of !IsPk in the condition), but I'm hoping to enable this path
> >>> with my patch.
> >>>
> >>> Let me know if I haven't answered or misunderstood your question.
> >>>
> >>>> Is there testing you have done?  This code should be pretty easy to do
> >>>> host based unit testing on. Any chance you have authored that to
> >>>> confirm use cases are not unexpectedly impacted?  Example of host
> >>>> based unit test of library is here:
> >>>> edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master ·
> >>>> tianocore/edk2
> >>>>
> (github.com)<https://nam11.safelinks.protection.outlook.com/?url=https%3A%
> 2F%2Fgithub.com%2Ftianocore%2Fedk2%2Ftree%2Fmaster%2FSecurityPkg%2F
> Library%2FSecureBootVariableLib%2FUnitTest&data=05%7C01%7Cjbobek%40n
> vidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7d
> b39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000%7C%7C%7C&sdata=djptmpLUwESbPeqFwFUz5sVbS1MBnD%2FP
> ua9H2y1nxFE%3D&reserved=0>
> >>> I've done an ad-hoc test by commenting out the switch to/from Custom
> >>> mode in EnrollFromDefaultKeysApp from SecurityPkg, booting in Setup
> mode
> >>> and using the modified app to enroll the keys. You could do a similar
> >>> test from the OS, but in my case this was more straightforward.
> >>>
> >>> I am aware of the host-based unit testing library in edk2, and I agree
> >>> that it would be great to have the code in AuthVariableLib tested for
> >>> all the different cases. However, I don't have any such tests right now,
> >>> and while I'm willing to potentially look into writing some, I'd have to
> >>> do it more or less on the side, meaning it could take a while.
> >>>
> >>> Best,
> >>> -Jan
> >>>
> >>>> On 1/22/2023 10:13 PM, Yao, Jiewen wrote:
> >>>>
> >>>> Hi Sean
> >>>> I would like to hear your feedback, since it is a little different from the
> original MSFT patch.
> >>>>
> >>>> Would you please take a look?
> >>>>
> >>>> Thank you
> >>>> Yao, Jiewen
> >>>>
> >>>>
> >>>>
> >>>> -----Original Message-----
> >>>> From: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>
> >>>> Sent: Saturday, January 21, 2023 6:59 AM
> >>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> >>>> Cc: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>; Laszlo
> Ersek <lersek@redhat.com><mailto:lersek@redhat.com>; Yao,
> >>>> Jiewen <jiewen.yao@intel.com><mailto:jiewen.yao@intel.com>
> >>>> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
> >>>>
> >>>> Hi all,
> >>>>
> >>>> I'm sending out v1 of my patch series that addresses a UEFI spec
> >>>> non-compliance when enrolling PK in setup mode. Additional info can be
> >>>> found in bugzilla [1]; the changes are split into 4 patches as
> >>>> suggested by Laszlo Ersek in comment #4.
> >>>>
> >>>> I've based my work on the patch by Matthew Carlson; I've credited him
> >>>> with co-authorship of the first patch even though in the end I decided
> >>>> to do the implementation a bit differently.
> >>>>
> >>>> Comments & reviews welcome!
> >>>>
> >>>> Cheers,
> >>>> -Jan
> >>>>
> >>>> References:
> >>>> 1.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> tianocore.org%2Fshow_bug.cgi%3Fid%3D2506&data=05%7C01%7Cjbobek%40n
> vidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7d
> b39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000%7C%7C%7C&sdata=PXam5BVxMmUwgRdGsq1RcnwC8yb8nmO
> zcLz3oKica7s%3D&reserved=0
> >>>>
> >>>> Jan Bobek (4):
> >>>>    SecurityPkg: limit verification of enrolled PK in setup mode
> >>>>    OvmfPkg: require self-signed PK when secure boot is enabled
> >>>>    ArmVirtPkg: require self-signed PK when secure boot is enabled
> >>>>    SecurityPkg: don't require PK to be self-signed by default
> >>>>
> >>>> SecurityPkg/SecurityPkg.dec                             | 7 +++++++
> >>>> ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
> >>>> ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
> >>>> ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
> >>>> OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
> >>>> OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
> >>>> OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
> >>>> OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
> >>>> OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
> >>>> OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
> >>>> OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
> >>>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
> >>>> SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
> >>>> 13 files changed, 50 insertions(+), 2 deletions(-)