On 25 June 2015 at 16:29, Laszlo Ersek <ler...@redhat.com> wrote:
> 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?
>
As far as I can tell, it knows about gEfiVariableGuid and
gEfiAuthenticatedVariableGuid, and the presence of the latter results
in the varstore being treated as an authenticated varstore. That all
looks fine to me.
I pulled these changes and tried building and running ArmVirtQemu.dsc
both with SECURE_BOOT_ENABLE = TRUE and SECURE_BOOT_ENABLE = FALSE,
and both seem to work fine.
--
Ard.
>>
>> !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
