Re: [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Nov 20, 2018 at 08:40:28PM +0800, Ming Huang wrote:
> 
> 
> On 11/20/2018 8:13 PM, Leif Lindholm wrote:
> > On Tue, Nov 20, 2018 at 05:01:46PM +0800, Ming Huang wrote:
> >> Now that the generic Variable Runtime DXE code no longer
> >> distinguishes between gEfiVariableGuid and
> >> gEfiAuthenticatedVariableGuid in the varstore FV header.
> >> We can relax the check in the flashFvb driver to accept
> >> either GUID regardless of whether we are running a secure
> >> boot capable build or not.
> > 
> > We are still in a situation where D06 is not buildable with Secure
> > Boot enabled though.
> > 
> > If you build with -D SECURE_BOOT_ENABLE=TRUE, you still end up with
> > /work/git/edk2-platforms/Platform/Hisilicon/D06/D06.dsc(...): error
> > 4000: Instance of library class [PlatformSecureLib] is not found
> >       in [/work/git/edk2/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf] [AARCH64]
> >       consumed by module [/work/git/edk2/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf]
> > 
> > And all Hisilicon platforms still use
> > AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
> > regardless of Secure Boot setting.
> > 
> > So what problem does this patch solve? A runtime one?
> 
> This patch solve bug in FlashFvbDxe.

Yes, but what bug? What is the symptom? What _specific_ problem goes
away by adding this patch? That information should have been in the
original commit message. I have no information available to me as I
now build -rc1 to suggest that this patch should be included.

> Should I add a patch before this patch
> to solve build error with -D SECURE_BOOT_ENABLE=TRUE in v4?

That would require a sane implementation of PlatformSecureLib,
implementing a real UserPhysicalPresent().
Can you turn that around within the next few hours?

Regards,

Leif

> Thanks.
> 
> > 
> > /
> >     Leif
> > 
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ming Huang <ming.huang@linaro.org>
> >> ---
> >>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf | 1 +
> >>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c   | 5 +++--
> >>  2 files changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
> >> index f8be4741ef7c..a0226e0d87c0 100644
> >> --- a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
> >> +++ b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
> >> @@ -44,6 +44,7 @@ [LibraryClasses]
> >>    UefiRuntimeLib
> >>  
> >>  [Guids]
> >> +  gEfiAuthenticatedVariableGuid
> >>    gEfiSystemNvDataFvGuid
> >>    gEfiVariableGuid
> >>  
> >> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
> >> index e18cc9e06ec2..12baed41cd4e 100644
> >> --- a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
> >> +++ b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
> >> @@ -189,7 +189,7 @@ InitializeFvAndVariableStoreHeaders (
> >>      // VARIABLE_STORE_HEADER
> >>      //
> >>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)Headers + (UINTN)FirmwareVolumeHeader->HeaderLength);
> >> -    CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);
> >> +    CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);
> >>      VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - FirmwareVolumeHeader->HeaderLength;
> >>      VariableStoreHeader->Format            = VARIABLE_STORE_FORMATTED;
> >>      VariableStoreHeader->State             = VARIABLE_STORE_HEALTHY;
> >> @@ -258,7 +258,8 @@ ValidateFvHeader (
> >>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeader + (UINTN)FwVolHeader->HeaderLength);
> >>  
> >>      // Check the Variable Store Guid
> >> -    if ( CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) == FALSE )
> >> +    if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) &&
> >> +        !CompareGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid))
> >>      {
> >>          DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Variable Store Guid non-compatible\n"));
> >>          return EFI_NOT_FOUND;
> >> -- 
> >> 2.9.5
> >>