Re: [PATCH 4/4] MdeModulePkg/SecurityStubDxe: Report failure if image is load earlier

[Laszlo Ersek <lersek@redhat.com>]

On 11/04/16 01:59, Ruiyu Ni wrote:
> The 3rd party image should be loaded after EndOfDxe event signal and
> DxeSmmReadyToLock protocol installation. But non-SMM platform doesn't
> published DxeSmmReadyToLock protocol.
> So the SecurityStubDxe can only depend on EndOfDxe event.
> 
> This patch enhances the SecurityStubDxe to listen on
> DxeSmmReadyToLock protocol installation and if any 3rd party image
> is loaded before DxeSmmReadyToLock, it reports failure.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  .../SecurityStubDxe/Defer3rdPartyImageLoad.c       | 57 ++++++++++++++++++++++
>  .../SecurityStubDxe/Defer3rdPartyImageLoad.h       |  5 +-
>  .../Universal/SecurityStubDxe/SecurityStubDxe.inf  |  3 ++
>  3 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.c b/MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.c
> index ca45d56..84d573b 100644
> --- a/MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.c
> +++ b/MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.c
> @@ -30,6 +30,7 @@ typedef struct {
>    DEFERRED_3RD_PARTY_IMAGE_INFO     *ImageInfo;    ///< deferred 3rd party image item
>  } DEFERRED_3RD_PARTY_IMAGE_TABLE;
>  
> +BOOLEAN                          mImageLoadedAfterEndOfDxe   = FALSE;
>  BOOLEAN                          mEndOfDxe                   = FALSE;
>  DEFERRED_3RD_PARTY_IMAGE_TABLE   mDeferred3rdPartyImage = {
>    0,       // Deferred image count
> @@ -257,6 +258,52 @@ EndOfDxe (
>  }
>  
>  /**
> +  Event notification for gEfiDxeSmmReadyToLockProtocolGuid event.
> +
> +  This function reports failure if any deferred image is loaded before
> +  this callback.
> +  Platform should publish ReadyToLock protocol immediately after signaling
> +  of the End of DXE Event.
> +
> +  @param  Event                 The Event that is being processed, not used.
> +  @param  Context               Event Context, not used.
> +
> +**/
> +VOID
> +EFIAPI
> +DxdSmmReadyToLock (

"Dxd" is a typo, it should be "Dxe".

> +  IN EFI_EVENT  Event,
> +  IN VOID       *Context
> +  )
> +{
> +  EFI_STATUS                Status;
> +  VOID                      *Interface;
> +
> +  Status = gBS->LocateProtocol (&gEfiDxeSmmReadyToLockProtocolGuid, NULL, &Interface);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +
> +  gBS->CloseEvent (Event);
> +
> +  if (mImageLoadedAfterEndOfDxe) {
> +    //
> +    // Platform should not dispatch the 3rd party images after signaling EndOfDxe event
> +    // but before publishing DxeSmmReadyToLock protocol.
> +    //
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "[Security] 3rd party images must be dispatched after DxeSmmReadyToLock Protocol installation!\n"
> +      ));
> +    REPORT_STATUS_CODE (
> +      EFI_ERROR_CODE | EFI_ERROR_UNRECOVERED,
> +      (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_EC_ILLEGAL_SOFTWARE_STATE)
> +      );
> +    ASSERT (FALSE);

I suggest to add an explicit CpuDeadLoop() call here, after the
ASSERT(FALSE). I learned this pattern from Mike:

https://www.mail-archive.com/edk2-devel@lists.01.org/msg04590.html

Thanks!
Laszlo

> +  }
> +}
> +
> +/**
>    Defer the 3rd party image load and installs Deferred Image Load Protocol.
>  
>    @param[in]  File                  This is a pointer to the device path of the file that
> @@ -303,6 +350,7 @@ Defer3rdPartyImageLoad (
>      );
>  
>    if (mEndOfDxe) {
> +    mImageLoadedAfterEndOfDxe = TRUE;
>      //
>      // The image might be first time loaded after EndOfDxe,
>      // So ImageInfo can be NULL.
> @@ -334,6 +382,7 @@ Defer3rdPartyImageLoadInitialize (
>    EFI_STATUS                           Status;
>    EFI_HANDLE                           Handle;
>    EFI_EVENT                            Event;
> +  VOID                                 *Registration;
>  
>    Handle = NULL;
>    Status = gBS->InstallMultipleProtocolInterfaces (
> @@ -353,4 +402,12 @@ Defer3rdPartyImageLoadInitialize (
>                    &Event
>                    );
>    ASSERT_EFI_ERROR (Status);
> +
> +  EfiCreateProtocolNotifyEvent (
> +    &gEfiDxeSmmReadyToLockProtocolGuid,
> +    TPL_CALLBACK,
> +    DxdSmmReadyToLock,
> +    NULL,
> +    &Registration
> +    );
>  }
> diff --git a/MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.h b/MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.h
> index 3fab258..75553ba 100644
> --- a/MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.h
> +++ b/MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.h
> @@ -15,16 +15,19 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #ifndef _DEFER_3RD_PARTY_IMAGE_LOAD_H_
>  #define _DEFER_3RD_PARTY_IMAGE_LOAD_H_
>  
> -#include <Uefi.h>
> +#include <PiDxe.h>
>  #include <Guid/EventGroup.h>
>  #include <Protocol/DeferredImageLoad.h>
>  #include <Protocol/FirmwareVolume2.h>
> +#include <Protocol/DxeSmmReadyToLock.h>
>  
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/DevicePathLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/ReportStatusCodeLib.h>
>  
>  /**
>    Returns information about a deferred image.
> diff --git a/MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf b/MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
> index be6ce6c..7f8f6cb 100644
> --- a/MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
> +++ b/MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf
> @@ -41,6 +41,8 @@ [LibraryClasses]
>    UefiBootServicesTableLib
>    DebugLib
>    SecurityManagementLib
> +  ReportStatusCodeLib
> +  UefiLib
>  
>  [Guids]
>    gEfiEndOfDxeEventGroupGuid                    ## CONSUMES ## Event
> @@ -49,6 +51,7 @@ [Protocols]
>    gEfiSecurityArchProtocolGuid                  ## PRODUCES
>    gEfiSecurity2ArchProtocolGuid                 ## PRODUCES
>    gEfiDeferredImageLoadProtocolGuid             ## PRODUCES
> +  gEfiDxeSmmReadyToLockProtocolGuid             ## CONSUMES
>  
>  [Depex]
>    TRUE
>