[edk2,edk2-platforms,v2,07/15] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe

Message ID 20181116065702.30559-8-ming.huang@linaro.org
State New
Headers show
Series
  • Fix D06 SBSA/SBBR issue and improve
Related show

Commit Message

Ming Huang Nov. 16, 2018, 6:56 a.m.
When SECURE_BOOT_ENABLE is TRUE, FlashFvbDxe should use
gEfiAuthenticatedVariableGuid, When SECURE_BOOT_ENABLE
is FALSE, gEfiVariableGuid should be used.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ming Huang <ming.huang@linaro.org>

---
 Silicon/Hisilicon/HisiPkg.dec                         |  1 +
 Platform/Hisilicon/D03/D03.dsc                        |  5 +++++
 Platform/Hisilicon/D05/D05.dsc                        |  5 +++++
 Platform/Hisilicon/D06/D06.dsc                        |  5 +++++
 Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf |  2 ++
 Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c   | 14 ++++++++++++--
 6 files changed, 30 insertions(+), 2 deletions(-)

-- 
2.9.5

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Nov. 19, 2018, 6:13 p.m. | #1
On Fri, Nov 16, 2018 at 02:56:54PM +0800, Ming Huang wrote:
> When SECURE_BOOT_ENABLE is TRUE, FlashFvbDxe should use

> gEfiAuthenticatedVariableGuid, When SECURE_BOOT_ENABLE

> is FALSE, gEfiVariableGuid should be used.

> 


Other platforms seem to resolve this by doing something like:

!if $(SECURE_BOOT_ENABLE)
  ...
  AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
!else
  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
!endif

Can the same mechanism be used here instead?

I _really_ don't like the idea of adding vendor-specific Pcds to
determine whether Secure Boot is enabled.

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ming Huang <ming.huang@linaro.org>

> ---

>  Silicon/Hisilicon/HisiPkg.dec                         |  1 +

>  Platform/Hisilicon/D03/D03.dsc                        |  5 +++++

>  Platform/Hisilicon/D05/D05.dsc                        |  5 +++++

>  Platform/Hisilicon/D06/D06.dsc                        |  5 +++++

>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf |  2 ++

>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c   | 14 ++++++++++++--

>  6 files changed, 30 insertions(+), 2 deletions(-)

> 

> diff --git a/Silicon/Hisilicon/HisiPkg.dec b/Silicon/Hisilicon/HisiPkg.dec

> index 404a3ae4af9d..af9359e4d0e0 100644

> --- a/Silicon/Hisilicon/HisiPkg.dec

> +++ b/Silicon/Hisilicon/HisiPkg.dec

> @@ -278,6 +278,7 @@ [PcdsFixedAtBuild]

>  

>    gHisiTokenSpaceGuid.Pcdsoctype|0|UINT32|0x00000061

>    gHisiTokenSpaceGuid.PcdSerDesFlowCtrlFlag|0|UINT32|0x40000056

> +  gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE|BOOLEAN|0x40000058

>  

>  [PcdsFeatureFlag]

>    gHisiTokenSpaceGuid.PcdIsItsSupported|FALSE|BOOLEAN|0x00000065

> diff --git a/Platform/Hisilicon/D03/D03.dsc b/Platform/Hisilicon/D03/D03.dsc

> index aa1da5d61f83..ba3096672db0 100644

> --- a/Platform/Hisilicon/D03/D03.dsc

> +++ b/Platform/Hisilicon/D03/D03.dsc

> @@ -281,6 +281,11 @@ [PcdsFixedAtBuild.common]

>    gHisiTokenSpaceGuid.PcdHb0Rb2IoSize|0xffff #64K

>  

>    gHisiTokenSpaceGuid.Pcdsoctype|0x1610

> +  !if $(SECURE_BOOT_ENABLE) == TRUE

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

> +  !else

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

> +  !endif

>  

>  ################################################################################

>  #

> diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc

> index 1040466633ef..b8500cef8742 100644

> --- a/Platform/Hisilicon/D05/D05.dsc

> +++ b/Platform/Hisilicon/D05/D05.dsc

> @@ -422,6 +422,11 @@ [PcdsFixedAtBuild.common]

>    gHisiTokenSpaceGuid.PcdHb1Rb7IoSize|0x10000 #64K

>  

>    gHisiTokenSpaceGuid.Pcdsoctype|0x1610

> +  !if $(SECURE_BOOT_ENABLE) == TRUE

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

> +  !else

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

> +  !endif

>  

>  ################################################################################

>  #

> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc

> index 1a479c160e80..b6ef9fedf0a7 100644

> --- a/Platform/Hisilicon/D06/D06.dsc

> +++ b/Platform/Hisilicon/D06/D06.dsc

> @@ -243,6 +243,11 @@ [PcdsFixedAtBuild.common]

>  

>    gEfiMdeModulePkgTokenSpaceGuid.PcdSrIovSupport|FALSE

>    gArmTokenSpaceGuid.PcdPciIoTranslation|0x0

> +  !if $(SECURE_BOOT_ENABLE) == TRUE

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

> +  !else

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

> +  !endif

>  

>  ################################################################################

>  #

> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf

> index f8be4741ef7c..47965a707032 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

>  

> @@ -62,6 +63,7 @@ [Pcd.common]

>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize

>  

>    gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked

> +  gHisiTokenSpaceGuid.PcdIsSecureBoot

>    gHisiTokenSpaceGuid.PcdSFCMEM0BaseAddress

>  

>  [Depex]

> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

> index e18cc9e06ec2..309941d6fe4d 100644

> --- a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

> +++ b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

> @@ -189,7 +189,11 @@ InitializeFvAndVariableStoreHeaders (

>      // VARIABLE_STORE_HEADER

>      //

>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)Headers + (UINTN)FirmwareVolumeHeader->HeaderLength);

> -    CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);

> +    if (PcdGetBool (PcdIsSecureBoot)) {

> +      CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);

> +    } else {

> +      CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);

> +    }

>      VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - FirmwareVolumeHeader->HeaderLength;

>      VariableStoreHeader->Format            = VARIABLE_STORE_FORMATTED;

>      VariableStoreHeader->State             = VARIABLE_STORE_HEALTHY;

> @@ -220,6 +224,7 @@ ValidateFvHeader (

>      VARIABLE_STORE_HEADER*      VariableStoreHeader;

>      UINTN                       VariableStoreLength;

>      UINTN                       FvLength;

> +    EFI_GUID                    *Guid;

>  

>      FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER*)Instance->RegionBaseAddress;

>  

> @@ -258,7 +263,12 @@ ValidateFvHeader (

>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeader + (UINTN)FwVolHeader->HeaderLength);

>  

>      // Check the Variable Store Guid

> -    if ( CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) == FALSE )

> +    if (PcdGetBool (PcdIsSecureBoot)) {

> +      Guid = &gEfiAuthenticatedVariableGuid;

> +    } else {

> +      Guid = &gEfiVariableGuid;

> +    }

> +    if (CompareGuid (&VariableStoreHeader->Signature, Guid) == FALSE)

>      {

>          DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Variable Store Guid non-compatible\n"));

>          return EFI_NOT_FOUND;

> -- 

> 2.9.5

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 19, 2018, 6:19 p.m. | #2
On Thu, 15 Nov 2018 at 22:57, Ming Huang <ming.huang@linaro.org> wrote:
>

> When SECURE_BOOT_ENABLE is TRUE, FlashFvbDxe should use

> gEfiAuthenticatedVariableGuid, When SECURE_BOOT_ENABLE

> is FALSE, gEfiVariableGuid should be used.

>


Can we fix the driver instead so we don't need to make this distinction?

Please refer to commit 8753858f84768fa6fa17191b86c97538457723ce in the
EDK2 for some background.

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ming Huang <ming.huang@linaro.org>

> ---

>  Silicon/Hisilicon/HisiPkg.dec                         |  1 +

>  Platform/Hisilicon/D03/D03.dsc                        |  5 +++++

>  Platform/Hisilicon/D05/D05.dsc                        |  5 +++++

>  Platform/Hisilicon/D06/D06.dsc                        |  5 +++++

>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf |  2 ++

>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c   | 14 ++++++++++++--

>  6 files changed, 30 insertions(+), 2 deletions(-)

>

> diff --git a/Silicon/Hisilicon/HisiPkg.dec b/Silicon/Hisilicon/HisiPkg.dec

> index 404a3ae4af9d..af9359e4d0e0 100644

> --- a/Silicon/Hisilicon/HisiPkg.dec

> +++ b/Silicon/Hisilicon/HisiPkg.dec

> @@ -278,6 +278,7 @@ [PcdsFixedAtBuild]

>

>    gHisiTokenSpaceGuid.Pcdsoctype|0|UINT32|0x00000061

>    gHisiTokenSpaceGuid.PcdSerDesFlowCtrlFlag|0|UINT32|0x40000056

> +  gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE|BOOLEAN|0x40000058

>

>  [PcdsFeatureFlag]

>    gHisiTokenSpaceGuid.PcdIsItsSupported|FALSE|BOOLEAN|0x00000065

> diff --git a/Platform/Hisilicon/D03/D03.dsc b/Platform/Hisilicon/D03/D03.dsc

> index aa1da5d61f83..ba3096672db0 100644

> --- a/Platform/Hisilicon/D03/D03.dsc

> +++ b/Platform/Hisilicon/D03/D03.dsc

> @@ -281,6 +281,11 @@ [PcdsFixedAtBuild.common]

>    gHisiTokenSpaceGuid.PcdHb0Rb2IoSize|0xffff #64K

>

>    gHisiTokenSpaceGuid.Pcdsoctype|0x1610

> +  !if $(SECURE_BOOT_ENABLE) == TRUE

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

> +  !else

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

> +  !endif

>

>  ################################################################################

>  #

> diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc

> index 1040466633ef..b8500cef8742 100644

> --- a/Platform/Hisilicon/D05/D05.dsc

> +++ b/Platform/Hisilicon/D05/D05.dsc

> @@ -422,6 +422,11 @@ [PcdsFixedAtBuild.common]

>    gHisiTokenSpaceGuid.PcdHb1Rb7IoSize|0x10000 #64K

>

>    gHisiTokenSpaceGuid.Pcdsoctype|0x1610

> +  !if $(SECURE_BOOT_ENABLE) == TRUE

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

> +  !else

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

> +  !endif

>

>  ################################################################################

>  #

> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc

> index 1a479c160e80..b6ef9fedf0a7 100644

> --- a/Platform/Hisilicon/D06/D06.dsc

> +++ b/Platform/Hisilicon/D06/D06.dsc

> @@ -243,6 +243,11 @@ [PcdsFixedAtBuild.common]

>

>    gEfiMdeModulePkgTokenSpaceGuid.PcdSrIovSupport|FALSE

>    gArmTokenSpaceGuid.PcdPciIoTranslation|0x0

> +  !if $(SECURE_BOOT_ENABLE) == TRUE

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

> +  !else

> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

> +  !endif

>

>  ################################################################################

>  #

> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf

> index f8be4741ef7c..47965a707032 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

>

> @@ -62,6 +63,7 @@ [Pcd.common]

>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize

>

>    gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked

> +  gHisiTokenSpaceGuid.PcdIsSecureBoot

>    gHisiTokenSpaceGuid.PcdSFCMEM0BaseAddress

>

>  [Depex]

> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

> index e18cc9e06ec2..309941d6fe4d 100644

> --- a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

> +++ b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

> @@ -189,7 +189,11 @@ InitializeFvAndVariableStoreHeaders (

>      // VARIABLE_STORE_HEADER

>      //

>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)Headers + (UINTN)FirmwareVolumeHeader->HeaderLength);

> -    CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);

> +    if (PcdGetBool (PcdIsSecureBoot)) {

> +      CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);

> +    } else {

> +      CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);

> +    }

>      VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - FirmwareVolumeHeader->HeaderLength;

>      VariableStoreHeader->Format            = VARIABLE_STORE_FORMATTED;

>      VariableStoreHeader->State             = VARIABLE_STORE_HEALTHY;

> @@ -220,6 +224,7 @@ ValidateFvHeader (

>      VARIABLE_STORE_HEADER*      VariableStoreHeader;

>      UINTN                       VariableStoreLength;

>      UINTN                       FvLength;

> +    EFI_GUID                    *Guid;

>

>      FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER*)Instance->RegionBaseAddress;

>

> @@ -258,7 +263,12 @@ ValidateFvHeader (

>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeader + (UINTN)FwVolHeader->HeaderLength);

>

>      // Check the Variable Store Guid

> -    if ( CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) == FALSE )

> +    if (PcdGetBool (PcdIsSecureBoot)) {

> +      Guid = &gEfiAuthenticatedVariableGuid;

> +    } else {

> +      Guid = &gEfiVariableGuid;

> +    }

> +    if (CompareGuid (&VariableStoreHeader->Signature, Guid) == FALSE)

>      {

>          DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Variable Store Guid non-compatible\n"));

>          return EFI_NOT_FOUND;

> --

> 2.9.5

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ming Huang Nov. 20, 2018, 6:42 a.m. | #3
On 11/20/2018 2:13 AM, Leif Lindholm wrote:
> On Fri, Nov 16, 2018 at 02:56:54PM +0800, Ming Huang wrote:

>> When SECURE_BOOT_ENABLE is TRUE, FlashFvbDxe should use

>> gEfiAuthenticatedVariableGuid, When SECURE_BOOT_ENABLE

>> is FALSE, gEfiVariableGuid should be used.

>>

> 

> Other platforms seem to resolve this by doing something like:

> 

> !if $(SECURE_BOOT_ENABLE)

>   ...

>   AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

> !else

>   AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

> !endif

> 

> Can the same mechanism be used here instead?


I see Ard provide a patch which is similar to this patch, so modify this
patch like that patch.
Thanks.

> 

> I _really_ don't like the idea of adding vendor-specific Pcds to

> determine whether Secure Boot is enabled.

> 

> /

>     Leif

> 

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ming Huang <ming.huang@linaro.org>

>> ---

>>  Silicon/Hisilicon/HisiPkg.dec                         |  1 +

>>  Platform/Hisilicon/D03/D03.dsc                        |  5 +++++

>>  Platform/Hisilicon/D05/D05.dsc                        |  5 +++++

>>  Platform/Hisilicon/D06/D06.dsc                        |  5 +++++

>>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf |  2 ++

>>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c   | 14 ++++++++++++--

>>  6 files changed, 30 insertions(+), 2 deletions(-)

>>

>> diff --git a/Silicon/Hisilicon/HisiPkg.dec b/Silicon/Hisilicon/HisiPkg.dec

>> index 404a3ae4af9d..af9359e4d0e0 100644

>> --- a/Silicon/Hisilicon/HisiPkg.dec

>> +++ b/Silicon/Hisilicon/HisiPkg.dec

>> @@ -278,6 +278,7 @@ [PcdsFixedAtBuild]

>>  

>>    gHisiTokenSpaceGuid.Pcdsoctype|0|UINT32|0x00000061

>>    gHisiTokenSpaceGuid.PcdSerDesFlowCtrlFlag|0|UINT32|0x40000056

>> +  gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE|BOOLEAN|0x40000058

>>  

>>  [PcdsFeatureFlag]

>>    gHisiTokenSpaceGuid.PcdIsItsSupported|FALSE|BOOLEAN|0x00000065

>> diff --git a/Platform/Hisilicon/D03/D03.dsc b/Platform/Hisilicon/D03/D03.dsc

>> index aa1da5d61f83..ba3096672db0 100644

>> --- a/Platform/Hisilicon/D03/D03.dsc

>> +++ b/Platform/Hisilicon/D03/D03.dsc

>> @@ -281,6 +281,11 @@ [PcdsFixedAtBuild.common]

>>    gHisiTokenSpaceGuid.PcdHb0Rb2IoSize|0xffff #64K

>>  

>>    gHisiTokenSpaceGuid.Pcdsoctype|0x1610

>> +  !if $(SECURE_BOOT_ENABLE) == TRUE

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

>> +  !else

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

>> +  !endif

>>  

>>  ################################################################################

>>  #

>> diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc

>> index 1040466633ef..b8500cef8742 100644

>> --- a/Platform/Hisilicon/D05/D05.dsc

>> +++ b/Platform/Hisilicon/D05/D05.dsc

>> @@ -422,6 +422,11 @@ [PcdsFixedAtBuild.common]

>>    gHisiTokenSpaceGuid.PcdHb1Rb7IoSize|0x10000 #64K

>>  

>>    gHisiTokenSpaceGuid.Pcdsoctype|0x1610

>> +  !if $(SECURE_BOOT_ENABLE) == TRUE

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

>> +  !else

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

>> +  !endif

>>  

>>  ################################################################################

>>  #

>> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc

>> index 1a479c160e80..b6ef9fedf0a7 100644

>> --- a/Platform/Hisilicon/D06/D06.dsc

>> +++ b/Platform/Hisilicon/D06/D06.dsc

>> @@ -243,6 +243,11 @@ [PcdsFixedAtBuild.common]

>>  

>>    gEfiMdeModulePkgTokenSpaceGuid.PcdSrIovSupport|FALSE

>>    gArmTokenSpaceGuid.PcdPciIoTranslation|0x0

>> +  !if $(SECURE_BOOT_ENABLE) == TRUE

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

>> +  !else

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

>> +  !endif

>>  

>>  ################################################################################

>>  #

>> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf

>> index f8be4741ef7c..47965a707032 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

>>  

>> @@ -62,6 +63,7 @@ [Pcd.common]

>>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize

>>  

>>    gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked

>> +  gHisiTokenSpaceGuid.PcdIsSecureBoot

>>    gHisiTokenSpaceGuid.PcdSFCMEM0BaseAddress

>>  

>>  [Depex]

>> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

>> index e18cc9e06ec2..309941d6fe4d 100644

>> --- a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

>> +++ b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

>> @@ -189,7 +189,11 @@ InitializeFvAndVariableStoreHeaders (

>>      // VARIABLE_STORE_HEADER

>>      //

>>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)Headers + (UINTN)FirmwareVolumeHeader->HeaderLength);

>> -    CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);

>> +    if (PcdGetBool (PcdIsSecureBoot)) {

>> +      CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);

>> +    } else {

>> +      CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);

>> +    }

>>      VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - FirmwareVolumeHeader->HeaderLength;

>>      VariableStoreHeader->Format            = VARIABLE_STORE_FORMATTED;

>>      VariableStoreHeader->State             = VARIABLE_STORE_HEALTHY;

>> @@ -220,6 +224,7 @@ ValidateFvHeader (

>>      VARIABLE_STORE_HEADER*      VariableStoreHeader;

>>      UINTN                       VariableStoreLength;

>>      UINTN                       FvLength;

>> +    EFI_GUID                    *Guid;

>>  

>>      FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER*)Instance->RegionBaseAddress;

>>  

>> @@ -258,7 +263,12 @@ ValidateFvHeader (

>>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeader + (UINTN)FwVolHeader->HeaderLength);

>>  

>>      // Check the Variable Store Guid

>> -    if ( CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) == FALSE )

>> +    if (PcdGetBool (PcdIsSecureBoot)) {

>> +      Guid = &gEfiAuthenticatedVariableGuid;

>> +    } else {

>> +      Guid = &gEfiVariableGuid;

>> +    }

>> +    if (CompareGuid (&VariableStoreHeader->Signature, Guid) == FALSE)

>>      {

>>          DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Variable Store Guid non-compatible\n"));

>>          return EFI_NOT_FOUND;

>> -- 

>> 2.9.5

>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ming Huang Nov. 20, 2018, 6:44 a.m. | #4
On 11/20/2018 2:19 AM, Ard Biesheuvel wrote:
> On Thu, 15 Nov 2018 at 22:57, Ming Huang <ming.huang@linaro.org> wrote:

>>

>> When SECURE_BOOT_ENABLE is TRUE, FlashFvbDxe should use

>> gEfiAuthenticatedVariableGuid, When SECURE_BOOT_ENABLE

>> is FALSE, gEfiVariableGuid should be used.

>>

> 

> Can we fix the driver instead so we don't need to make this distinction?


Yes, modify it as below patch.
Thank you.

> 

> Please refer to commit 8753858f84768fa6fa17191b86c97538457723ce in the

> EDK2 for some background.

> 

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ming Huang <ming.huang@linaro.org>

>> ---

>>  Silicon/Hisilicon/HisiPkg.dec                         |  1 +

>>  Platform/Hisilicon/D03/D03.dsc                        |  5 +++++

>>  Platform/Hisilicon/D05/D05.dsc                        |  5 +++++

>>  Platform/Hisilicon/D06/D06.dsc                        |  5 +++++

>>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf |  2 ++

>>  Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c   | 14 ++++++++++++--

>>  6 files changed, 30 insertions(+), 2 deletions(-)

>>

>> diff --git a/Silicon/Hisilicon/HisiPkg.dec b/Silicon/Hisilicon/HisiPkg.dec

>> index 404a3ae4af9d..af9359e4d0e0 100644

>> --- a/Silicon/Hisilicon/HisiPkg.dec

>> +++ b/Silicon/Hisilicon/HisiPkg.dec

>> @@ -278,6 +278,7 @@ [PcdsFixedAtBuild]

>>

>>    gHisiTokenSpaceGuid.Pcdsoctype|0|UINT32|0x00000061

>>    gHisiTokenSpaceGuid.PcdSerDesFlowCtrlFlag|0|UINT32|0x40000056

>> +  gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE|BOOLEAN|0x40000058

>>

>>  [PcdsFeatureFlag]

>>    gHisiTokenSpaceGuid.PcdIsItsSupported|FALSE|BOOLEAN|0x00000065

>> diff --git a/Platform/Hisilicon/D03/D03.dsc b/Platform/Hisilicon/D03/D03.dsc

>> index aa1da5d61f83..ba3096672db0 100644

>> --- a/Platform/Hisilicon/D03/D03.dsc

>> +++ b/Platform/Hisilicon/D03/D03.dsc

>> @@ -281,6 +281,11 @@ [PcdsFixedAtBuild.common]

>>    gHisiTokenSpaceGuid.PcdHb0Rb2IoSize|0xffff #64K

>>

>>    gHisiTokenSpaceGuid.Pcdsoctype|0x1610

>> +  !if $(SECURE_BOOT_ENABLE) == TRUE

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

>> +  !else

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

>> +  !endif

>>

>>  ################################################################################

>>  #

>> diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc

>> index 1040466633ef..b8500cef8742 100644

>> --- a/Platform/Hisilicon/D05/D05.dsc

>> +++ b/Platform/Hisilicon/D05/D05.dsc

>> @@ -422,6 +422,11 @@ [PcdsFixedAtBuild.common]

>>    gHisiTokenSpaceGuid.PcdHb1Rb7IoSize|0x10000 #64K

>>

>>    gHisiTokenSpaceGuid.Pcdsoctype|0x1610

>> +  !if $(SECURE_BOOT_ENABLE) == TRUE

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

>> +  !else

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

>> +  !endif

>>

>>  ################################################################################

>>  #

>> diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc

>> index 1a479c160e80..b6ef9fedf0a7 100644

>> --- a/Platform/Hisilicon/D06/D06.dsc

>> +++ b/Platform/Hisilicon/D06/D06.dsc

>> @@ -243,6 +243,11 @@ [PcdsFixedAtBuild.common]

>>

>>    gEfiMdeModulePkgTokenSpaceGuid.PcdSrIovSupport|FALSE

>>    gArmTokenSpaceGuid.PcdPciIoTranslation|0x0

>> +  !if $(SECURE_BOOT_ENABLE) == TRUE

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE

>> +  !else

>> +    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE

>> +  !endif

>>

>>  ################################################################################

>>  #

>> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf

>> index f8be4741ef7c..47965a707032 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

>>

>> @@ -62,6 +63,7 @@ [Pcd.common]

>>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize

>>

>>    gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked

>> +  gHisiTokenSpaceGuid.PcdIsSecureBoot

>>    gHisiTokenSpaceGuid.PcdSFCMEM0BaseAddress

>>

>>  [Depex]

>> diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

>> index e18cc9e06ec2..309941d6fe4d 100644

>> --- a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

>> +++ b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c

>> @@ -189,7 +189,11 @@ InitializeFvAndVariableStoreHeaders (

>>      // VARIABLE_STORE_HEADER

>>      //

>>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)Headers + (UINTN)FirmwareVolumeHeader->HeaderLength);

>> -    CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);

>> +    if (PcdGetBool (PcdIsSecureBoot)) {

>> +      CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);

>> +    } else {

>> +      CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);

>> +    }

>>      VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - FirmwareVolumeHeader->HeaderLength;

>>      VariableStoreHeader->Format            = VARIABLE_STORE_FORMATTED;

>>      VariableStoreHeader->State             = VARIABLE_STORE_HEALTHY;

>> @@ -220,6 +224,7 @@ ValidateFvHeader (

>>      VARIABLE_STORE_HEADER*      VariableStoreHeader;

>>      UINTN                       VariableStoreLength;

>>      UINTN                       FvLength;

>> +    EFI_GUID                    *Guid;

>>

>>      FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER*)Instance->RegionBaseAddress;

>>

>> @@ -258,7 +263,12 @@ ValidateFvHeader (

>>      VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeader + (UINTN)FwVolHeader->HeaderLength);

>>

>>      // Check the Variable Store Guid

>> -    if ( CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) == FALSE )

>> +    if (PcdGetBool (PcdIsSecureBoot)) {

>> +      Guid = &gEfiAuthenticatedVariableGuid;

>> +    } else {

>> +      Guid = &gEfiVariableGuid;

>> +    }

>> +    if (CompareGuid (&VariableStoreHeader->Signature, Guid) == FALSE)

>>      {

>>          DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Variable Store Guid non-compatible\n"));

>>          return EFI_NOT_FOUND;

>> --

>> 2.9.5

>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/Silicon/Hisilicon/HisiPkg.dec b/Silicon/Hisilicon/HisiPkg.dec
index 404a3ae4af9d..af9359e4d0e0 100644
--- a/Silicon/Hisilicon/HisiPkg.dec
+++ b/Silicon/Hisilicon/HisiPkg.dec
@@ -278,6 +278,7 @@  [PcdsFixedAtBuild]
 
   gHisiTokenSpaceGuid.Pcdsoctype|0|UINT32|0x00000061
   gHisiTokenSpaceGuid.PcdSerDesFlowCtrlFlag|0|UINT32|0x40000056
+  gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE|BOOLEAN|0x40000058
 
 [PcdsFeatureFlag]
   gHisiTokenSpaceGuid.PcdIsItsSupported|FALSE|BOOLEAN|0x00000065
diff --git a/Platform/Hisilicon/D03/D03.dsc b/Platform/Hisilicon/D03/D03.dsc
index aa1da5d61f83..ba3096672db0 100644
--- a/Platform/Hisilicon/D03/D03.dsc
+++ b/Platform/Hisilicon/D03/D03.dsc
@@ -281,6 +281,11 @@  [PcdsFixedAtBuild.common]
   gHisiTokenSpaceGuid.PcdHb0Rb2IoSize|0xffff #64K
 
   gHisiTokenSpaceGuid.Pcdsoctype|0x1610
+  !if $(SECURE_BOOT_ENABLE) == TRUE
+    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE
+  !else
+    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE
+  !endif
 
 ################################################################################
 #
diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc
index 1040466633ef..b8500cef8742 100644
--- a/Platform/Hisilicon/D05/D05.dsc
+++ b/Platform/Hisilicon/D05/D05.dsc
@@ -422,6 +422,11 @@  [PcdsFixedAtBuild.common]
   gHisiTokenSpaceGuid.PcdHb1Rb7IoSize|0x10000 #64K
 
   gHisiTokenSpaceGuid.Pcdsoctype|0x1610
+  !if $(SECURE_BOOT_ENABLE) == TRUE
+    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE
+  !else
+    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE
+  !endif
 
 ################################################################################
 #
diff --git a/Platform/Hisilicon/D06/D06.dsc b/Platform/Hisilicon/D06/D06.dsc
index 1a479c160e80..b6ef9fedf0a7 100644
--- a/Platform/Hisilicon/D06/D06.dsc
+++ b/Platform/Hisilicon/D06/D06.dsc
@@ -243,6 +243,11 @@  [PcdsFixedAtBuild.common]
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdSrIovSupport|FALSE
   gArmTokenSpaceGuid.PcdPciIoTranslation|0x0
+  !if $(SECURE_BOOT_ENABLE) == TRUE
+    gHisiTokenSpaceGuid.PcdIsSecureBoot|TRUE
+  !else
+    gHisiTokenSpaceGuid.PcdIsSecureBoot|FALSE
+  !endif
 
 ################################################################################
 #
diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
index f8be4741ef7c..47965a707032 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
 
@@ -62,6 +63,7 @@  [Pcd.common]
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
 
   gArmPlatformTokenSpaceGuid.PcdNorFlashCheckBlockLocked
+  gHisiTokenSpaceGuid.PcdIsSecureBoot
   gHisiTokenSpaceGuid.PcdSFCMEM0BaseAddress
 
 [Depex]
diff --git a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
index e18cc9e06ec2..309941d6fe4d 100644
--- a/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
+++ b/Silicon/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.c
@@ -189,7 +189,11 @@  InitializeFvAndVariableStoreHeaders (
     // VARIABLE_STORE_HEADER
     //
     VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)Headers + (UINTN)FirmwareVolumeHeader->HeaderLength);
-    CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);
+    if (PcdGetBool (PcdIsSecureBoot)) {
+      CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);
+    } else {
+      CopyGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid);
+    }
     VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) - FirmwareVolumeHeader->HeaderLength;
     VariableStoreHeader->Format            = VARIABLE_STORE_FORMATTED;
     VariableStoreHeader->State             = VARIABLE_STORE_HEALTHY;
@@ -220,6 +224,7 @@  ValidateFvHeader (
     VARIABLE_STORE_HEADER*      VariableStoreHeader;
     UINTN                       VariableStoreLength;
     UINTN                       FvLength;
+    EFI_GUID                    *Guid;
 
     FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER*)Instance->RegionBaseAddress;
 
@@ -258,7 +263,12 @@  ValidateFvHeader (
     VariableStoreHeader = (VARIABLE_STORE_HEADER*)((UINTN)FwVolHeader + (UINTN)FwVolHeader->HeaderLength);
 
     // Check the Variable Store Guid
-    if ( CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) == FALSE )
+    if (PcdGetBool (PcdIsSecureBoot)) {
+      Guid = &gEfiAuthenticatedVariableGuid;
+    } else {
+      Guid = &gEfiVariableGuid;
+    }
+    if (CompareGuid (&VariableStoreHeader->Signature, Guid) == FALSE)
     {
         DEBUG ((EFI_D_ERROR, "ValidateFvHeader: Variable Store Guid non-compatible\n"));
         return EFI_NOT_FOUND;