Re: [PATCH v2 11/11] StandaloneMmPkg/Core: permit encapsulated firmware volumes

["Yao, Jiewen" <jiewen.yao@intel.com>]

Add Mike Kinney.

Hi Mike
Do you have any suggestion on how to implement this in a self-contained environment for gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress?

 STATIC UINT64 mExtractGuidedSectionHandlerInfo[64];
 PcdSet64 (PcdGuidedExtractHandlerTableAddress, (UINT64)mExtractGuidedSectionHandlerInfo);

Maybe, we can consider below from secure coding perspective.
1) Define a MACRO for 64.
2) Add a check (somewhere) to see if there is overflow on 64, then stop and report error.



Thank you
Yao Jiewen

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, January 18, 2019 7:42 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel@lists.01.org; Achin Gupta <achin.gupta@arm.com>;
> Supreeth Venkatesh <supreeth.venkatesh@arm.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>;
> Thomas Panakamattam Abraham <thomas.abraham@arm.com>; Sami
> Mujawar <Sami.Mujawar@arm.com>
> Subject: Re: [PATCH v2 11/11] StandaloneMmPkg/Core: permit encapsulated
> firmware volumes
> 
> On Fri, 18 Jan 2019 at 16:40, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> >
> > The idea seems good.
> >
> > May I know if there is any restriction on 64 handlers?
> >
> > +STATIC UINT64 mExtractGuidedSectionHandlerInfo[64];
> >
> > If a system is configured with more handlers, what is expected behavior?
> >
> 
> Good question. I wasn't really sure how to implement this. For any
> given platform configuration, I don't think you will ever need more
> than one handler, unless you are encapsulating a compressed FV inside
> a signed FV perhaps?
> 
> Do you have any suggestions how to improve this code?
> 
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > > Sent: Wednesday, January 16, 2019 12:23 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Achin Gupta
> > > <achin.gupta@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Supreeth
> > > Venkatesh <supreeth.venkatesh@arm.com>; Leif Lindholm
> > > <leif.lindholm@linaro.org>; Jagadeesh Ujja <jagadeesh.ujja@arm.com>;
> > > Thomas Panakamattam Abraham <thomas.abraham@arm.com>; Sami
> > > Mujawar <Sami.Mujawar@arm.com>
> > > Subject: [PATCH v2 11/11] StandaloneMmPkg/Core: permit encapsulated
> > > firmware volumes
> > >
> > > Standalone MM requires 4 KB section alignment for all images, so that
> > > strict permissions can be applied. Unfortunately, this results in a
> > > lot of wasted space, which is usually costly in the secure world
> > > environment that standalone MM is expected to operate in.
> > >
> > > So let's permit the standalone MM drivers (but not the core) to be
> > > delivered in a compressed firmware volume.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  StandaloneMmPkg/Core/FwVol.c
> > > | 99 ++++++++++++++++++--
> > >  StandaloneMmPkg/Core/StandaloneMmCore.inf
> > > |  1 +
> > >
> > >
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Standal
> > > oneMmCoreEntryPoint.c |  5 +
> > >
> > >
> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCo
> > > reEntryPoint.inf       |  3 +
> > >  4 files changed, 99 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/StandaloneMmPkg/Core/FwVol.c
> > > b/StandaloneMmPkg/Core/FwVol.c
> > > index 5abf98c24797..8eb827dda5c4 100644
> > > --- a/StandaloneMmPkg/Core/FwVol.c
> > > +++ b/StandaloneMmPkg/Core/FwVol.c
> > > @@ -14,6 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> > > ANY KIND, EITHER EXPRESS OR IMPLIED.
> > >
> > >  #include "StandaloneMmCore.h"
> > >  #include <Library/FvLib.h>
> > > +#include <Library/ExtractGuidedSectionLib.h>
> > >
> > >  //
> > >  // List of file types supported by dispatcher
> > > @@ -65,15 +66,25 @@ Returns:
> > >
> > >  --*/
> > >  {
> > > -  EFI_STATUS          Status;
> > > -  EFI_STATUS          DepexStatus;
> > > -  EFI_FFS_FILE_HEADER *FileHeader;
> > > -  EFI_FV_FILETYPE     FileType;
> > > -  VOID                *Pe32Data;
> > > -  UINTN               Pe32DataSize;
> > > -  VOID                *Depex;
> > > -  UINTN               DepexSize;
> > > -  UINTN               Index;
> > > +  EFI_STATUS                              Status;
> > > +  EFI_STATUS                              DepexStatus;
> > > +  EFI_FFS_FILE_HEADER                     *FileHeader;
> > > +  EFI_FV_FILETYPE                         FileType;
> > > +  VOID                                    *Pe32Data;
> > > +  UINTN                                   Pe32DataSize;
> > > +  VOID                                    *Depex;
> > > +  UINTN                                   DepexSize;
> > > +  UINTN                                   Index;
> > > +  EFI_COMMON_SECTION_HEADER               *Section;
> > > +  VOID                                    *SectionData;
> > > +  UINTN                                   SectionDataSize;
> > > +  UINT32                                  DstBufferSize;
> > > +  VOID                                    *ScratchBuffer;
> > > +  UINT32                                  ScratchBufferSize;
> > > +  VOID                                    *DstBuffer;
> > > +  UINT16                                  SectionAttribute;
> > > +  UINT32
> AuthenticationStatus;
> > > +  EFI_FIRMWARE_VOLUME_HEADER              *InnerFvHeader;
> > >
> > >    DEBUG ((DEBUG_INFO, "MmCoreFfsFindMmDriver - 0x%x\n",
> > > FwVolHeader));
> > >
> > > @@ -83,6 +94,71 @@ Returns:
> > >
> > >    FvIsBeingProcesssed (FwVolHeader);
> > >
> > > +  //
> > > +  // First check for encapsulated compressed firmware volumes
> > > +  //
> > > +  FileHeader = NULL;
> > > +  do {
> > > +    Status = FfsFindNextFile
> > > (EFI_FV_FILETYPE_FIRMWARE_VOLUME_IMAGE,
> > > +               FwVolHeader, &FileHeader);
> > > +    if (EFI_ERROR (Status)) {
> > > +      break;
> > > +    }
> > > +    Status = FfsFindSectionData (EFI_SECTION_GUID_DEFINED,
> > > FileHeader,
> > > +               &SectionData, &SectionDataSize);
> > > +    if (EFI_ERROR (Status)) {
> > > +      break;
> > > +    }
> > > +    Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
> > > +    Status = ExtractGuidedSectionGetInfo (Section, &DstBufferSize,
> > > +               &ScratchBufferSize, &SectionAttribute);
> > > +    if (EFI_ERROR (Status)) {
> > > +      break;
> > > +    }
> > > +
> > > +    //
> > > +    // Allocate scratch buffer
> > > +    //
> > > +    ScratchBuffer = (VOID *)(UINTN)AllocatePages
> (EFI_SIZE_TO_PAGES
> > > (ScratchBufferSize));
> > > +    if (ScratchBuffer == NULL) {
> > > +      return EFI_OUT_OF_RESOURCES;
> > > +    }
> > > +
> > > +    //
> > > +    // Allocate destination buffer
> > > +    //
> > > +    DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES
> > > (DstBufferSize));
> > > +    if (DstBuffer == NULL) {
> > > +      return EFI_OUT_OF_RESOURCES;
> > > +    }
> > > +
> > > +    //
> > > +    // Call decompress function
> > > +    //
> > > +    Status = ExtractGuidedSectionDecode (Section, &DstBuffer,
> > > ScratchBuffer,
> > > +               &AuthenticationStatus);
> > > +    FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES
> (ScratchBufferSize));
> > > +    if (EFI_ERROR (Status)) {
> > > +      goto FreeDstBuffer;
> > > +    }
> > > +
> > > +    DEBUG ((DEBUG_INFO,
> > > +      "Processing compressed firmware volume (AuthenticationStatus
> > > == %x)\n",
> > > +      AuthenticationStatus));
> > > +
> > > +    Status = FindFfsSectionInSections (DstBuffer, DstBufferSize,
> > > +               EFI_SECTION_FIRMWARE_VOLUME_IMAGE,
> &Section);
> > > +    if (EFI_ERROR (Status)) {
> > > +      goto FreeDstBuffer;
> > > +    }
> > > +
> > > +    InnerFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)(Section +
> 1);
> > > +    Status = MmCoreFfsFindMmDriver (InnerFvHeader);
> > > +    if (EFI_ERROR (Status)) {
> > > +      goto FreeDstBuffer;
> > > +    }
> > > +  } while (TRUE);
> > > +
> > >    for (Index = 0; Index < sizeof (mMmFileTypes) / sizeof
> (mMmFileTypes[0]);
> > > Index++) {
> > >      DEBUG ((DEBUG_INFO, "Check MmFileTypes - 0x%x\n",
> > > mMmFileTypes[Index]));
> > >      FileType = mMmFileTypes[Index];
> > > @@ -100,5 +176,10 @@ Returns:
> > >      } while (!EFI_ERROR (Status));
> > >    }
> > >
> > > +  return EFI_SUCCESS;
> > > +
> > > +FreeDstBuffer:
> > > +  FreePages (DstBuffer, EFI_SIZE_TO_PAGES (DstBufferSize));
> > > +
> > >    return Status;
> > >  }
> > > diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> > > b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> > > index ff2b8b9cef03..83d31e2d92c5 100644
> > > --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
> > > +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
> > > @@ -49,6 +49,7 @@ [LibraryClasses]
> > >    BaseMemoryLib
> > >    CacheMaintenanceLib
> > >    DebugLib
> > > +  ExtractGuidedSectionLib
> > >    FvLib
> > >    HobLib
> > >    MemoryAllocationLib
> > > diff --git
> > >
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Stand
> > > aloneMmCoreEntryPoint.c
> > >
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Stand
> > > aloneMmCoreEntryPoint.c
> > > index 5cca532456fd..67ff9112d5c0 100644
> > > ---
> > >
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Stand
> > > aloneMmCoreEntryPoint.c
> > > +++
> > >
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/AArch64/Stand
> > > aloneMmCoreEntryPoint.c
> > > @@ -205,6 +205,8 @@ GetSpmVersion (VOID)
> > >    return Status;
> > >  }
> > >
> > > +STATIC UINT64 mExtractGuidedSectionHandlerInfo[64];
> > > +
> > >  /**
> > >    The entry point of Standalone MM Foundation.
> > >
> > > @@ -285,6 +287,9 @@ _ModuleEntryPoint (
> > >      goto finish;
> > >    }
> > >
> > > +  PcdSet64 (PcdGuidedExtractHandlerTableAddress,
> > > +    (UINT64)mExtractGuidedSectionHandlerInfo);
> > > +
> > >    //
> > >    // Create Hoblist based upon boot information passed by privileged
> > > software
> > >    //
> > > diff --git
> > >
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> > > CoreEntryPoint.inf
> > >
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> > > CoreEntryPoint.inf
> > > index 769eaeeefbea..55d769fa77e4 100644
> > > ---
> > >
> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> > > CoreEntryPoint.inf
> > > +++
> > >
> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMm
> > > CoreEntryPoint.inf
> > > @@ -54,3 +54,6 @@ [Guids]
> > >    gEfiMmPeiMmramMemoryReserveGuid
> > >    gEfiStandaloneMmNonSecureBufferGuid
> > >    gEfiArmTfCpuDriverEpDescriptorGuid
> > > +
> > > +[PatchPcd]
> > > +  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
> > > --
> > > 2.17.1
> >