From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web08.4904.1628331739333508700 for ; Sat, 07 Aug 2021 03:22:20 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=r3Y/RSjs; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 4B385240105 for ; Sat, 7 Aug 2021 12:22:16 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1628331736; bh=VjbjXHG5vunAEL4skfNVf45BarG0iolRxwas8mtwJJ8=; h=Subject:To:Cc:From:Date:From; b=r3Y/RSjsDwV3r1Igb7xucl8ncK53p0/k22SO4KHh/E35XlIWmcRnB41XdOK9VArVT 22UbgIMsmqUcWr0HHRYn2uEYL54q6oerl08Nsqp2XyNTiEfEFAp8J273zaIGWxGoeS HyAdhlPgOci2TyBCKAVulOXO3IHLnO6OA0Cc0fhnzeKscgBevEjdp/z2/kWLKqwEiG 5U76sBCGviN0dH//edJcF/GaXt9k7n5E1wBsGoDGa7z208tSM76wNfnjXHgDb7/5yo sSDmximnT4lqOIAubRcmEDL2FaMHIEDrGJ/BTucb8RIi72q4lMXzuLoI8no+GnNnrW 7qe5mMxGjcyrA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Ghdgp0gwyz6tmJ; Sat, 7 Aug 2021 12:22:13 +0200 (CEST) Subject: Re: [edk2-devel] [RFC PATCH 0/7] OVMF: Disable the TPM2 platform hierarchy To: devel@edk2.groups.io, jiewen.yao@intel.com, Stefan Berger Cc: "marcandre.lureau@redhat.com" , "lersek@redhat.com" , "dick_wilkins@phoenix.com" References: <20210806153326.990749-1-stefanb@linux.vnet.ibm.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: Date: Sat, 7 Aug 2021 10:22:13 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable 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 pro= totype, 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. P= lease move the whole driver their and you should not remove and code by com= ment. 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=20 duplication should be outright banned? My PE/COFF loader proposal merged=20 at least 4 copies of the Authenticode hashing and 3 copies of a record=20 construction algorithm together. There have been other code=20 centralisations, such as certificate iteration. If code exists twice, it=20 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=20 driver was recommended to be located in edk2-platforms. I know there are=20 a bunch of TPM-related things (e.g. Image measuring) in SecurityPkg, and=20 as someone with no expertise around TPM or the edk2-platforms design, I=20 would never even have thought to look in edk2-platforms for such code.=20 And especially for library code edk2-platforms seems to be an=20 unfortunate location, as edk2 packages can never depend on them. > Please also merge 2, 3, 4 into 1. I don=E2=80=99t 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=20 separation and the patches after 1 signal to me that there have been no=20 functional changes to the library at all. Probably the best idea is to=20 just move it to SecurityPkg entirely, including the PCDs, and update=20 MinPlatformPkg to consume it from there instead? Best regards, Marvin > > Thank you > Yao Jiewen > >> -----Original Message----- >> From: Stefan Berger >> Sent: Friday, August 6, 2021 11:33 PM >> To: devel@edk2.groups.io; Yao, Jiewen >> Cc: marcandre.lureau@redhat.com; lersek@redhat.com; >> dick_wilkins@phoenix.com; Stefan Berger >> 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=3D3510 >> https://bugzilla.tianocore.org/show_bug.cgi?id=3D3499 >> >> 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/PeiDxeTpmPlatformHiera= r >> chyLib.c >> create mode 100644 >> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHiera= r >> chyLib.inf >> create mode 100644 >> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformH= i >> erarchyLib.c >> create mode 100644 >> SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformH= i >> erarchyLib.inf >> >> -- >> 2.31.1 > > >=20 > >