From: "Marvin Häuser" <mhaeuser@posteo.de>
To: devel@edk2.groups.io, jiewen.yao@intel.com,
Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: "marcandre.lureau@redhat.com" <marcandre.lureau@redhat.com>,
"lersek@redhat.com" <lersek@redhat.com>,
"dick_wilkins@phoenix.com" <dick_wilkins@phoenix.com>
Subject: Re: [edk2-devel] [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
Date: Sat, 7 Aug 2021 10:22:13 +0000 [thread overview]
Message-ID: <a13579fb-1c1e-3156-c572-a39e16f555ff@posteo.de> (raw)
In-Reply-To: <PH0PR11MB488543D5E14E0E79595021F38CF49@PH0PR11MB4885.namprd11.prod.outlook.com>
Hey Jiewen,
Hey Stefan,
On 07.08.21 08:00, Yao, Jiewen wrote:
> Hi Stefan
> It seems this patch series is not a production fix. It is more like a prototype, my personally feeling.
>
> A common issue in patch 2, 3, 4, 5, is that using "comment" to remove the code. Please remove the unnecessary code directly without // or /**/ in C, and # in INF.
>
> For patch 1, if you want to move the code to SecurityPkg, that is fine. Please move the whole driver their and you should not remove and code by comment. Please fix the issue to make it pass build, instead of commenting the code like work-around.
> Otherwise, you may copy the module to OvmfPkg. Then you can modify it as you need.
Maybe there should be stricter policies about what code goes where, and
duplication should be outright banned? My PE/COFF loader proposal merged
at least 4 copies of the Authenticode hashing and 3 copies of a record
construction algorithm together. There have been other code
centralisations, such as certificate iteration. If code exists twice, it
needs to me maintained twice, and in reality this does not happen.
Regarding what goes where, e.g. FatPkg is in edk2, but the new EXT4
driver was recommended to be located in edk2-platforms. I know there are
a bunch of TPM-related things (e.g. Image measuring) in SecurityPkg, and
as someone with no expertise around TPM or the edk2-platforms design, I
would never even have thought to look in edk2-platforms for such code.
And especially for library code edk2-platforms seems to be an
unfortunate location, as edk2 packages can never depend on them.
> Please also merge 2, 3, 4 into 1. I don’t think we want a broken patch in 1, then add fix in 2, 3, 4.
I think for this initial inspection this was actually a good thing. The
separation and the patches after 1 signal to me that there have been no
functional changes to the library at all. Probably the best idea is to
just move it to SecurityPkg entirely, including the PCDs, and update
MinPlatformPkg to consume it from there instead?
Best regards,
Marvin
>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Sent: Friday, August 6, 2021 11:33 PM
>> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: marcandre.lureau@redhat.com; lersek@redhat.com;
>> dick_wilkins@phoenix.com; Stefan Berger <stefanb@linux.vnet.ibm.com>
>> Subject: [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy
>>
>> This series imports code from the edk2-platforms project related to
>> changing the password of the TPM2 platform hierarchy and uses it to
>> disable the TPM2 platform hierarchy in OVMF. It addresses the OVMF
>> aspects of the following bugs:
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3510
>> https://bugzilla.tianocore.org/show_bug.cgi?id=3499
>>
>> There's no doubt that my struggles with the build system and handling
>> of dependencies are visible in this series. Quite a few aspects of
>> getting things right are more or less guesswork and I am often not sure
>> what the correct way of doing things are. If 'you' wanted to fix
>> things up and repost it, please go ahead...
>>
>> Stefan
>>
>> Stefan Berger (7):
>> SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from
>> edk2-platforms
>> SecruityPkg/TPM: Disable dependency on MinPlatformPkg
>> SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy)
>> SecurityPkg/TPM: Disable a Pcd
>> SecurityPkg/TPM: Add a NULL implementation of
>> PeiDxeTpmPlatformHierarchyLib
>> OVMF: Reference new classes in the build system for compilation
>> OVMF: Disable the TPM2 platform hierarchy
>>
>> OvmfPkg/AmdSev/AmdSevX64.dsc | 3 +
>> .../PlatformBootManagerLib/BdsPlatform.c | 6 +
>> .../PlatformBootManagerLib.inf | 1 +
>> .../PlatformBootManagerLibBhyve/BdsPlatform.c | 6 +
>> .../PlatformBootManagerLibGrub/BdsPlatform.c | 6 +
>> OvmfPkg/OvmfPkgIa32.dsc | 3 +
>> OvmfPkg/OvmfPkgIa32X64.dsc | 3 +
>> OvmfPkg/OvmfPkgX64.dsc | 3 +
>> .../Include/Library/TpmPlatformHierarchyLib.h | 27 ++
>> .../PeiDxeTpmPlatformHierarchyLib.c | 266 ++++++++++++++++++
>> .../PeiDxeTpmPlatformHierarchyLib.inf | 46 +++
>> .../PeiDxeTpmPlatformHierarchyLib.c | 23 ++
>> .../PeiDxeTpmPlatformHierarchyLib.inf | 39 +++
>> 13 files changed, 432 insertions(+)
>> create mode 100644 SecurityPkg/Include/Library/TpmPlatformHierarchyLib.h
>> create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.c
>> create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierar
>> chyLib.inf
>> create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.c
>> create mode 100644
>> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHi
>> erarchyLib.inf
>>
>> --
>> 2.31.1
>
>
>
>
>
next prev parent reply other threads:[~2021-08-07 10:22 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-06 15:33 [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 1/7] SecurityPkg/TPM: Import PeiDxeTpmPlatformHierarchyLib.c from edk2-platforms Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 2/7] SecruityPkg/TPM: Disable dependency on MinPlatformPkg Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 3/7] SecurityPkg/TPM: Disable PcdGetBool (PcdRandomizePlatformHierarchy) Stefan Berger
2021-08-07 5:38 ` Yao, Jiewen
2021-08-06 15:33 ` [RFC PATCH 4/7] SecurityPkg/TPM: Disable a Pcd Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 5/7] SecurityPkg/TPM: Add a NULL implementation of PeiDxeTpmPlatformHierarchyLib Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 6/7] OVMF: Reference new classes in the build system for compilation Stefan Berger
2021-08-06 15:33 ` [RFC PATCH 7/7] OVMF: Disable the TPM2 platform hierarchy Stefan Berger
2021-08-07 6:00 ` [RFC PATCH 0/7] " Yao, Jiewen
2021-08-07 10:22 ` Marvin Häuser [this message]
2021-08-08 0:31 ` [edk2-devel] " Yao, Jiewen
2021-08-07 15:03 ` Stefan Berger
2021-08-08 0:54 ` Yao, Jiewen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a13579fb-1c1e-3156-c572-a39e16f555ff@posteo.de \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox