Re: [PATCH 7/7] ovmf: add DxeTpm2MeasureBootLib

[Laszlo Ersek <lersek@redhat.com>]

On 02/23/18 14:23, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The library registers a security management handler, to measure images
> that are not measure in PEI phase.
>
> This seems to work for example with the qemu PXE rom:
>
> Loading driver at 0x0003E6C2000 EntryPoint=0x0003E6C9076 8086100e.efi
>
> And the following binary_bios_measurements log entry seems to be
> added:
>
> PCR: 2	type: EV_EFI_BOOT_SERVICES_DRIVER     	size: 0x4e	digest: 70a22475e9f18806d2ed9193b48d80d26779d9a4
>
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  OvmfPkg/OvmfPkgX64.dsc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 2281bd5ff8..92ed9f3b0c 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -677,7 +677,10 @@
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>      <LibraryClasses>
>        NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -	}
> +!if $(TPM2_ENABLE) == TRUE
> +      NULL|SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLib.inf
> +!endif
> +  }
>  !else
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>  !endif
>

This looks OK to me.

First, can you please clean up the SecurityStubDxe stanza as follows (as
a separate patch):

> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 96fc7b82e708..f4288b625cba 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -634,14 +634,12 @@ [Components]
>
>    MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
>
> -!if $(SECURE_BOOT_ENABLE) == TRUE
>    MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {
>      <LibraryClasses>
> +!if $(SECURE_BOOT_ENABLE) == TRUE
>        NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
> -       }
> -!else
> -  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
>  !endif
> +  }
>
>    MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
>    PcAtChipsetPkg/8259InterruptControllerDxe/8259.inf

The idea is that "SecurityStubDxe.inf" should be included
unconditionally; only its plug-in libs should be conditional on various
build flags. While the current (pre-patch) code does that -- in effect
-- for SECURE_BOOT_ENABLE already, your patch (as-is) can only add
TPM2_ENABLE *within* SECURE_BOOT_ENABLE.

I don't think that's for the best -- first we should make
DxeImageVerificationLib the *only* bit that's conditional on
SECURE_BOOT_ENABLE, and then we can add DxeTpm2MeasureBootLib
independently. (If neither build option is specified, we'll have a
<LibraryClasses> list that's empty, but that's perfectly fine.)

Thanks!
Laszlo