* [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled @ 2019-07-03 10:22 Gary Lin 2019-07-03 19:49 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Gary Lin @ 2019-07-03 10:22 UTC (permalink / raw) To: devel@edk2.groups.io Cc: Jordan Justen, Laszlo Ersek, Marc-André Lureau, Stefan Berger DxeTpmMeasurementLib is only useful when TPM is enabled. Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> Cc: Stefan Berger <stefanb@linux.ibm.com> Signed-off-by: Gary Lin <glin@suse.com> --- OvmfPkg/OvmfPkgIa32.dsc | 10 +++++++--- OvmfPkg/OvmfPkgIa32X64.dsc | 10 +++++++--- OvmfPkg/OvmfPkgX64.dsc | 10 +++++++--- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index 87716123997a..47f1c375a4c3 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -176,12 +176,16 @@ [LibraryClasses] OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf !endif -!if $(SECURE_BOOT_ENABLE) == TRUE - PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf +!if $(TPM2_ENABLE) == TRUE TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf - AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf !else TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf +!endif + +!if $(SECURE_BOOT_ENABLE) == TRUE + PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf + AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf +!else AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf !endif VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index ad20531ceb8b..5d4ad3d9a01a 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -181,12 +181,16 @@ [LibraryClasses] OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf !endif -!if $(SECURE_BOOT_ENABLE) == TRUE - PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf +!if $(TPM2_ENABLE) == TRUE TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf - AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf !else TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf +!endif + +!if $(SECURE_BOOT_ENABLE) == TRUE + PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf + AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf +!else AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf !endif VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index 0542ac2235b4..743eb719aae5 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -181,12 +181,16 @@ [LibraryClasses] OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf !endif -!if $(SECURE_BOOT_ENABLE) == TRUE - PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf +!if $(TPM2_ENABLE) == TRUE TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf - AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf !else TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf +!endif + +!if $(SECURE_BOOT_ENABLE) == TRUE + PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf + AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf +!else AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf !endif VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf -- 2.22.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled 2019-07-03 10:22 [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled Gary Lin @ 2019-07-03 19:49 ` Laszlo Ersek 2019-07-04 3:58 ` Gary Lin 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2019-07-03 19:49 UTC (permalink / raw) To: devel, glin; +Cc: Jordan Justen, Marc-André Lureau, Stefan Berger Hi Gary, On 07/03/19 12:22, Gary Lin wrote: > DxeTpmMeasurementLib is only useful when TPM is enabled. > > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Cc: Stefan Berger <stefanb@linux.ibm.com> > Signed-off-by: Gary Lin <glin@suse.com> > --- > OvmfPkg/OvmfPkgIa32.dsc | 10 +++++++--- > OvmfPkg/OvmfPkgIa32X64.dsc | 10 +++++++--- > OvmfPkg/OvmfPkgX64.dsc | 10 +++++++--- > 3 files changed, 21 insertions(+), 9 deletions(-) This is a good patch, thank you for it. I see two opportunities for improvement. (1) There's something weird going on with your newline characters. The view I get (in both my INBOX and in my list folder) is identical to mail-archive.com's view: http://mid.mail-archive.com/20190703102228.25441-1-glin@suse.com Can you double check your settings, please? (2) The commit message should be more convincing. How about this: ----------- (a) OvmfPkg first had to resolve the TpmMeasurementLib class -- for SECURE_BOOT_ENABLE only -- when the DxeImageVerificationLib instance became dependent on TpmMeasurementLib. For details, refer to commit 0d28d286bf4d ("OvmfPkg: resolve TpmMeasurementLib dependency introduced in r14687", 2013-09-21). (b) At the time, only one instance of TpmMeasurementLib existed, namely DxeTpmMeasurementLib. This lib instance didn't do anything -- like it was desirable for OVMF --, because OVMF didn't include any Tcg / TrEE protocol implementations. (c) In commit 308521b13354 ("MdeModulePkg: Move TpmMeasurementLib LibraryClass from SecurityPkg", 2015-07-01), TpmMeasurementLibNull was introduced. (d) In commit 285542ebbb03 ("OvmfPkg: Link AuthVariableLib for following merged variable driver deploy", 2015-07-01), a TpmMeasurementLib resolution became necessary regardless of SECURE_BOOT_ENABLE. And so TpmMeasurementLib was resolved to TpmMeasurementLibNull in OVMF, but only in the non-SECURE_BOOT_ENABLE case. This step -- possibly, the larger series containing commit 285542ebbb03 -- missed an opportunity for simplification: given (b), the DxeTpmMeasurementLib instance should have been simply replaced with the TpmMeasurementLibNull instance, regardless of SECURE_BOOT_ENABLE. (e) In commit 1abfa4ce4835 ("Add TPM2 support defined in trusted computing group.", 2015-08-13), the TrEE dependency was replaced with a Tcg2 dependency in DxeTpmMeasurementLib. (f) Starting with commit 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09), OVMF would include a Tcg2 protocol implementation, thereby satisfying DxeTpmMeasurementLib's dependency. With TPM2_ENABLE, it would actually make sense to consume DxeTpmMeasurementLib -- however, DxeTpmMeasurementLib would never be used without SECURE_BOOT_ENABLE. Therefore, we have the following four scenarios: - TPM2_ENABLE + SECURE_BOOT_ENABLE: works as expected. - Neither enabled: works as expected. - Only TPM2_ENABLE: this build is currently incorrect, because Variable/RuntimeDxe consumes TpmMeasurementLib directly, but TpmMeasureAndLogData() will never reach the TPM because we link TpmMeasurementLibNull into the variable driver. This is a problem from the larger series containing (f). - Only SECURE_BOOT_ENABLE: this build works as expected, but it is wasteful -- given that the protocol database will never contain Tcg2 without TPM2_ENABLE, we should simply use TpmMeasurementLibNull. This is a problem from (d). Resolving TpmMeasurementLib to DxeTpmMeasurementLib as a function of *only* TPM2_ENABLE, we can fix / optimize the last two cases. ----------- To reflect the reasoning in the subject line, I suggest: OvmfPkg: use DxeTpmMeasurementLib if and only if TPM2_ENABLE If you agree, please submit a v2 like this. Thanks! Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled 2019-07-03 19:49 ` [edk2-devel] " Laszlo Ersek @ 2019-07-04 3:58 ` Gary Lin 2019-07-04 8:33 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Gary Lin @ 2019-07-04 3:58 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: Jordan Justen, Stefan Berger, Marc-André Lureau On Wed, Jul 03, 2019 at 09:49:26PM +0200, Laszlo Ersek wrote: > Hi Gary, > > On 07/03/19 12:22, Gary Lin wrote: > > DxeTpmMeasurementLib is only useful when TPM is enabled. > > > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > > Cc: Stefan Berger <stefanb@linux.ibm.com> > > Signed-off-by: Gary Lin <glin@suse.com> > > --- > > OvmfPkg/OvmfPkgIa32.dsc | 10 +++++++--- > > OvmfPkg/OvmfPkgIa32X64.dsc | 10 +++++++--- > > OvmfPkg/OvmfPkgX64.dsc | 10 +++++++--- > > 3 files changed, 21 insertions(+), 9 deletions(-) > > This is a good patch, thank you for it. I see two opportunities for > improvement. > > (1) There's something weird going on with your newline characters. The > view I get (in both my INBOX and in my list folder) is identical to > mail-archive.com's view: > > http://mid.mail-archive.com/20190703102228.25441-1-glin@suse.com > > Can you double check your settings, please? > I didn't change my git settings except the mail server due to our recent server migration. Not sure if it's caused by the new mail server or not... > > (2) The commit message should be more convincing. How about this: > Will follow your suggestion to update the patch. BTW, just found that there is a TPM2_ENABLE block below the SECURE_BOOT_ENABLE block. I'll move TpmMeasurementLib there to reduce the lines of change. Thanks, Gary Lin > ----------- > (a) OvmfPkg first had to resolve the TpmMeasurementLib class -- for > SECURE_BOOT_ENABLE only -- when the DxeImageVerificationLib instance > became dependent on TpmMeasurementLib. For details, refer to commit > 0d28d286bf4d ("OvmfPkg: resolve TpmMeasurementLib dependency > introduced in r14687", 2013-09-21). > > (b) At the time, only one instance of TpmMeasurementLib existed, namely > DxeTpmMeasurementLib. This lib instance didn't do anything -- like it > was desirable for OVMF --, because OVMF didn't include any Tcg / TrEE > protocol implementations. > > (c) In commit 308521b13354 ("MdeModulePkg: Move TpmMeasurementLib > LibraryClass from SecurityPkg", 2015-07-01), TpmMeasurementLibNull was > introduced. > > (d) In commit 285542ebbb03 ("OvmfPkg: Link AuthVariableLib for following > merged variable driver deploy", 2015-07-01), a TpmMeasurementLib > resolution became necessary regardless of SECURE_BOOT_ENABLE. And so > TpmMeasurementLib was resolved to TpmMeasurementLibNull in OVMF, but > only in the non-SECURE_BOOT_ENABLE case. This step -- possibly, the > larger series containing commit 285542ebbb03 -- missed an opportunity > for simplification: given (b), the DxeTpmMeasurementLib instance > should have been simply replaced with the TpmMeasurementLibNull > instance, regardless of SECURE_BOOT_ENABLE. > > (e) In commit 1abfa4ce4835 ("Add TPM2 support defined in trusted computing > group.", 2015-08-13), the TrEE dependency was replaced with a Tcg2 > dependency in DxeTpmMeasurementLib. > > (f) Starting with commit 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", > 2018-03-09), OVMF would include a Tcg2 protocol implementation, > thereby satisfying DxeTpmMeasurementLib's dependency. With > TPM2_ENABLE, it would actually make sense to consume > DxeTpmMeasurementLib -- however, DxeTpmMeasurementLib would never be > used without SECURE_BOOT_ENABLE. > > Therefore, we have the following four scenarios: > > - TPM2_ENABLE + SECURE_BOOT_ENABLE: works as expected. > > - Neither enabled: works as expected. > > - Only TPM2_ENABLE: this build is currently incorrect, because > Variable/RuntimeDxe consumes TpmMeasurementLib directly, but > TpmMeasureAndLogData() will never reach the TPM because we link > TpmMeasurementLibNull into the variable driver. This is a problem from > the larger series containing (f). > > - Only SECURE_BOOT_ENABLE: this build works as expected, but it is > wasteful -- given that the protocol database will never contain Tcg2 > without TPM2_ENABLE, we should simply use TpmMeasurementLibNull. This is > a problem from (d). > > Resolving TpmMeasurementLib to DxeTpmMeasurementLib as a function of > *only* TPM2_ENABLE, we can fix / optimize the last two cases. > ----------- > > To reflect the reasoning in the subject line, I suggest: > > OvmfPkg: use DxeTpmMeasurementLib if and only if TPM2_ENABLE > > If you agree, please submit a v2 like this. > > Thanks! > Laszlo > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled 2019-07-04 3:58 ` Gary Lin @ 2019-07-04 8:33 ` Laszlo Ersek 2019-07-04 8:58 ` Gary Lin 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2019-07-04 8:33 UTC (permalink / raw) To: Gary Lin, devel@edk2.groups.io Cc: Jordan Justen, Stefan Berger, Marc-André Lureau On 07/04/19 05:58, Gary Lin wrote: > On Wed, Jul 03, 2019 at 09:49:26PM +0200, Laszlo Ersek wrote: >> Hi Gary, >> >> On 07/03/19 12:22, Gary Lin wrote: >>> DxeTpmMeasurementLib is only useful when TPM is enabled. >>> >>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>> Cc: Laszlo Ersek <lersek@redhat.com> >>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> >>> Cc: Stefan Berger <stefanb@linux.ibm.com> >>> Signed-off-by: Gary Lin <glin@suse.com> >>> --- >>> OvmfPkg/OvmfPkgIa32.dsc | 10 +++++++--- >>> OvmfPkg/OvmfPkgIa32X64.dsc | 10 +++++++--- >>> OvmfPkg/OvmfPkgX64.dsc | 10 +++++++--- >>> 3 files changed, 21 insertions(+), 9 deletions(-) >> >> This is a good patch, thank you for it. I see two opportunities for >> improvement. >> >> (1) There's something weird going on with your newline characters. The >> view I get (in both my INBOX and in my list folder) is identical to >> mail-archive.com's view: >> >> http://mid.mail-archive.com/20190703102228.25441-1-glin@suse.com >> >> Can you double check your settings, please? >> > I didn't change my git settings except the mail server due to our > recent server migration. Not sure if it's caused by the new mail server > or not... Not sure... your v2 on the list doesn't seem to suffer from this issue, thankfully :) > >> >> (2) The commit message should be more convincing. How about this: >> > Will follow your suggestion to update the patch. > > BTW, just found that there is a TPM2_ENABLE block below the > SECURE_BOOT_ENABLE block. I'll move TpmMeasurementLib there to reduce > the lines of change. Good idea! Thanks Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled 2019-07-04 8:33 ` Laszlo Ersek @ 2019-07-04 8:58 ` Gary Lin 0 siblings, 0 replies; 5+ messages in thread From: Gary Lin @ 2019-07-04 8:58 UTC (permalink / raw) To: Laszlo Ersek Cc: devel@edk2.groups.io, Jordan Justen, Stefan Berger, Marc-André Lureau On Thu, Jul 04, 2019 at 10:33:55AM +0200, Laszlo Ersek wrote: > On 07/04/19 05:58, Gary Lin wrote: > > On Wed, Jul 03, 2019 at 09:49:26PM +0200, Laszlo Ersek wrote: > >> Hi Gary, > >> > >> On 07/03/19 12:22, Gary Lin wrote: > >>> DxeTpmMeasurementLib is only useful when TPM is enabled. > >>> > >>> Cc: Jordan Justen <jordan.l.justen@intel.com> > >>> Cc: Laszlo Ersek <lersek@redhat.com> > >>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > >>> Cc: Stefan Berger <stefanb@linux.ibm.com> > >>> Signed-off-by: Gary Lin <glin@suse.com> > >>> --- > >>> OvmfPkg/OvmfPkgIa32.dsc | 10 +++++++--- > >>> OvmfPkg/OvmfPkgIa32X64.dsc | 10 +++++++--- > >>> OvmfPkg/OvmfPkgX64.dsc | 10 +++++++--- > >>> 3 files changed, 21 insertions(+), 9 deletions(-) > >> > >> This is a good patch, thank you for it. I see two opportunities for > >> improvement. > >> > >> (1) There's something weird going on with your newline characters. The > >> view I get (in both my INBOX and in my list folder) is identical to > >> mail-archive.com's view: > >> > >> http://mid.mail-archive.com/20190703102228.25441-1-glin@suse.com > >> > >> Can you double check your settings, please? > >> > > I didn't change my git settings except the mail server due to our > > recent server migration. Not sure if it's caused by the new mail server > > or not... > > Not sure... your v2 on the list doesn't seem to suffer from this issue, > thankfully :) > I actually strip CRs manually with dos2unix before sending the patch. It seems that my old server strip CRs automatically but the new server tends to keep it. When I viewed my first patch with mutt, the viewer ignored CRs so I didn't notice the newline problem. Then I sent the patch to a colleague who is using thunderbird, all CRs become LFs. This is really annoying :-( Gary Lin > > > >> > >> (2) The commit message should be more convincing. How about this: > >> > > Will follow your suggestion to update the patch. > > > > BTW, just found that there is a TPM2_ENABLE block below the > > SECURE_BOOT_ENABLE block. I'll move TpmMeasurementLib there to reduce > > the lines of change. > > Good idea! > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-04 9:05 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-07-03 10:22 [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled Gary Lin 2019-07-03 19:49 ` [edk2-devel] " Laszlo Ersek 2019-07-04 3:58 ` Gary Lin 2019-07-04 8:33 ` Laszlo Ersek 2019-07-04 8:58 ` Gary Lin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox