Re: [Qemu-devel] [edk2] [PATCH 4/4] ovmf: process TPM PPI request in AfterConsole()

[Laszlo Ersek]

On 05/15/18 14:30, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
> 
> Call Tcg2PhysicalPresenceLibProcessRequest() to process pending PPI
> requests from PlatformBootManagerAfterConsole().
> 
> Laszlo understanding of edk2 is that the PPI operation processing was
> meant to occur *entirely* before End-Of-Dxe, so that 3rd party UEFI
> drivers couldn't interfere with PPI opcode processing *at all*.
> 
> He suggested that we should *not* call
> Tcg2PhysicalPresenceLibProcessRequest() from BeforeConsole(). Because,
> an "auth" console, i.e. one that does not depend on a 3rd party
> driver, is *in general* impossible to guarantee. Instead we could opt
> to trust 3rd party drivers, and use the "normal" console(s) in
> AfterConsole(), in order to let the user confirm the PPI requests. It
> will depend on the user to enable Secure Boot, so that the
> trustworthiness of those 3rd party drivers is ensured. If an attacker
> roots the guest OS from within, queues some TPM2 PPI requests, and
> also modifies drivers on the EFI system partition and/or in GPU option
> ROMs (?), then those drivers will not load after guest reboot, and
> thus the dependent console(s) won't be used for confirming the PPI
> requests.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c      | 8 ++++++++
>  .../PlatformBootManagerLib/PlatformBootManagerLib.inf     | 2 ++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c 
> b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 004b753f4d26..8b1beaa3e207 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -16,6 +16,7 @@
>  #include <Guid/XenInfo.h>
>  #include <Guid/RootBridgesConnectedEventGroup.h>
>  #include <Protocol/FirmwareVolume2.h>
> +#include <Library/Tcg2PhysicalPresenceLib.h>
>  
>  
>  //
> @@ -1410,6 +1411,13 @@ PlatformBootManagerAfterConsole (
>    //
>    PciAcpiInitialization ();
>  
> +
> +  //
> +  // Process TPM PPI request
> +  //
> +  Tcg2PhysicalPresenceLibProcessRequest (NULL);
> +
> +

Please just keep one empty line before and after the new code. With that
cleanup, for this patch:

Reviewed-by: Laszlo Ersek <address@hidden>

This series is a very nice work IMO, thank you both Stefan and
Marc-André. I hope v2 can be merged!

Thanks!
Laszlo

>    //
>    // Process QEMU's -kernel command line option
>    //
> diff --git 
> a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf 
> b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 27789b7377bc..4b72c44bcf0a 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -38,6 +38,7 @@ [Packages]
>    IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec
>    SourceLevelDebugPkg/SourceLevelDebugPkg.dec
>    OvmfPkg/OvmfPkg.dec
> +  SecurityPkg/SecurityPkg.dec
>  
>  [LibraryClasses]
>    BaseLib
> @@ -56,6 +57,7 @@ [LibraryClasses]
>    LoadLinuxLib
>    QemuBootOrderLib
>    UefiLib
> +  Tcg2PhysicalPresenceLib
>  
>  [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent
>