From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 03 Jul 2019 12:49:33 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C9B5A308FC20; Wed, 3 Jul 2019 19:49:32 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-89.ams2.redhat.com [10.36.116.89]) by smtp.corp.redhat.com (Postfix) with ESMTP id CAC0950ABA; Wed, 3 Jul 2019 19:49:27 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled To: devel@edk2.groups.io, glin@suse.com Cc: Jordan Justen , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , Stefan Berger References: <20190703102228.25441-1-glin@suse.com> From: "Laszlo Ersek" Message-ID: <5f74464e-1a3a-455a-ba46-0f00e20f4ce7@redhat.com> Date: Wed, 3 Jul 2019 21:49:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190703102228.25441-1-glin@suse.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.43]); Wed, 03 Jul 2019 19:49:33 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi Gary, On 07/03/19 12:22, Gary Lin wrote: > DxeTpmMeasurementLib is only useful when TPM is enabled. > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Marc-Andr=C3=A9 Lureau > Cc: Stefan Berger > Signed-off-by: Gary Lin > --- > 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 wa= s 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 computin= g 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 i= s 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