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.jus...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Cc: Stefan Berger <stef...@linux.ibm.com>
> > Signed-off-by: Gary Lin <g...@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:
>
> 20190703102228.25441-1-glin@suse.com">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
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43245): https://edk2.groups.io/g/devel/message/43245
Mute This Topic: https://groups.io/mt/32295955/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
