Ard, Thanks for your confirmation on ArmVirtQemu(I did not do it as I have no the environment, the result makes me more confident about the quality of patchset) and your answer to Laszlo.
Laszlo, Answer your question below. > 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? [Star] Yes > > What happens if the GUID found matches neither of the two well-known GUIDs? [Star] The varstore will be recognized as invalid. Please see the original GetVariableStoreStatus() in MdeModulePkg\Universal\Variable\Pei\Variable.c, MdeModulePkg\Universal\Variable\RuntimeDxe\Variable.c, SecurityPkg\VariableAuthenticated\Pei\Variable.c and SecurityPkg\VariableAuthenticated\RuntimeDxe\Variable.c. Originally, MdeModulePkg\Universal\Variable\Pei\Variable.c and MdeModulePkg\Universal\Variable\RuntimeDxe\Variable.c only covered gEfiVariableGuid, SecurityPkg\VariableAuthenticated\Pei\Variable.c and SecurityPkg\VariableAuthenticated\RuntimeDxe\Variable.c only covered gEfiAuthenticatedVariableGuid. Then the merged MdeModulePkg\Universal\Variable\Pei\Variable.c and MdeModulePkg\Universal\Variable\RuntimeDxe\Variable.c will cover both gEfiVariableGuid and gEfiAuthenticatedVariableGuid. Another, ok I will try to refine the patch about the library class resolutions you concerned and git bisect Ard concerned. Wait a moment, I will send the V2 patch serial. Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] Sent: Thursday, June 25, 2015 10:44 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 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
