Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
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. > 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. OK, I will send the patch in next series. Thanks. > > / > Leif > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
On Tue, Nov 20, 2018 at 11:00:49PM +0800, Ming Huang wrote: > 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. OK, but if we don't have authenticated variables (all the way to the hardware), then this is the correct behaviour? Making the test pass anyway is not the correct solution. / Leif ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
On 11/20/18 16:00, Ming Huang wrote:
>
>
> 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.
Side comment (because the main comment is Ard's): regarding the
Signature GUID in VARIABLE_STORE_HEADER, commit d92eaabefbe0 ("OvmfPkg:
simplify VARIABLE_STORE_HEADER generation", 2016-02-15) might provide
some background.
Thanks,
Laszlo
> 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
>>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
On 11/20/18 15:40, Ard Biesheuvel wrote: > On Tue, 20 Nov 2018 at 15:30, Ming Huang wrote: >> >> >> >> 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. >> > > I think it is a terrible idea to enable secure boot now in an insecure > manner, and enable 'real' secure boot later. > > Note that it is impossible to implement secure boot in a secure manner > using the generic VariableRuntimeDxe. The crypto routines that perform > the authentication are located in EfiRuntimeServicesCode memory > regions, which are writable by the OS, and so any exploit on the OS > side can modify that code to defeat the checks. Also, the SPI flash > that backs the variable store is accessible by the OS directly. > > That means a proper secure boot implementation will not be based on > any of the components in use currently, and so enabling it does > nothing except confuse people or give them a false sense of security. > If this is based on OS or firmware test results, please disregard > those - this is a tick the box mentality that is wholly incompatible > with building secure systems. > Yup. Thanks, Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
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 > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
On Tue, 20 Nov 2018 at 15:30, Ming Huang wrote: > > > > 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. > I think it is a terrible idea to enable secure boot now in an insecure manner, and enable 'real' secure boot later. Note that it is impossible to implement secure boot in a secure manner using the generic VariableRuntimeDxe. The crypto routines that perform the authentication are located in EfiRuntimeServicesCode memory regions, which are writable by the OS, and so any exploit on the OS side can modify that code to defeat the checks. Also, the SPI flash that backs the variable store is accessible by the OS directly. That means a proper secure boot implementation will not be based on any of the components in use currently, and so enabling it does nothing except confuse people or give them a false sense of security. If this is based on OS or firmware test results, please disregard those - this is a tick the box mentality that is wholly incompatible with building secure systems. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
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. > >> 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
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
---
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 (>Signature, );
+CopyGuid (>Signature,
);
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 (>Signature, )
== FALSE )
+if (!CompareGuid (>Signature, )
&&
+!CompareGuid (>Signature,
))
{
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
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
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
> >> ---
> >> 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 (>Signature, );
> >> +CopyGuid (>Signature,
> >> );
> >> 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 (>Signature, )
> >> == FALSE )
> >> +if (!CompareGuid (>Signature, )
> >> &&
> >> +!CompareGuid (>Signature,
> >> ))
> >> {
> >> 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
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
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. Should I add a patch before this patch
to solve build error with -D SECURE_BOOT_ENABLE=TRUE in v4?
Thanks.
>
> /
> Leif
>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ming Huang
>> ---
>> 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 (>Signature, );
>> +CopyGuid (>Signature,
>> );
>> 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 (>Signature, )
>> == FALSE )
>> +if (!CompareGuid (>Signature, ) &&
>> +!CompareGuid (>Signature,
>> ))
>> {
>> 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
Re: [edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
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?
/
Leif
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ming Huang
> ---
> 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 (>Signature, );
> +CopyGuid (>Signature,
> );
> 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 (>Signature, ) ==
> FALSE )
> +if (!CompareGuid (>Signature, ) &&
> +!CompareGuid (>Signature,
> ))
> {
> 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
[edk2] [PATCH edk2-platforms v3 1/5] Hisilicon/D0x: Fix secure boot bug in FlashFvbDxe
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.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ming Huang
---
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 (>Signature, );
+CopyGuid (>Signature, );
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 (>Signature, ) ==
FALSE )
+if (!CompareGuid (>Signature, ) &&
+!CompareGuid (>Signature,
))
{
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
