Re: [edk2-devel] [PATCH v2 00/11] Enhance Secure Boot Variable Libraries

["Kun Qin" <kuqin12@gmail.com>]

[-- 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 --]