Re: [edk2] [PATCH v2 11/11] StandaloneMmPkg/Core: permit encapsulated firmware volumes
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.biesheu...@linaro.org]
> Sent: Friday, January 18, 2019 7:42 AM
> To: Yao, Jiewen
> Cc: edk2-devel@lists.01.org; Achin Gupta ;
> Supreeth Venkatesh ; Leif Lindholm
> ; Jagadeesh Ujja ;
> Thomas Panakamattam Abraham ; Sami
> Mujawar
> Subject: Re: [PATCH v2 11/11] StandaloneMmPkg/Core: permit encapsulated
> firmware volumes
>
> On Fri, 18 Jan 2019 at 16:40, Yao, Jiewen 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.biesheu...@linaro.org]
> > > Sent: Wednesday, January 16, 2019 12:23 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Ard Biesheuvel ; Achin Gupta
> > > ; Yao, Jiewen ;
> Supreeth
> > > Venkatesh ; Leif Lindholm
> > > ; Jagadeesh Ujja ;
> > > Thomas Panakamattam Abraham ; Sami
> > > Mujawar
> > > 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
> > > ---
> > > 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
> > > +#include
> > >
> > > //
> > > // 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
Re: [edk2] [PATCH v2 11/11] StandaloneMmPkg/Core: permit encapsulated firmware volumes
On Fri, 18 Jan 2019 at 16:40, Yao, Jiewen 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.biesheu...@linaro.org]
> > Sent: Wednesday, January 16, 2019 12:23 PM
> > To: edk2-devel@lists.01.org
> > Cc: Ard Biesheuvel ; Achin Gupta
> > ; Yao, Jiewen ; Supreeth
> > Venkatesh ; Leif Lindholm
> > ; Jagadeesh Ujja ;
> > Thomas Panakamattam Abraham ; Sami
> > Mujawar
> > 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
> > ---
> > 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
> > +#include
> >
> > //
> > // 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, );
> > +if (EFI_ERROR (Status)) {
> > + break;
> > +}
> > +Status = FfsFindSectionData (EFI_SECTION_GUID_DEFINED,
> > FileHeader,
> > + , );
> > +if (EFI_ERROR (Status)) {
> > + break;
> > +}
> > +Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
> > +Status = ExtractGuidedSectionGetInfo (Section, ,
> > + , );
> > +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;
> > +}
> > +
> > +
Re: [edk2] [PATCH v2 11/11] StandaloneMmPkg/Core: permit encapsulated firmware volumes
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?
Thank you
Yao Jiewen
> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Wednesday, January 16, 2019 12:23 PM
> To: edk2-devel@lists.01.org
> Cc: Ard Biesheuvel ; Achin Gupta
> ; Yao, Jiewen ; Supreeth
> Venkatesh ; Leif Lindholm
> ; Jagadeesh Ujja ;
> Thomas Panakamattam Abraham ; Sami
> Mujawar
> 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
> ---
> 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
> +#include
>
> //
> // 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, );
> +if (EFI_ERROR (Status)) {
> + break;
> +}
> +Status = FfsFindSectionData (EFI_SECTION_GUID_DEFINED,
> FileHeader,
> + , );
> +if (EFI_ERROR (Status)) {
> + break;
> +}
> +Section = (EFI_COMMON_SECTION_HEADER *)(FileHeader + 1);
> +Status = ExtractGuidedSectionGetInfo (Section, ,
> + , );
> +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, ,
> ScratchBuffer,
> + );
> +FreePages (ScratchBuffer, EFI_SIZE_TO_PAGES (ScratchBufferSize));
> +if (EFI_ERROR (Status)) {
> + goto FreeDstBuffer;
> +}
> +
> +DEBUG ((DEBUG_INFO,
> +
