Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled

["Gary Lin" <glin@suse.com>]

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
> 
> 
> 
>