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 10:39 PM, Leif Lindholm wrote:
> On Tue, Nov 20, 2018 at 10:29:57PM +0800, Ming Huang wrote:
>>>>> 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.
> 
> OK, I will ask a third time: what _problem_ does this solve?
> What is the symptom?
> When someone uses the buggy firmware, what does not work for them?
> This information _always_ needs to be in the commit message.

This patch is using with series v1 patch 'Hisilicon/D06: Fix SBBR-SCT AuthVar issue'
to fix the SCT issue:
RT.SetVariable - Set Invalid Time Base Auth Variable – FAILURE;
RT.SetVariable - Create one Time Base Auth Variable, the expect return
status should be EFI_SUCCESS – FAILURE.

> 
>>>> 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.
> 
> If there is not enough time to implement a real PlatformSecureLib,
> there is no need to have Secure Boot at all. Same thing goes for
> secure variable store (to hardware devices that are not accessible
> from Non-secure world).
> 
>> This patch will add when we implement real secure boot in future.
> 
> That sounds like the best thing to do.
> 
> Meanwhile, could you create a patch to get rid of the SECURE_BOOT
> options completely from the .dsc/.fdf please? I don't like having it
> in there when we know it doesn't build.
> 
> /
>     Leif
>