[PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled

[PATCH 1/1] OvmfPkg: Only import DxeTpmMeasurementLib when TPM is enabled

[Gary Lin]

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

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

[Laszlo Ersek]

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

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

[Gary Lin]

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

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

[Laszlo Ersek]

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

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

[Gary Lin]

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
>