public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries
       [not found] <16F848B3160A57C1.5246@groups.io>
@ 2022-06-28 21:18 ` Kun Qin
  2022-06-29  8:50   ` Yao, Jiewen
  0 siblings, 1 reply; 6+ messages in thread
From: Kun Qin @ 2022-06-28 21:18 UTC (permalink / raw)
  To: edk2-devel-groups-io, kuqin12
  Cc: Jiewen Yao, Jian J Wang, Min Xu, Sean Brogan, Ard Biesheuvel,
	Jordan Justen, Gerd Hoffmann, Rebecca Cran, Peter Grehan,
	Sebastien Boeuf, Andrew Fish, Ray Ni

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

Hi SecurityPkg maintainers & reviewers,

I posted this patch series a while back intending to generalize the usage
of a few interfaces from secure boot libraries. Could you please help
reviewing them and provide feedback? Any input is appreciated.

Regards,
Kun

On Mon, Jun 13, 2022 at 1:39 PM Kun Qin via groups.io <kuqin12=
gmail.com@groups.io> wrote:

> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3909
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3910
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3911
>
> This is a revamp of a previously submitted patch series based on top of
> master branch: https://edk2.groups.io/g/devel/message/89507. No changes
> added.
>
> Current SecureBootVariableLib provide great support for deleting secure
> boot related variables, creating time-based payloads.
>
> However, for secure boot enrollment, the SecureBootVariableProvisionLib
> interfaces always assume the changes from variable storage, limiting the
> usage, requiring existing platforms to change key initialization process
> to adapt to the new methods, as well as bringing in extra dependencies
> such as FV protocol, time protocols.
>
> This patch series proposes to update the implementation for Secure Boot
> Variable libraries and their consumers to better support the related
> variables operations.
>
> Patch v2 branch:
> https://github.com/kuqin12/edk2/tree/secure_boot_enhance_v2
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> 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>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Ray Ni <ray.ni@intel.com>
>
> Kun Qin (8):
>   SecurityPkg: UefiSecureBoot: Definitions of cert and payload
>     structures
>   SecurityPkg: PlatformPKProtectionLib: Added PK protection interface
>   SecurityPkg: SecureBootVariableLib: Updated time based payload creator
>   SecurityPkg: SecureBootVariableProvisionLib: Updated implementation
>   SecurityPkg: Secure Boot Drivers: Added common header files
>   SecurityPkg: SecureBootConfigDxe: Updated invocation pattern
>   OvmfPkg: Pipeline: Resolve SecureBootVariableLib dependency
>   EmulatorPkg: Pipeline: Resolve SecureBootVariableLib dependency
>
> kuqin (3):
>   SecurityPkg: SecureBootVariableLib: Updated signature list creator
>   SecurityPkg: SecureBootVariableLib: Added newly supported interfaces
>   SecurityPkg: SecureBootVariableLib: Added unit tests
>
>  SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c
>                  |    1 +
>  SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
>  |   51 +
>  SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
>                  |  486 ++++-
>  SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
>         |   36 +
>  SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
>                 |  201 ++
>  SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
>     |   13 +
>  SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
>       | 2037 ++++++++++++++++++++
>  SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.c
>      |  145 +-
>  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
>             |  128 +-
>  SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c
>    |    1 +
>  EmulatorPkg/EmulatorPkg.dsc
>                  |    1 +
>  OvmfPkg/Bhyve/BhyveX64.dsc
>                 |    1 +
>  OvmfPkg/CloudHv/CloudHvX64.dsc
>                 |    1 +
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc
>                 |    1 +
>  OvmfPkg/OvmfPkgIa32.dsc
>                  |    1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc
>                 |    1 +
>  OvmfPkg/OvmfPkgX64.dsc
>                 |    1 +
>  SecurityPkg/Include/Library/PlatformPKProtectionLib.h
>                  |   31 +
>  SecurityPkg/Include/Library/SecureBootVariableLib.h
>                  |  103 +-
>  SecurityPkg/Include/UefiSecureBoot.h
>                 |   94 +
>  SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
> |   36 +
>  SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf
>                  |   14 +-
>  SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
>       |   33 +
>  SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
>                 |   45 +
>  SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
>   |   25 +
>  SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
>     |   36 +
>  SecurityPkg/SecurityPkg.ci.yaml
>                  |   11 +
>  SecurityPkg/SecurityPkg.dec
>                  |    5 +
>  SecurityPkg/SecurityPkg.dsc
>                  |    2 +
>  SecurityPkg/Test/SecurityPkgHostTest.dsc
>                 |   38 +
>  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>            |    1 +
>  31 files changed, 3468 insertions(+), 112 deletions(-)
>  create mode 100644
> SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
>  create mode 100644
> SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
>  create mode 100644
> SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
>  create mode 100644
> SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
>  create mode 100644
> SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
>  create mode 100644 SecurityPkg/Include/Library/PlatformPKProtectionLib.h
>  create mode 100644 SecurityPkg/Include/UefiSecureBoot.h
>  create mode 100644
> SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
>  create mode 100644
> SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
>  create mode 100644
> SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
>  create mode 100644
> SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
>  create mode 100644
> SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
>  create mode 100644 SecurityPkg/Test/SecurityPkgHostTest.dsc
>
> --
> 2.35.1.windows.2
>
>
>
> 
>
>
>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries
  2022-06-28 21:18 ` [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries Kun Qin
@ 2022-06-29  8:50   ` Yao, Jiewen
  2022-06-29 18:06     ` Kun Qin
  0 siblings, 1 reply; 6+ messages in thread
From: Yao, Jiewen @ 2022-06-29  8:50 UTC (permalink / raw)
  To: Kun Qin, edk2-devel-groups-io
  Cc: Wang, Jian J, Xu, Min M, Sean Brogan, Ard Biesheuvel,
	Justen, Jordan L, Gerd Hoffmann, Rebecca Cran, Peter Grehan,
	Boeuf, Sebastien, Andrew Fish, Ni, Ray

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

Hi Kun
Thank you to make the redesign.

Overall the patch set looks good to me. Some questions:


  1.  Is that from project MU? If so, I would like to see acked-by or tested-by from project MU owner. That can give me more confidence to accept it. 😊


  1.  Is below data from some document? If so, would please add URL? Also, why do we have to use this timestamp? What if a different timestamp is used?

+// MS Default Time-Based Payload Creation Date
+// This is the date that is used when creating SecureBoot default variables.
+// NOTE: This is a placeholder date that doesn't correspond to anything else.
+//
+EFI_TIME  mDefaultPayloadTimestamp = {
+  15,   // Year (2015)
+  8,    // Month (Aug)
+  28,   // Day (28)
+  0,    // Hour
+  0,    // Minute
+  0,    // Second
+  0,    // Pad1
+  0,    // Nanosecond
+  0,    // Timezone (Dummy value)
+  0,    // Daylight (Dummy value)
+  0     // Pad2
+};


From: Kun Qin <kuqin12@gmail.com>
Sent: Wednesday, June 29, 2022 5:19 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; kuqin12@gmail.com
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Rebecca Cran <rebecca@bsdio.com>; Peter Grehan <grehan@freebsd.org>; Boeuf, Sebastien <sebastien.boeuf@intel.com>; Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries

Hi SecurityPkg maintainers & reviewers,

I posted this patch series a while back intending to generalize the usage of a few interfaces from secure boot libraries. Could you please help reviewing them and provide feedback? Any input is appreciated.

Regards,
Kun

On Mon, Jun 13, 2022 at 1:39 PM Kun Qin via groups.io<http://groups.io> <kuqin12=gmail.com@groups.io<mailto:gmail.com@groups.io>> wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3909
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3910
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3911

This is a revamp of a previously submitted patch series based on top of
master branch: https://edk2.groups.io/g/devel/message/89507. No changes
added.

Current SecureBootVariableLib provide great support for deleting secure
boot related variables, creating time-based payloads.

However, for secure boot enrollment, the SecureBootVariableProvisionLib
interfaces always assume the changes from variable storage, limiting the
usage, requiring existing platforms to change key initialization process
to adapt to the new methods, as well as bringing in extra dependencies
such as FV protocol, time protocols.

This patch series proposes to update the implementation for Secure Boot
Variable libraries and their consumers to better support the related
variables operations.

Patch v2 branch: https://github.com/kuqin12/edk2/tree/secure_boot_enhance_v2

Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Cc: Min Xu <min.m.xu@intel.com<mailto:min.m.xu@intel.com>>
Cc: Sean Brogan <sean.brogan@microsoft.com<mailto:sean.brogan@microsoft.com>>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb%2Btianocore@kernel.org>>
Cc: Jordan Justen <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>
Cc: Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>
Cc: Rebecca Cran <rebecca@bsdio.com<mailto:rebecca@bsdio.com>>
Cc: Peter Grehan <grehan@freebsd.org<mailto:grehan@freebsd.org>>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com<mailto:sebastien.boeuf@intel.com>>
Cc: Andrew Fish <afish@apple.com<mailto:afish@apple.com>>
Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>

Kun Qin (8):
  SecurityPkg: UefiSecureBoot: Definitions of cert and payload
    structures
  SecurityPkg: PlatformPKProtectionLib: Added PK protection interface
  SecurityPkg: SecureBootVariableLib: Updated time based payload creator
  SecurityPkg: SecureBootVariableProvisionLib: Updated implementation
  SecurityPkg: Secure Boot Drivers: Added common header files
  SecurityPkg: SecureBootConfigDxe: Updated invocation pattern
  OvmfPkg: Pipeline: Resolve SecureBootVariableLib dependency
  EmulatorPkg: Pipeline: Resolve SecureBootVariableLib dependency

kuqin (3):
  SecurityPkg: SecureBootVariableLib: Updated signature list creator
  SecurityPkg: SecureBootVariableLib: Added newly supported interfaces
  SecurityPkg: SecureBootVariableLib: Added unit tests

 SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c                           |    1 +
 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c   |   51 +
 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c                         |  486 ++++-
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c          |   36 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c                          |  201 ++
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c      |   13 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c        | 2037 ++++++++++++++++++++
 SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.c       |  145 +-
 SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c              |  128 +-
 SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c     |    1 +
 EmulatorPkg/EmulatorPkg.dsc                                                               |    1 +
 OvmfPkg/Bhyve/BhyveX64.dsc                                                                |    1 +
 OvmfPkg/CloudHv/CloudHvX64.dsc                                                            |    1 +
 OvmfPkg/IntelTdx/IntelTdxX64.dsc                                                          |    1 +
 OvmfPkg/OvmfPkgIa32.dsc                                                                   |    1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                                                                |    1 +
 OvmfPkg/OvmfPkgX64.dsc                                                                    |    1 +
 SecurityPkg/Include/Library/PlatformPKProtectionLib.h                                     |   31 +
 SecurityPkg/Include/Library/SecureBootVariableLib.h                                       |  103 +-
 SecurityPkg/Include/UefiSecureBoot.h                                                      |   94 +
 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf |   36 +
 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf                       |   14 +-
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf        |   33 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf                        |   45 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf    |   25 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf      |   36 +
 SecurityPkg/SecurityPkg.ci.yaml                                                           |   11 +
 SecurityPkg/SecurityPkg.dec                                                               |    5 +
 SecurityPkg/SecurityPkg.dsc                                                               |    2 +
 SecurityPkg/Test/SecurityPkgHostTest.dsc                                                  |   38 +
 SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf             |    1 +
 31 files changed, 3468 insertions(+), 112 deletions(-)
 create mode 100644 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
 create mode 100644 SecurityPkg/Include/Library/PlatformPKProtectionLib.h
 create mode 100644 SecurityPkg/Include/UefiSecureBoot.h
 create mode 100644 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
 create mode 100644 SecurityPkg/Test/SecurityPkgHostTest.dsc

--
2.35.1.windows.2






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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries
  2022-06-29  8:50   ` Yao, Jiewen
@ 2022-06-29 18:06     ` Kun Qin
  2022-06-30  0:19       ` Yao, Jiewen
  0 siblings, 1 reply; 6+ messages in thread
From: Kun Qin @ 2022-06-29 18:06 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: Wang, Jian J, Xu, Min M, Sean Brogan, Ard Biesheuvel,
	Justen, Jordan L, Gerd Hoffmann, Rebecca Cran, Peter Grehan,
	Boeuf, Sebastien, Andrew Fish, Ni, Ray

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

Hi Jiewen,

Thanks for reading through these patches.

For #1, yes, we implemented these changes in project MU and validated 
them on both our virtual platform 
(https://github.com/microsoft/mu_tiano_platforms) and other proprietary 
hardware platforms. I will leave the acked-by or tested-by to others on 
the MU teams.

For #2, this is just an arbitrary timestamp from a previous date so that 
we can create time based payload without hard dependency on time 
protocol (which could result in potential executing sequence issue). I 
will update comment to avoid confusion in the next round of patch. But 
should you have other suggestions to improve this, please let me know.

Regards,
Kun

On 6/29/2022 1:50 AM, Yao, Jiewen wrote:
>
> Hi Kun
>
> Thank you to make the redesign.
>
> Overall the patch set looks good to me. Some questions:
>
>  1. Is that from project MU? If so, I would like to see acked-by or
>     tested-by from project MU owner. That can give me more confidence
>     to accept it. 😊
>
>  2. Is below data from some document? If so, would please add URL?
>     Also, why do we have to use this timestamp? What if a different
>     timestamp is used?
>
> +// MS Default Time-Based Payload Creation Date
>
> +// This is the date that is used when creating SecureBoot default 
> variables.
>
> +// NOTE: This is a placeholder date that doesn't correspond to 
> anything else.
>
> +//
>
> +EFI_TIME  mDefaultPayloadTimestamp = {
>
> +  15,   // Year (2015)
>
> +  8,    // Month (Aug)
>
> +  28,   // Day (28)
>
> +  0,    // Hour
>
> +  0,    // Minute
>
> +  0,    // Second
>
> +  0,    // Pad1
>
> +  0,    // Nanosecond
>
> +  0,    // Timezone (Dummy value)
>
> +  0,    // Daylight (Dummy value)
>
> +  0     // Pad2
>
> +};
>
> *From:* Kun Qin <kuqin12@gmail.com>
> *Sent:* Wednesday, June 29, 2022 5:19 AM
> *To:* edk2-devel-groups-io <devel@edk2.groups.io>; kuqin12@gmail.com
> *Cc:* Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J 
> <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>; Sean Brogan 
> <sean.brogan@microsoft.com>; Ard Biesheuvel 
> <ardb+tianocore@kernel.org>; Justen, Jordan L 
> <jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; 
> Rebecca Cran <rebecca@bsdio.com>; Peter Grehan <grehan@freebsd.org>; 
> Boeuf, Sebastien <sebastien.boeuf@intel.com>; Andrew Fish 
> <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
> *Subject:* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot 
> Variable Libraries
>
> Hi SecurityPkg maintainers & reviewers,
>
> I posted this patch series a while back intending to generalize the 
> usage of a few interfaces from secure boot libraries. Could you please 
> help reviewing them and provide feedback? Any input is appreciated.
>
> Regards,
>
> Kun
>
> On Mon, Jun 13, 2022 at 1:39 PM Kun Qin via groups.io 
> <http://groups.io> <kuqin12=gmail.com@groups.io> wrote:
>
>     REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3909
>     REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3910
>     REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3911
>
>     This is a revamp of a previously submitted patch series based on
>     top of
>     master branch: https://edk2.groups.io/g/devel/message/89507. No
>     changes
>     added.
>
>     Current SecureBootVariableLib provide great support for deleting
>     secure
>     boot related variables, creating time-based payloads.
>
>     However, for secure boot enrollment, the
>     SecureBootVariableProvisionLib
>     interfaces always assume the changes from variable storage,
>     limiting the
>     usage, requiring existing platforms to change key initialization
>     process
>     to adapt to the new methods, as well as bringing in extra dependencies
>     such as FV protocol, time protocols.
>
>     This patch series proposes to update the implementation for Secure
>     Boot
>     Variable libraries and their consumers to better support the related
>     variables operations.
>
>     Patch v2 branch:
>     https://github.com/kuqin12/edk2/tree/secure_boot_enhance_v2
>
>     Cc: Jiewen Yao <jiewen.yao@intel.com>
>     Cc: Jian J Wang <jian.j.wang@intel.com>
>     Cc: Min Xu <min.m.xu@intel.com>
>     Cc: Sean Brogan <sean.brogan@microsoft.com>
>     Cc: Ard Biesheuvel <ardb+tianocore@kernel.org
>     <mailto:ardb%2Btianocore@kernel.org>>
>     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>
>     Cc: Andrew Fish <afish@apple.com>
>     Cc: Ray Ni <ray.ni@intel.com>
>
>     Kun Qin (8):
>       SecurityPkg: UefiSecureBoot: Definitions of cert and payload
>         structures
>       SecurityPkg: PlatformPKProtectionLib: Added PK protection interface
>       SecurityPkg: SecureBootVariableLib: Updated time based payload
>     creator
>       SecurityPkg: SecureBootVariableProvisionLib: Updated implementation
>       SecurityPkg: Secure Boot Drivers: Added common header files
>       SecurityPkg: SecureBootConfigDxe: Updated invocation pattern
>       OvmfPkg: Pipeline: Resolve SecureBootVariableLib dependency
>       EmulatorPkg: Pipeline: Resolve SecureBootVariableLib dependency
>
>     kuqin (3):
>       SecurityPkg: SecureBootVariableLib: Updated signature list creator
>       SecurityPkg: SecureBootVariableLib: Added newly supported interfaces
>       SecurityPkg: SecureBootVariableLib: Added unit tests
>
>      SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c  
>                      |    1 +
>      SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
>      |   51 +
>      SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
>                        |  486 ++++-
>      SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
>             |   36 +
>      SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
>                       |  201 ++
>      SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
>         |   13 +
>      SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
>           | 2037 ++++++++++++++++++++
>      SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.c
>          |  145 +-
>      SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
>                 |  128 +-
>      SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c
>        |    1 +
>      EmulatorPkg/EmulatorPkg.dsc                                    
>      |    1 +
>      OvmfPkg/Bhyve/BhyveX64.dsc                                     | 
>       1 +
>      OvmfPkg/CloudHv/CloudHvX64.dsc                                  
>       |    1 +
>      OvmfPkg/IntelTdx/IntelTdxX64.dsc                                
>         |    1 +
>      OvmfPkg/OvmfPkgIa32.dsc                                      |    1 +
>      OvmfPkg/OvmfPkgIa32X64.dsc                                     | 
>       1 +
>      OvmfPkg/OvmfPkgX64.dsc                                     |    1 +
>      SecurityPkg/Include/Library/PlatformPKProtectionLib.h            
>            |   31 +
>      SecurityPkg/Include/Library/SecureBootVariableLib.h              
>                            |  103 +-
>      SecurityPkg/Include/UefiSecureBoot.h                            
>             |   94 +
>      SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
>     |   36 +
>      SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf
>                        |   14 +-
>      SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
>           |   33 +
>      SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
>                       |   45 +
>      SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
>       |   25 +
>      SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
>         |   36 +
>      SecurityPkg/SecurityPkg.ci.yaml                                  
>        |   11 +
>      SecurityPkg/SecurityPkg.dec                                    
>      |    5 +
>      SecurityPkg/SecurityPkg.dsc                                    
>      |    2 +
>      SecurityPkg/Test/SecurityPkgHostTest.dsc                        
>                 |   38 +
>      SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>                |    1 +
>      31 files changed, 3468 insertions(+), 112 deletions(-)
>      create mode 100644
>     SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
>      create mode 100644
>     SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
>      create mode 100644
>     SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
>      create mode 100644
>     SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
>      create mode 100644
>     SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
>      create mode 100644
>     SecurityPkg/Include/Library/PlatformPKProtectionLib.h
>      create mode 100644 SecurityPkg/Include/UefiSecureBoot.h
>      create mode 100644
>     SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
>      create mode 100644
>     SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
>      create mode 100644
>     SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
>      create mode 100644
>     SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
>      create mode 100644
>     SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
>      create mode 100644 SecurityPkg/Test/SecurityPkgHostTest.dsc
>
>     -- 
>     2.35.1.windows.2
>
>
>
>
>
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries
  2022-06-29 18:06     ` Kun Qin
@ 2022-06-30  0:19       ` Yao, Jiewen
  2022-06-30  0:23         ` Kun Qin
  0 siblings, 1 reply; 6+ messages in thread
From: Yao, Jiewen @ 2022-06-30  0:19 UTC (permalink / raw)
  To: devel@edk2.groups.io, kuqin12@gmail.com
  Cc: Wang, Jian J, Xu, Min M, Sean Brogan, Ard Biesheuvel,
	Justen, Jordan L, Gerd Hoffmann, Rebecca Cran, Peter Grehan,
	Boeuf, Sebastien, Andrew Fish, Ni, Ray

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

Sounds great.

1) I assume that if it is accepted by project MU, then it must be reviewed and tested. Please add required tag in next patch set.

2) I suggest just use one of: a) all zero, b) initial data Jan/1/1970, c) current data.

With above change, reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

Thank you
Yao, Jiewen


From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
Sent: Thursday, June 30, 2022 2:07 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Rebecca Cran <rebecca@bsdio.com>; Peter Grehan <grehan@freebsd.org>; Boeuf, Sebastien <sebastien.boeuf@intel.com>; Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries


Hi Jiewen,

Thanks for reading through these patches.

For #1, yes, we implemented these changes in project MU and validated them on both our virtual platform (https://github.com/microsoft/mu_tiano_platforms) and other proprietary hardware platforms. I will leave the acked-by or tested-by to others on the MU teams.

For #2, this is just an arbitrary timestamp from a previous date so that we can create time based payload without hard dependency on time protocol (which could result in potential executing sequence issue). I will update comment to avoid confusion in the next round of patch. But should you have other suggestions to improve this, please let me know.

Regards,
Kun
On 6/29/2022 1:50 AM, Yao, Jiewen wrote:
Hi Kun
Thank you to make the redesign.

Overall the patch set looks good to me. Some questions:


  1.  Is that from project MU? If so, I would like to see acked-by or tested-by from project MU owner. That can give me more confidence to accept it. 😊


  1.  Is below data from some document? If so, would please add URL? Also, why do we have to use this timestamp? What if a different timestamp is used?

+// MS Default Time-Based Payload Creation Date
+// This is the date that is used when creating SecureBoot default variables.
+// NOTE: This is a placeholder date that doesn't correspond to anything else.
+//
+EFI_TIME  mDefaultPayloadTimestamp = {
+  15,   // Year (2015)
+  8,    // Month (Aug)
+  28,   // Day (28)
+  0,    // Hour
+  0,    // Minute
+  0,    // Second
+  0,    // Pad1
+  0,    // Nanosecond
+  0,    // Timezone (Dummy value)
+  0,    // Daylight (Dummy value)
+  0     // Pad2
+};


From: Kun Qin <kuqin12@gmail.com><mailto:kuqin12@gmail.com>
Sent: Wednesday, June 29, 2022 5:19 AM
To: edk2-devel-groups-io <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; kuqin12@gmail.com<mailto:kuqin12@gmail.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com><mailto:jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com><mailto:jian.j.wang@intel.com>; Xu, Min M <min.m.xu@intel.com><mailto:min.m.xu@intel.com>; Sean Brogan <sean.brogan@microsoft.com><mailto:sean.brogan@microsoft.com>; Ard Biesheuvel <ardb+tianocore@kernel.org><mailto:ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com><mailto:jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.com><mailto:kraxel@redhat.com>; Rebecca Cran <rebecca@bsdio.com><mailto:rebecca@bsdio.com>; Peter Grehan <grehan@freebsd.org><mailto:grehan@freebsd.org>; Boeuf, Sebastien <sebastien.boeuf@intel.com><mailto:sebastien.boeuf@intel.com>; Andrew Fish <afish@apple.com><mailto:afish@apple.com>; Ni, Ray <ray.ni@intel.com><mailto:ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries

Hi SecurityPkg maintainers & reviewers,

I posted this patch series a while back intending to generalize the usage of a few interfaces from secure boot libraries. Could you please help reviewing them and provide feedback? Any input is appreciated.

Regards,
Kun

On Mon, Jun 13, 2022 at 1:39 PM Kun Qin via groups.io<http://groups.io> <kuqin12=gmail.com@groups.io<mailto:gmail.com@groups.io>> wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3909
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3910
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3911

This is a revamp of a previously submitted patch series based on top of
master branch: https://edk2.groups.io/g/devel/message/89507. No changes
added.

Current SecureBootVariableLib provide great support for deleting secure
boot related variables, creating time-based payloads.

However, for secure boot enrollment, the SecureBootVariableProvisionLib
interfaces always assume the changes from variable storage, limiting the
usage, requiring existing platforms to change key initialization process
to adapt to the new methods, as well as bringing in extra dependencies
such as FV protocol, time protocols.

This patch series proposes to update the implementation for Secure Boot
Variable libraries and their consumers to better support the related
variables operations.

Patch v2 branch: https://github.com/kuqin12/edk2/tree/secure_boot_enhance_v2

Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Cc: Min Xu <min.m.xu@intel.com<mailto:min.m.xu@intel.com>>
Cc: Sean Brogan <sean.brogan@microsoft.com<mailto:sean.brogan@microsoft.com>>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org<mailto:ardb%2Btianocore@kernel.org>>
Cc: Jordan Justen <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>
Cc: Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>
Cc: Rebecca Cran <rebecca@bsdio.com<mailto:rebecca@bsdio.com>>
Cc: Peter Grehan <grehan@freebsd.org<mailto:grehan@freebsd.org>>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com<mailto:sebastien.boeuf@intel.com>>
Cc: Andrew Fish <afish@apple.com<mailto:afish@apple.com>>
Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>

Kun Qin (8):
  SecurityPkg: UefiSecureBoot: Definitions of cert and payload
    structures
  SecurityPkg: PlatformPKProtectionLib: Added PK protection interface
  SecurityPkg: SecureBootVariableLib: Updated time based payload creator
  SecurityPkg: SecureBootVariableProvisionLib: Updated implementation
  SecurityPkg: Secure Boot Drivers: Added common header files
  SecurityPkg: SecureBootConfigDxe: Updated invocation pattern
  OvmfPkg: Pipeline: Resolve SecureBootVariableLib dependency
  EmulatorPkg: Pipeline: Resolve SecureBootVariableLib dependency

kuqin (3):
  SecurityPkg: SecureBootVariableLib: Updated signature list creator
  SecurityPkg: SecureBootVariableLib: Added newly supported interfaces
  SecurityPkg: SecureBootVariableLib: Added unit tests

 SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c                           |    1 +
 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c   |   51 +
 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c                         |  486 ++++-
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c          |   36 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c                          |  201 ++
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c      |   13 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c        | 2037 ++++++++++++++++++++
 SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.c       |  145 +-
 SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c              |  128 +-
 SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c     |    1 +
 EmulatorPkg/EmulatorPkg.dsc                                                               |    1 +
 OvmfPkg/Bhyve/BhyveX64.dsc                                                                |    1 +
 OvmfPkg/CloudHv/CloudHvX64.dsc                                                            |    1 +
 OvmfPkg/IntelTdx/IntelTdxX64.dsc                                                          |    1 +
 OvmfPkg/OvmfPkgIa32.dsc                                                                   |    1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                                                                |    1 +
 OvmfPkg/OvmfPkgX64.dsc                                                                    |    1 +
 SecurityPkg/Include/Library/PlatformPKProtectionLib.h                                     |   31 +
 SecurityPkg/Include/Library/SecureBootVariableLib.h                                       |  103 +-
 SecurityPkg/Include/UefiSecureBoot.h                                                      |   94 +
 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf |   36 +
 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf                       |   14 +-
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf        |   33 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf                        |   45 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf    |   25 +
 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf      |   36 +
 SecurityPkg/SecurityPkg.ci.yaml                                                           |   11 +
 SecurityPkg/SecurityPkg.dec                                                               |    5 +
 SecurityPkg/SecurityPkg.dsc                                                               |    2 +
 SecurityPkg/Test/SecurityPkgHostTest.dsc                                                  |   38 +
 SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf             |    1 +
 31 files changed, 3468 insertions(+), 112 deletions(-)
 create mode 100644 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
 create mode 100644 SecurityPkg/Include/Library/PlatformPKProtectionLib.h
 create mode 100644 SecurityPkg/Include/UefiSecureBoot.h
 create mode 100644 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
 create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
 create mode 100644 SecurityPkg/Test/SecurityPkgHostTest.dsc

--
2.35.1.windows.2








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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries
  2022-06-30  0:19       ` Yao, Jiewen
@ 2022-06-30  0:23         ` Kun Qin
  0 siblings, 0 replies; 6+ messages in thread
From: Kun Qin @ 2022-06-30  0:23 UTC (permalink / raw)
  To: devel, jiewen.yao
  Cc: Wang, Jian J, Xu, Min M, Sean Brogan, Ard Biesheuvel,
	Justen, Jordan L, Gerd Hoffmann, Rebecca Cran, Peter Grehan,
	Boeuf, Sebastien, Andrew Fish, Ni, Ray

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

Thanks, Jiewen. I will incorporate the change you suggested below and 
re-validate on the platforms we have, while I wait for the tags from MU 
repo owners.

Regards,
Kun

On 6/29/2022 5:19 PM, Yao, Jiewen wrote:
>
> Sounds great.
>
> 1) I assume that if it is accepted by project MU, then it must be 
> reviewed and tested. Please add required tag in next patch set.
>
> 2) I suggest just use one of: a) all zero, b) initial data Jan/1/1970, 
> c) current data.
>
> With above change, reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
>
> Thank you
>
> Yao, Jiewen
>
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of *Kun Qin
> *Sent:* Thursday, June 30, 2022 2:07 AM
> *To:* devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> *Cc:* Wang, Jian J <jian.j.wang@intel.com>; Xu, Min M 
> <min.m.xu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Ard 
> Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L 
> <jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; 
> Rebecca Cran <rebecca@bsdio.com>; Peter Grehan <grehan@freebsd.org>; 
> Boeuf, Sebastien <sebastien.boeuf@intel.com>; Andrew Fish 
> <afish@apple.com>; Ni, Ray <ray.ni@intel.com>
> *Subject:* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot 
> Variable Libraries
>
> Hi Jiewen,
>
> Thanks for reading through these patches.
>
> For #1, yes, we implemented these changes in project MU and validated 
> them on both our virtual platform 
> (https://github.com/microsoft/mu_tiano_platforms) and other 
> proprietary hardware platforms. I will leave the acked-by or tested-by 
> to others on the MU teams.
>
> For #2, this is just an arbitrary timestamp from a previous date so 
> that we can create time based payload without hard dependency on time 
> protocol (which could result in potential executing sequence issue). I 
> will update comment to avoid confusion in the next round of patch. But 
> should you have other suggestions to improve this, please let me know.
>
> Regards,
> Kun
>
> On 6/29/2022 1:50 AM, Yao, Jiewen wrote:
>
>     Hi Kun
>
>     Thank you to make the redesign.
>
>     Overall the patch set looks good to me. Some questions:
>
>      1. Is that from project MU? If so, I would like to see acked-by
>         or tested-by from project MU owner. That can give me more
>         confidence to accept it. 😊
>
>      2. Is below data from some document? If so, would please add URL?
>         Also, why do we have to use this timestamp? What if a
>         different timestamp is used?
>
>     +// MS Default Time-Based Payload Creation Date
>
>     +// This is the date that is used when creating SecureBoot default
>     variables.
>
>     +// NOTE: This is a placeholder date that doesn't correspond to
>     anything else.
>
>     +//
>
>     +EFI_TIME  mDefaultPayloadTimestamp = {
>
>     +  15,   // Year (2015)
>
>     +  8,    // Month (Aug)
>
>     +  28,   // Day (28)
>
>     +  0,    // Hour
>
>     +  0,    // Minute
>
>     +  0,    // Second
>
>     +  0,    // Pad1
>
>     +  0,    // Nanosecond
>
>     +  0,    // Timezone (Dummy value)
>
>     +  0,    // Daylight (Dummy value)
>
>     +  0     // Pad2
>
>     +};
>
>     *From:* Kun Qin <kuqin12@gmail.com> <mailto:kuqin12@gmail.com>
>     *Sent:* Wednesday, June 29, 2022 5:19 AM
>     *To:* edk2-devel-groups-io <devel@edk2.groups.io>
>     <mailto:devel@edk2.groups.io>; kuqin12@gmail.com
>     *Cc:* Yao, Jiewen <jiewen.yao@intel.com>
>     <mailto:jiewen.yao@intel.com>; Wang, Jian J
>     <jian.j.wang@intel.com> <mailto:jian.j.wang@intel.com>; Xu, Min M
>     <min.m.xu@intel.com> <mailto:min.m.xu@intel.com>; Sean Brogan
>     <sean.brogan@microsoft.com> <mailto:sean.brogan@microsoft.com>;
>     Ard Biesheuvel <ardb+tianocore@kernel.org>
>     <mailto:ardb+tianocore@kernel.org>; Justen, Jordan L
>     <jordan.l.justen@intel.com> <mailto:jordan.l.justen@intel.com>;
>     Gerd Hoffmann <kraxel@redhat.com> <mailto:kraxel@redhat.com>;
>     Rebecca Cran <rebecca@bsdio.com> <mailto:rebecca@bsdio.com>; Peter
>     Grehan <grehan@freebsd.org> <mailto:grehan@freebsd.org>; Boeuf,
>     Sebastien <sebastien.boeuf@intel.com>
>     <mailto:sebastien.boeuf@intel.com>; Andrew Fish <afish@apple.com>
>     <mailto:afish@apple.com>; Ni, Ray <ray.ni@intel.com>
>     <mailto:ray.ni@intel.com>
>     *Subject:* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot
>     Variable Libraries
>
>     Hi SecurityPkg maintainers & reviewers,
>
>     I posted this patch series a while back intending to generalize
>     the usage of a few interfaces from secure boot libraries. Could
>     you please help reviewing them and provide feedback? Any input is
>     appreciated.
>
>     Regards,
>
>     Kun
>
>     On Mon, Jun 13, 2022 at 1:39 PM Kun Qin via groups.io
>     <http://groups.io> <kuqin12=gmail.com@groups.io> wrote:
>
>         REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3909
>         REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3910
>         REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3911
>
>         This is a revamp of a previously submitted patch series based
>         on top of
>         master branch: https://edk2.groups.io/g/devel/message/89507.
>         No changes
>         added.
>
>         Current SecureBootVariableLib provide great support for
>         deleting secure
>         boot related variables, creating time-based payloads.
>
>         However, for secure boot enrollment, the
>         SecureBootVariableProvisionLib
>         interfaces always assume the changes from variable storage,
>         limiting the
>         usage, requiring existing platforms to change key
>         initialization process
>         to adapt to the new methods, as well as bringing in extra
>         dependencies
>         such as FV protocol, time protocols.
>
>         This patch series proposes to update the implementation for
>         Secure Boot
>         Variable libraries and their consumers to better support the
>         related
>         variables operations.
>
>         Patch v2 branch:
>         https://github.com/kuqin12/edk2/tree/secure_boot_enhance_v2
>
>         Cc: Jiewen Yao <jiewen.yao@intel.com>
>         Cc: Jian J Wang <jian.j.wang@intel.com>
>         Cc: Min Xu <min.m.xu@intel.com>
>         Cc: Sean Brogan <sean.brogan@microsoft.com>
>         Cc: Ard Biesheuvel <ardb+tianocore@kernel.org
>         <mailto:ardb%2Btianocore@kernel.org>>
>         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>
>         Cc: Andrew Fish <afish@apple.com>
>         Cc: Ray Ni <ray.ni@intel.com>
>
>         Kun Qin (8):
>           SecurityPkg: UefiSecureBoot: Definitions of cert and payload
>             structures
>           SecurityPkg: PlatformPKProtectionLib: Added PK protection
>         interface
>           SecurityPkg: SecureBootVariableLib: Updated time based
>         payload creator
>           SecurityPkg: SecureBootVariableProvisionLib: Updated
>         implementation
>           SecurityPkg: Secure Boot Drivers: Added common header files
>           SecurityPkg: SecureBootConfigDxe: Updated invocation pattern
>           OvmfPkg: Pipeline: Resolve SecureBootVariableLib dependency
>           EmulatorPkg: Pipeline: Resolve SecureBootVariableLib dependency
>
>         kuqin (3):
>           SecurityPkg: SecureBootVariableLib: Updated signature list
>         creator
>           SecurityPkg: SecureBootVariableLib: Added newly supported
>         interfaces
>           SecurityPkg: SecureBootVariableLib: Added unit tests
>
>          SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c
>                            |    1 +
>          SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
>          |   51 +
>          SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
>                            |  486 ++++-
>          SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
>                 |   36 +
>          SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
>                           |  201 ++
>          SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
>             |   13 +
>          SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
>               | 2037 ++++++++++++++++++++
>          SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.c
>              |  145 +-
>          SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
>                     |  128 +-
>          SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c
>            |    1 +
>          EmulatorPkg/EmulatorPkg.dsc                                  
>              |    1 +
>          OvmfPkg/Bhyve/BhyveX64.dsc                                  
>             |    1 +
>          OvmfPkg/CloudHv/CloudHvX64.dsc                              
>                 |    1 +
>          OvmfPkg/IntelTdx/IntelTdxX64.dsc                            
>                   |    1 +
>          OvmfPkg/OvmfPkgIa32.dsc                                      
>          |    1 +
>          OvmfPkg/OvmfPkgIa32X64.dsc                                  
>             |    1 +
>          OvmfPkg/OvmfPkgX64.dsc                                      
>         |    1 +
>          SecurityPkg/Include/Library/PlatformPKProtectionLib.h        
>                    |   31 +
>          SecurityPkg/Include/Library/SecureBootVariableLib.h          
>                  |  103 +-
>          SecurityPkg/Include/UefiSecureBoot.h                        
>                       |   94 +
>          SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
>         |   36 +
>          SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf
>                            |   14 +-
>          SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
>               |   33 +
>          SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
>                           |   45 +
>          SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
>           |   25 +
>          SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
>             |   36 +
>          SecurityPkg/SecurityPkg.ci.yaml                              
>                  |   11 +
>          SecurityPkg/SecurityPkg.dec                                  
>              |    5 +
>          SecurityPkg/SecurityPkg.dsc                                  
>              |    2 +
>          SecurityPkg/Test/SecurityPkgHostTest.dsc                    
>                           |   38 +
>          SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
>                    |    1 +
>          31 files changed, 3468 insertions(+), 112 deletions(-)
>          create mode 100644
>         SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
>          create mode 100644
>         SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
>          create mode 100644
>         SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
>          create mode 100644
>         SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
>          create mode 100644
>         SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
>          create mode 100644
>         SecurityPkg/Include/Library/PlatformPKProtectionLib.h
>          create mode 100644 SecurityPkg/Include/UefiSecureBoot.h
>          create mode 100644
>         SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
>          create mode 100644
>         SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
>          create mode 100644
>         SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
>          create mode 100644
>         SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
>          create mode 100644
>         SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
>          create mode 100644 SecurityPkg/Test/SecurityPkgHostTest.dsc
>
>         -- 
>         2.35.1.windows.2
>
>
>
>
>
>
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries
  2022-06-13 20:39 Kun Qin
@ 2022-06-30 19:44 ` Michael Kubacki
  0 siblings, 0 replies; 6+ messages in thread
From: Michael Kubacki @ 2022-06-30 19:44 UTC (permalink / raw)
  To: devel, kuqin12
  Cc: Jiewen Yao, Jian J Wang, Min Xu, Sean Brogan, Ard Biesheuvel,
	Jordan Justen, Gerd Hoffmann, Rebecca Cran, Peter Grehan,
	Sebastien Boeuf, Andrew Fish, Ray Ni

Acked-by: Michael Kubacki <michael.kubacki@microsoft.com>

On 6/13/2022 4:39 PM, Kun Qin wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3909
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3910
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3911
> 
> This is a revamp of a previously submitted patch series based on top of
> master branch: https://edk2.groups.io/g/devel/message/89507. No changes
> added.
> 
> Current SecureBootVariableLib provide great support for deleting secure
> boot related variables, creating time-based payloads.
> 
> However, for secure boot enrollment, the SecureBootVariableProvisionLib
> interfaces always assume the changes from variable storage, limiting the
> usage, requiring existing platforms to change key initialization process
> to adapt to the new methods, as well as bringing in extra dependencies
> such as FV protocol, time protocols.
> 
> This patch series proposes to update the implementation for Secure Boot
> Variable libraries and their consumers to better support the related
> variables operations.
> 
> Patch v2 branch: https://github.com/kuqin12/edk2/tree/secure_boot_enhance_v2
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> 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>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Ray Ni <ray.ni@intel.com>
> 
> Kun Qin (8):
>    SecurityPkg: UefiSecureBoot: Definitions of cert and payload
>      structures
>    SecurityPkg: PlatformPKProtectionLib: Added PK protection interface
>    SecurityPkg: SecureBootVariableLib: Updated time based payload creator
>    SecurityPkg: SecureBootVariableProvisionLib: Updated implementation
>    SecurityPkg: Secure Boot Drivers: Added common header files
>    SecurityPkg: SecureBootConfigDxe: Updated invocation pattern
>    OvmfPkg: Pipeline: Resolve SecureBootVariableLib dependency
>    EmulatorPkg: Pipeline: Resolve SecureBootVariableLib dependency
> 
> kuqin (3):
>    SecurityPkg: SecureBootVariableLib: Updated signature list creator
>    SecurityPkg: SecureBootVariableLib: Added newly supported interfaces
>    SecurityPkg: SecureBootVariableLib: Added unit tests
> 
>   SecurityPkg/EnrollFromDefaultKeysApp/EnrollFromDefaultKeysApp.c                           |    1 +
>   SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c   |   51 +
>   SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c                         |  486 ++++-
>   SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c          |   36 +
>   SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c                          |  201 ++
>   SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c      |   13 +
>   SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c        | 2037 ++++++++++++++++++++
>   SecurityPkg/Library/SecureBootVariableProvisionLib/SecureBootVariableProvisionLib.c       |  145 +-
>   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c              |  128 +-
>   SecurityPkg/VariableAuthenticated/SecureBootDefaultKeysDxe/SecureBootDefaultKeysDxe.c     |    1 +
>   EmulatorPkg/EmulatorPkg.dsc                                                               |    1 +
>   OvmfPkg/Bhyve/BhyveX64.dsc                                                                |    1 +
>   OvmfPkg/CloudHv/CloudHvX64.dsc                                                            |    1 +
>   OvmfPkg/IntelTdx/IntelTdxX64.dsc                                                          |    1 +
>   OvmfPkg/OvmfPkgIa32.dsc                                                                   |    1 +
>   OvmfPkg/OvmfPkgIa32X64.dsc                                                                |    1 +
>   OvmfPkg/OvmfPkgX64.dsc                                                                    |    1 +
>   SecurityPkg/Include/Library/PlatformPKProtectionLib.h                                     |   31 +
>   SecurityPkg/Include/Library/SecureBootVariableLib.h                                       |  103 +-
>   SecurityPkg/Include/UefiSecureBoot.h                                                      |   94 +
>   SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf |   36 +
>   SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.inf                       |   14 +-
>   SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf        |   33 +
>   SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf                        |   45 +
>   SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf    |   25 +
>   SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf      |   36 +
>   SecurityPkg/SecurityPkg.ci.yaml                                                           |   11 +
>   SecurityPkg/SecurityPkg.dec                                                               |    5 +
>   SecurityPkg/SecurityPkg.dsc                                                               |    2 +
>   SecurityPkg/Test/SecurityPkgHostTest.dsc                                                  |   38 +
>   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf             |    1 +
>   31 files changed, 3468 insertions(+), 112 deletions(-)
>   create mode 100644 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.c
>   create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.c
>   create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.c
>   create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.c
>   create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.c
>   create mode 100644 SecurityPkg/Include/Library/PlatformPKProtectionLib.h
>   create mode 100644 SecurityPkg/Include/UefiSecureBoot.h
>   create mode 100644 SecurityPkg/Library/PlatformPKProtectionLibVarPolicy/PlatformPKProtectionLibVarPolicy.inf
>   create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockPlatformPKProtectionLib.inf
>   create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiLib.inf
>   create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/MockUefiRuntimeServicesTableLib.inf
>   create mode 100644 SecurityPkg/Library/SecureBootVariableLib/UnitTest/SecureBootVariableLibUnitTest.inf
>   create mode 100644 SecurityPkg/Test/SecurityPkgHostTest.dsc
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-06-30 19:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <16F848B3160A57C1.5246@groups.io>
2022-06-28 21:18 ` [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries Kun Qin
2022-06-29  8:50   ` Yao, Jiewen
2022-06-29 18:06     ` Kun Qin
2022-06-30  0:19       ` Yao, Jiewen
2022-06-30  0:23         ` Kun Qin
2022-06-13 20:39 Kun Qin
2022-06-30 19:44 ` [edk2-devel] " Michael Kubacki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox