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

[Ming Huang <ming.huang@linaro.org>]

On 11/20/2018 8:58 PM, Leif Lindholm wrote:
> 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.

The bug is that gEfiAuthenticatedVariableGuid should be used in FlashFvbDxe,
not gEfiVariableGuid when enable secure boot.

> 
>> 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?

My original idea is using OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.
There is not enough time to implement a real UserPhysicalPresent.
This patch will add when we implement real secure boot in future.

Thanks.

> 
> 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
>>>>