* [PATCH v1 0/4] Don't require self-signed PK in setup mode
@ 2023-01-20 22:58 Jan Bobek
2023-01-20 22:58 ` [PATCH v1 1/4] SecurityPkg: limit verification of enrolled " Jan Bobek
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Jan Bobek @ 2023-01-20 22:58 UTC (permalink / raw)
To: devel; +Cc: Jan Bobek, Laszlo Ersek, Jiewen Yao
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v1 1/4] SecurityPkg: limit verification of enrolled PK in setup mode
2023-01-20 22:58 [PATCH v1 0/4] Don't require self-signed PK in setup mode Jan Bobek
@ 2023-01-20 22:58 ` Jan Bobek
2023-01-20 22:58 ` [PATCH v1 2/4] OvmfPkg: require self-signed PK when secure boot is enabled Jan Bobek
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Jan Bobek @ 2023-01-20 22:58 UTC (permalink / raw)
To: devel
Cc: Jan Bobek, Laszlo Ersek, Jiewen Yao, Jian J Wang, Min Xu,
Matthew Carlson
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v1 2/4] OvmfPkg: require self-signed PK when secure boot is enabled
2023-01-20 22:58 [PATCH v1 0/4] Don't require self-signed PK in setup mode Jan Bobek
2023-01-20 22:58 ` [PATCH v1 1/4] SecurityPkg: limit verification of enrolled " Jan Bobek
@ 2023-01-20 22:58 ` Jan Bobek
2023-01-20 22:58 ` [PATCH v1 3/4] ArmVirtPkg: " Jan Bobek
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Jan Bobek @ 2023-01-20 22:58 UTC (permalink / raw)
To: devel
Cc: Jan Bobek, Laszlo Ersek, Jiewen Yao, Ard Biesheuvel,
Jordan Justen, Gerd Hoffmann, Rebecca Cran, Peter Grehan,
Sebastien Boeuf
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled
2023-01-20 22:58 [PATCH v1 0/4] Don't require self-signed PK in setup mode Jan Bobek
2023-01-20 22:58 ` [PATCH v1 1/4] SecurityPkg: limit verification of enrolled " Jan Bobek
2023-01-20 22:58 ` [PATCH v1 2/4] OvmfPkg: require self-signed PK when secure boot is enabled Jan Bobek
@ 2023-01-20 22:58 ` Jan Bobek
2023-02-03 0:11 ` Yao, Jiewen
2023-02-03 10:49 ` Ard Biesheuvel
2023-01-20 22:58 ` [PATCH v1 4/4] SecurityPkg: don't require PK to be self-signed by default Jan Bobek
2023-01-23 6:13 ` [PATCH v1 0/4] Don't require self-signed PK in setup mode Yao, Jiewen
4 siblings, 2 replies; 17+ messages in thread
From: Jan Bobek @ 2023-01-20 22:58 UTC (permalink / raw)
To: devel
Cc: Jan Bobek, Laszlo Ersek, Jiewen Yao, Ard Biesheuvel,
Leif Lindholm, Sami Mujawar, Gerd Hoffmann
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v1 4/4] SecurityPkg: don't require PK to be self-signed by default
2023-01-20 22:58 [PATCH v1 0/4] Don't require self-signed PK in setup mode Jan Bobek
` (2 preceding siblings ...)
2023-01-20 22:58 ` [PATCH v1 3/4] ArmVirtPkg: " Jan Bobek
@ 2023-01-20 22:58 ` Jan Bobek
2023-01-23 6:13 ` [PATCH v1 0/4] Don't require self-signed PK in setup mode Yao, Jiewen
4 siblings, 0 replies; 17+ messages in thread
From: Jan Bobek @ 2023-01-20 22:58 UTC (permalink / raw)
To: devel; +Cc: Jan Bobek, Laszlo Ersek, Jiewen Yao, Jian J Wang
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v1 0/4] Don't require self-signed PK in setup mode
2023-01-20 22:58 [PATCH v1 0/4] Don't require self-signed PK in setup mode Jan Bobek
` (3 preceding siblings ...)
2023-01-20 22:58 ` [PATCH v1 4/4] SecurityPkg: don't require PK to be self-signed by default Jan Bobek
@ 2023-01-23 6:13 ` Yao, Jiewen
2023-01-25 5:51 ` [edk2-devel] " Sean
4 siblings, 1 reply; 17+ messages in thread
From: Yao, Jiewen @ 2023-01-23 6:13 UTC (permalink / raw)
To: Jan Bobek, devel@edk2.groups.io, Sean Brogan; +Cc: Laszlo Ersek
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode
2023-01-23 6:13 ` [PATCH v1 0/4] Don't require self-signed PK in setup mode Yao, Jiewen
@ 2023-01-25 5:51 ` Sean
2023-01-25 21:38 ` Jan Bobek
0 siblings, 1 reply; 17+ messages in thread
From: Sean @ 2023-01-25 5:51 UTC (permalink / raw)
To: devel@edk2.groups.io, jiewen.yao@intel.com, Jan Bobek,
Sean Brogan
Cc: Laszlo Ersek
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode
2023-01-25 5:51 ` [edk2-devel] " Sean
@ 2023-01-25 21:38 ` Jan Bobek
2023-01-27 21:28 ` Sean
0 siblings, 1 reply; 17+ messages in thread
From: Jan Bobek @ 2023-01-25 21:38 UTC (permalink / raw)
To: Sean Brogan
Cc: devel@edk2.groups.io, jiewen.yao@intel.com, Sean Brogan,
Laszlo Ersek
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(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode
2023-01-25 21:38 ` Jan Bobek
@ 2023-01-27 21:28 ` Sean
2023-01-27 22:05 ` Jan Bobek
0 siblings, 1 reply; 17+ messages in thread
From: Sean @ 2023-01-27 21:28 UTC (permalink / raw)
To: Jan Bobek
Cc: devel@edk2.groups.io, jiewen.yao@intel.com, Sean Brogan,
Laszlo Ersek
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(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode
2023-01-27 21:28 ` Sean
@ 2023-01-27 22:05 ` Jan Bobek
2023-01-28 2:37 ` Sean
0 siblings, 1 reply; 17+ messages in thread
From: Jan Bobek @ 2023-01-27 22:05 UTC (permalink / raw)
To: Sean Brogan
Cc: devel@edk2.groups.io, jiewen.yao@intel.com, Sean Brogan,
Laszlo Ersek
> 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(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode
2023-01-27 22:05 ` Jan Bobek
@ 2023-01-28 2:37 ` Sean
2023-02-03 0:08 ` Yao, Jiewen
0 siblings, 1 reply; 17+ messages in thread
From: Sean @ 2023-01-28 2:37 UTC (permalink / raw)
To: Jan Bobek
Cc: devel@edk2.groups.io, jiewen.yao@intel.com, Sean Brogan,
Laszlo Ersek
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(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode
2023-01-28 2:37 ` Sean
@ 2023-02-03 0:08 ` Yao, Jiewen
0 siblings, 0 replies; 17+ messages in thread
From: Yao, Jiewen @ 2023-02-03 0:08 UTC (permalink / raw)
To: Sean Brogan, Jan Bobek; +Cc: devel@edk2.groups.io, Sean Brogan, Laszlo Ersek
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(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled
2023-01-20 22:58 ` [PATCH v1 3/4] ArmVirtPkg: " Jan Bobek
@ 2023-02-03 0:11 ` Yao, Jiewen
2023-02-03 10:49 ` Ard Biesheuvel
1 sibling, 0 replies; 17+ messages in thread
From: Yao, Jiewen @ 2023-02-03 0:11 UTC (permalink / raw)
To: Jan Bobek, devel@edk2.groups.io
Cc: Laszlo Ersek, Ard Biesheuvel, Leif Lindholm, Sami Mujawar,
Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled
2023-01-20 22:58 ` [PATCH v1 3/4] ArmVirtPkg: " Jan Bobek
2023-02-03 0:11 ` Yao, Jiewen
@ 2023-02-03 10:49 ` Ard Biesheuvel
2023-02-03 11:14 ` Yao, Jiewen
2023-02-03 11:39 ` Gerd Hoffmann
1 sibling, 2 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2023-02-03 10:49 UTC (permalink / raw)
To: Jan Bobek
Cc: devel, Laszlo Ersek, Jiewen Yao, Ard Biesheuvel, Leif Lindholm,
Sami Mujawar, Gerd Hoffmann
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled
2023-02-03 10:49 ` Ard Biesheuvel
@ 2023-02-03 11:14 ` Yao, Jiewen
2023-02-03 11:15 ` Ard Biesheuvel
2023-02-03 11:39 ` Gerd Hoffmann
1 sibling, 1 reply; 17+ messages in thread
From: Yao, Jiewen @ 2023-02-03 11:14 UTC (permalink / raw)
To: Ard Biesheuvel, Jan Bobek
Cc: devel@edk2.groups.io, Laszlo Ersek, Ard Biesheuvel, Leif Lindholm,
Sami Mujawar, Gerd Hoffmann
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
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled
2023-02-03 11:14 ` Yao, Jiewen
@ 2023-02-03 11:15 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2023-02-03 11:15 UTC (permalink / raw)
To: Yao, Jiewen
Cc: Jan Bobek, devel@edk2.groups.io, Laszlo Ersek, Ard Biesheuvel,
Leif Lindholm, Sami Mujawar, Gerd Hoffmann
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
> > >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 3/4] ArmVirtPkg: require self-signed PK when secure boot is enabled
2023-02-03 10:49 ` Ard Biesheuvel
2023-02-03 11:14 ` Yao, Jiewen
@ 2023-02-03 11:39 ` Gerd Hoffmann
1 sibling, 0 replies; 17+ messages in thread
From: Gerd Hoffmann @ 2023-02-03 11:39 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jan Bobek, devel, Laszlo Ersek, Jiewen Yao, Ard Biesheuvel,
Leif Lindholm, Sami Mujawar
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-02-03 11:39 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-20 22:58 [PATCH v1 0/4] Don't require self-signed PK in setup mode Jan Bobek
2023-01-20 22:58 ` [PATCH v1 1/4] SecurityPkg: limit verification of enrolled " Jan Bobek
2023-01-20 22:58 ` [PATCH v1 2/4] OvmfPkg: require self-signed PK when secure boot is enabled Jan Bobek
2023-01-20 22:58 ` [PATCH v1 3/4] ArmVirtPkg: " Jan Bobek
2023-02-03 0:11 ` Yao, Jiewen
2023-02-03 10:49 ` Ard Biesheuvel
2023-02-03 11:14 ` Yao, Jiewen
2023-02-03 11:15 ` Ard Biesheuvel
2023-02-03 11:39 ` Gerd Hoffmann
2023-01-20 22:58 ` [PATCH v1 4/4] SecurityPkg: don't require PK to be self-signed by default Jan Bobek
2023-01-23 6:13 ` [PATCH v1 0/4] Don't require self-signed PK in setup mode Yao, Jiewen
2023-01-25 5:51 ` [edk2-devel] " Sean
2023-01-25 21:38 ` Jan Bobek
2023-01-27 21:28 ` Sean
2023-01-27 22:05 ` Jan Bobek
2023-01-28 2:37 ` Sean
2023-02-03 0:08 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox