On 06/25/15 14:59, Zeng, Star wrote:
> Ard & Laszlo,
>
> Variable store header is initialized by platform dsc, the merged
> variable driver will AUTOMATICALLY compare EFI_VARIABLE_GUID/
> EFI_AUTHENTICATED_VARIABLE_GUID against
> VARIABLE_STORE_HEADER.Signature to know the format of variable
> (VARIABLE_HEADER or AUTHENTICATED_VARIABLE_HEADER(In fact, it is
> VARIABLE_HEADER in AuthenticatedVariableFormat.h)). There is no break
> to SECURE_BOOT_ENABLE = FALSE case. For example in VarStore.fdf.inc
Okay -- so this means that the merged variable driver does not *expect*
a specific GUID in the varstore, but *detects* whatever is there, and
acts correspondingly. Is that right?
What happens if the GUID found matches neither of the two well-known GUIDs?
>
> !if $(SECURE_BOOT_ENABLE) == TRUE
> # Signature: gEfiAuthenticatedVariableGuid =
> # { 0xaaf32c78, 0x947b, 0x439a,
> # { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
> 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
> 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
> !else
> # Signature: gEfiVariableGuid =
> # { 0xddcf3616, 0x3275, 0x4164,
> # { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }}
> 0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,
> 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,
> !endif
>
> AuthVariableLib will not to touch variable storage format(variable store
> header or variable header), but only to provide auth services that follow
> UEFI spec.
>
>
> I had been aware DxeTpmMeasurementLib in [LibraryClasses] section. I
> also found BaseCryptLib|.../ RuntimeCryptLib.inf has been in
> [LibraryClasses.common.DXE_RUNTIME_DRIVER] section and
> OpensslLib|.../OpensslLib.inf has been in [LibraryClasses] section.
>
> I just copied them to MdeModulePkg/.../VariableRuntimeDxe.inf from
> original SecurityPkg/.../VariableRuntimeDxe.inf, and then add
> TpmMeasurementLib and AuthVariableLib declaration.
>
> SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableRuntimeDxe.inf {
> <LibraryClasses>
> BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> }
>
> Do you prefer the change to be like below in [LibraryClasses] section?
> !if $(SECURE_BOOT_ENABLE) == TRUE
> PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
>
> TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> + AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> !if $(NETWORK_IP6_ENABLE) == TRUE
> TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
> !endif
> +!else
> +
> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
> +
> AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf
> !endif
We may have two issues here:
- The current library class resolutions may not be "minimal" in the
OVMF DSC files, regardless of this patchset. Any such redundancy
should be reduced first, probably.
- Then, all of the resultant library class resolutions should be
considered / updated by this patchset.
Please feel free to attack the first issue as well. If you prefer, I can
add that task to my queue, but it's going to take long...
Thanks
Laszlo
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, June 25, 2015 7:45 PM
> To: Laszlo Ersek
> Cc: Zeng, Star; edk2-devel@lists.sourceforge.net; Justen, Jordan L
> Subject: Re: [edk2] [PATCH 00/20] Separate auth variable service from Auth
> Variable driver.
>
> On 25 June 2015 at 13:15, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 06/25/15 11:46, Star Zeng wrote:
>>> For your easy review,
>>> the forked code is at g...@github.com:lzeng14/edk2.git branch
>>> SeparateAuthVariableService.
>>>
>>> What to do:
>>> 1. Move TpmMeasurementLib LibraryClass from SecurityPkg to MdeModulePkg.
>>> 2. Implement a NULL TpmMeasurementLib library instance in MdeModulePkg.
>>> 3. Move authenticated variable definition from
>>> AuthenticatedVariableFormat.h to VariableFormat.h.
>>> 4. Merge VariableInfo in SecurityPkg to VariableInfo in MdeModulePkg.
>>> 5. Merge from VariablePei in SecurityPkg to VariablePei in MdeModulePkg.
>>> 6. Add AuthVariableLib LibraryClass definitions in MdeModulePkg.
>>> 7. Implement a NULL AuthVariableLib library instance in MdeModulePkg.
>>> 8. Implement AuthVariableLib library instance in SecurityPkg.
>>> 9. Merge from Auth Variable driver in SecurityPkg to Variable drive in
>>> MdeModulePkg.
>>> 10. Update platform package to use the merged Variable driver.
>>>
>>> Why to do:
>>> 1. Share code.
>>> We are moving forward to separate auth variable service from Auth
>>> Variable driver in SecurityPkg to AuthVariableLib. Then the
>>> AuthVariableLib could benefit and be used by different implementation of
>>> Auth Variable drivers.
>>> 2. Remove code duplication and reduce maintenance effort.
>>> 2.1. After auth variable service separated from Auth Variable driver
>>> in SecurityPkg to AuthVariableLib. The remaining code logic of Auth
>>> Variable driver in SecurityPkg will be almost same with Variable
>>> driver in MdeModulePkg. We are going to merge them.
>>> 2.2. The functionality of VariableInfo in SecurityPkg has covered
>>> VariableInfo in MdeModulePkg.
>>> 2.3. The code logic of VariablePei in SecurityPkg is same with
>>> VariablePei in MdeModulePkg.
>>> 3. TpmMeasurementLib is consumed by Auth Variable driver in
>>> SecurityPkg now, as Auth Variable driver in SecurityPkg will be
>>> merged to Variable driver in MdeModulePkg, so the library class also needs
>>> to be moved to MdeModulePkg.
>>> 4. gEfiAuthenticatedVariableGuid will be used by both merged Variable
>>> driver and AuthVariableLib, AUTHENTICATED_VARIABLE_HEADER will be
>>> used by merged Variable driver.
>>>
>>> What test done:
>>> Nt32: Boot with SECURE_BOOT_ENABLE = TRUE or FALSE, enable secure boot with
>>> SECURE_BOOT_ENABLE = TRUE.
>>> OVMF: Boot with SECURE_BOOT_ENABLE = TRUE or FALSE, enable secure boot with
>>> SECURE_BOOT_ENABLE = TRUE.
>>> Vlv2TbltDevice: Boot and enable secure boot with SECURE_BOOT_ENABLE = TRUE.
>>>
>>> What is the impact to platform:
>>> 1. Only platform dsc and fdf need to be updated except the change in
>>> ArmPlatformPkg.dec and NorFlashAuthenticatedDxe.inf to remove
>>> gVariableAuthenticatedRuntimeDxeFileGuid and use
>>> gVariableRuntimeDxeFileGuid.
>>
>> Since Ard added secure boot support to ArmVirtPkg, I will let him
>> review patch 19/20.
>>
>> With regard to OVMF, I have a question, and some notes.
>>
>> * Question: In point 4 above, do I understand correctly that the
>> varstore is now supposed to have only one GUID, the authenticated one,
>> regardless of whether secure boot is enabled in the build or not?
>>
>> If that's the case, then it's problematic.
>>
>> First, technically, "OvmfPkg/VarStore.fdf.inc" will be broken for the
>> SECURE_BOOT_ENABLE=FALSE case, because this FDF include file will
>> build a varstore template that will be then rejected by the variable driver.
>>
>> Second, such an update will break all preexistent virtual machines
>> (and, well, physical machines too) that use firmware with
>> SECURE_BOOT_ENABLE=FALSE. Their current varstores contain the
>> non-authenticated GUID, and after a firmware upgrade (despite
>> *keeping* SECURE_BOOT_ENABLE=FALSE), those varstores will be rejected.
>> (If I understand right.)
>>
>> I think that the new, merged, variable driver should be parametrized,
>> with regard to the varstore GUID. The driver should take a fixed
>> pointer PCD, to a 16 byte array, specifying the GUID, and DSC files
>> should be able to specify that PCD, based on SECURE_BOOT_ENABLE.
>>
>
> This should probably be moved to AuthVariableLib, i.e., it should have a
> function that returns the appropriate GUID for non-authenticated resp
> authenticated variable stores.
>
>
>> * Notes (for patch 16/20):
>>
>> Please grep OvmfPkg for all instances of SECURE_BOOT_ENABLE, and adapt
>> them all as appropriate.
>>
>> The patch currently adds one new conditional and removes one in each
>> DSC file, and modifies one conditional in each FDF file.
>>
>> That's not good enough. Beyond the VarStore.fdf.inc issue mentioned
>> above, each DSC file contains 8 "SECURE_BOOT_ENABLE" conditionals, and
>> *some* of those overlap with your patch. For example, the
>>
>> TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasu
>> TpmMeasurementLib|rementLib.inf
>>
>> library resolution is already present elsewhere.
>>
>> So please investigate all current SECURE_BOOT_ENABLE conditionals
>> under OvmfPkg/, and update them / simplify them all as appropriate.
>>
>> Thanks
>> Laszlo
------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors
network devices and physical & virtual servers, alerts via email & sms
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
