Re: [edk2] [PATCH v2 1/2] Maintainers: add TPM2 reviewers for OvmfPkg
Hi On Thu, Feb 21, 2019 at 1:28 PM Laszlo Ersek wrote: > > OVMF can be built with a significant amount of TPM2 code now; add > Marc-André and Stefan as Reviewers for TPM2-related patches. > > Keep the list of "R" entries alphabetically sorted. > > Cc: Andrew Fish > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Leif Lindholm > Cc: Marc-André Lureau > Cc: Michael D Kinney > Cc: Stefan Berger > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > Reviewed-by: Leif Lindholm > Reviewed-by: Stefan Berger > Acked-by: Ard Biesheuvel > Acked-by: Jordan Justen Reviewed-by: Marc-André Lureau > --- > > Notes: > v2: > - pick up feedback tags > - describe the Reviewership scope, in the format seen under MdeModulePkg > [Marc-André] > > Maintainers.txt | 4 > 1 file changed, 4 insertions(+) > > diff --git a/Maintainers.txt b/Maintainers.txt > index 3b2676bc32ea..e45b77039d67 100644 > --- a/Maintainers.txt > +++ b/Maintainers.txt > @@ -218,6 +218,10 @@ M: Laszlo Ersek > M: Ard Biesheuvel > R: Anthony Perard > R: Julien Grall > +R: Marc-André Lureau > + (TPM2 modules) > +R: Stefan Berger > + (TPM2 modules) > S: Maintained > > PcAtChipsetPkg > -- > 2.19.1.3.g30247aa5d201 > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v2] OvmfPkg: Add TCG2 Configuration menu to the Device Manager menu
Hi On Fri, Jan 25, 2019 at 10:30 PM Stefan Berger wrote: > > This patch adds the TCG2 Configuration menu to the Device Manager > menu. We can apparently reuse the sample Tcg2ConfigDxe from > SecurityPkg/Tcg/Tcg2Config without obvious adverse effects. The > added TCG2 Configuration menu now shows details about the attached > TPM 2.0 and lets one for example configure the active PCR banks > or issue commands, among other things. > > The code is added to Ovmf by building with -DTPM2_ENABLE and > -DTPM2_CONFIG_ENABLE. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Stefan Berger patch looks good (I have done minimal testing, though). Reviewed-by: Marc-André Lureau > --- > OvmfPkg/OvmfPkgIa32.dsc| 4 > OvmfPkg/OvmfPkgIa32.fdf| 3 +++ > OvmfPkg/OvmfPkgIa32X64.dsc | 4 > OvmfPkg/OvmfPkgIa32X64.fdf | 3 +++ > OvmfPkg/OvmfPkgX64.dsc | 4 > OvmfPkg/OvmfPkgX64.fdf | 3 +++ > 6 files changed, 21 insertions(+) > > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index aee19b75d7..2b642ab5dc 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -40,6 +40,7 @@ >DEFINE SMM_REQUIRE = FALSE >DEFINE TLS_ENABLE = FALSE >DEFINE TPM2_ENABLE = FALSE > + DEFINE TPM2_CONFIG_ENABLE = FALSE > ># ># Flash size selection. Setting FD_SIZE_IN_KB on the command line directly > to > @@ -632,6 +633,9 @@ > > NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf > > NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf >} > +!if $(TPM2_CONFIG_ENABLE) == TRUE > + SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf > +!endif > !endif > ># > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index e013099136..4999403ad7 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -381,6 +381,9 @@ INF > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > !if $(TPM2_ENABLE) == TRUE > INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf > +!if $(TPM2_CONFIG_ENABLE) == TRUE > +INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf > +!endif > !endif > > > > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 90cbd8e341..14a5c1bb29 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -40,6 +40,7 @@ >DEFINE SMM_REQUIRE = FALSE >DEFINE TLS_ENABLE = FALSE >DEFINE TPM2_ENABLE = FALSE > + DEFINE TPM2_CONFIG_ENABLE = FALSE > ># ># Flash size selection. Setting FD_SIZE_IN_KB on the command line directly > to > @@ -640,6 +641,9 @@ > > NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf > > NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf >} > +!if $(TPM2_CONFIG_ENABLE) == TRUE > + SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf > +!endif > !endif > > [Components.X64] > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index afaa334384..d0cc107928 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -388,6 +388,9 @@ INF > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf > > !if $(TPM2_ENABLE) == TRUE > INF SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf > +!if $(TPM2_CONFIG_ENABLE) == TRUE > +INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf > +!endif > !endif > > > > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 83d16eb00b..aa7197f533 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -40,6 +40,7 @@ >DEFINE SMM_REQUIRE = FALSE >DEFINE TLS_ENABLE = FALSE >DEFINE TPM2_ENABLE = FALSE > + DEFINE TPM2_CONFIG_ENABLE = FALSE > ># ># Flash size selection. Setting FD_SIZE_IN_KB on the command line directly > to > @@ -639,6 +640,9 @@ > > NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf > > NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf >} > +!if $(TPM2_CONFIG_ENABLE) == TRUE > + SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf > +!endif > !endif > ># > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index afaa334384..d0cc107928 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/
Re: [edk2] [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
Hi On Tue, Oct 2, 2018 at 4:19 PM Laszlo Ersek wrote: > > On 10/02/18 14:10, Marc-André Lureau wrote: > > Hi > > > > On Tue, Oct 2, 2018 at 3:55 PM Laszlo Ersek wrote: > >> > >> On 10/02/18 13:37, Marc-André Lureau wrote: > >>> On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek wrote: > >> > >>>> Please fix your git settings for your local edk2 clone. Your patch > >>>> contains context lines with LF (not CRLF) line endings, and that's not > >>>> correct for edk2. > >>> > >>> Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ? > >> > >> Yes, it should be caught by "PatchCheck.py". The "force_crlf" member is > >> set iff the pathname doesn't end in ".sh", and then the > >> check_added_line() method verifies CRLF. > >> > >> I don't know why it doesn't work in practice. Can you submit a TianoCore > >> BZ about it? > >> > >> (Your patch uses LF on both context lines and new (added) lines, so the > >> current script logic should complain, yes.) > >> > >>> Hmm, this is weird, the original patch has crlf (see attach file). > >>> send-email or the mail server somehow stripped it? > >>> > >>> I use your recommend git settings, I don't know what I am missing here. > >> > >> You are right, the attachment looks fine. > >> > >> ... Can you resend the v2 patch (just to me directly, off-list) with > >> > >> --transfer-encoding=quoted-printable > >> > >> ? My guess is that the base64 encoding in git-send-email includes an > >> automatic CR stripping phase. > > > > Hmm, actually it's my emacs settings that stripped the crlf from the patch. > > > > I edited the patch manually to add some notes... > > Haha, serves you right then ;) > > I suggest never editing patches after formatting them. In the first > place, there's no reason to: you can add, edit and remove notes on > commits with "git notes" (without changing the commit hashes / git > history). And "git-format-patch" takes a "--notes" option. (Same for > "git show".) > > I do mention git-notes in the "unkempt guide": > > git config notes.rewriteRef refs/notes/commits > > git format-patch \ > --notes \ > > git notes edit COMMIT_HASH_OF_THAT_PATCH > Yes, somehow git-notes doesn't fit with my idea of how patch mail notes for v2/v3... should look: Notes: v3: - foo v2: - blablah If there is a way to avoid the "Notes:" quoting, that would be nice. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
Hi On Tue, Oct 2, 2018 at 3:55 PM Laszlo Ersek wrote: > > On 10/02/18 13:37, Marc-André Lureau wrote: > > On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek wrote: > > >> Please fix your git settings for your local edk2 clone. Your patch > >> contains context lines with LF (not CRLF) line endings, and that's not > >> correct for edk2. > > > > Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ? > > Yes, it should be caught by "PatchCheck.py". The "force_crlf" member is > set iff the pathname doesn't end in ".sh", and then the > check_added_line() method verifies CRLF. > > I don't know why it doesn't work in practice. Can you submit a TianoCore > BZ about it? > > (Your patch uses LF on both context lines and new (added) lines, so the > current script logic should complain, yes.) > > > Hmm, this is weird, the original patch has crlf (see attach file). > > send-email or the mail server somehow stripped it? > > > > I use your recommend git settings, I don't know what I am missing here. > > You are right, the attachment looks fine. > > ... Can you resend the v2 patch (just to me directly, off-list) with > > --transfer-encoding=quoted-printable > > ? My guess is that the base64 encoding in git-send-email includes an > automatic CR stripping phase. Hmm, actually it's my emacs settings that stripped the crlf from the patch. I edited the patch manually to add some notes... > > If indeed bas64 is the culprit, I'll update the wiki article to specify > quoted-printable. > > Thanks > Laszlo > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 1/1] OvmfPkg/PlatformPei: clear CPU caches
Hi On Tue, Oct 2, 2018 at 2:55 PM Laszlo Ersek wrote: > > Hi Marc-André, > > On 10/02/18 10:36, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > This is for conformance with the TCG "Platform Reset Attack Mitigation > > Specification". Because clearing the CPU caches at boot doesn't impact > > performance significantly, do it unconditionally, for simplicity's > > sake. > > > > Flush the cache on all logical processors, thanks to > > EFI_PEI_MP_SERVICES_PPI and CacheMaintenanceLib. > > > > Cc: Anthony Perard > > Cc: Ard Biesheuvel > > Cc: Jordan Justen > > Cc: Julien Grall > > Cc: Kinney Michael D > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Marc-André Lureau > > --- > > > > v2: > > - use CacheMaintenanceLib, instead of WBINVD usage > > - rename OnMpServicesAvailable->ClearCacheOnMpServicesAvailable > > - commit message & comments update > > > > OvmfPkg/PlatformPei/PlatformPei.inf | 2 + > > OvmfPkg/PlatformPei/Platform.h | 5 + > > OvmfPkg/PlatformPei/ClearCache.c| 113 > > OvmfPkg/PlatformPei/Platform.c | 1 + > > 4 files changed, 121 insertions(+) > > Please fix your git settings for your local edk2 clone. Your patch > contains context lines with LF (not CRLF) line endings, and that's not > correct for edk2. Shouldn't it be catched by: python BaseTools/Scripts/PatchCheck.py -1 ? > > I decoded your base64-encoded patch email manually, hence my statement > about the line endings. Here's a snippet from > "OvmfPkg/PlatformPei/PlatformPei.inf": > > 0d90 64 69 66 66 20 2d 2d 67 69 74 20 61 2f 4f 76 6d |diff --git a/Ovm| > 0da0 66 50 6b 67 2f 50 6c 61 74 66 6f 72 6d 50 65 69 |fPkg/PlatformPei| > 0db0 2f 50 6c 61 74 66 6f 72 6d 50 65 69 2e 69 6e 66 |/PlatformPei.inf| > 0dc0 20 62 2f 4f 76 6d 66 50 6b 67 2f 50 6c 61 74 66 | b/OvmfPkg/Platf| > 0dd0 6f 72 6d 50 65 69 2f 50 6c 61 74 66 6f 72 6d 50 |ormPei/PlatformP| > 0de0 65 69 2e 69 6e 66 0a 69 6e 64 65 78 20 39 63 35 |ei.inf.index 9c5| > 0df0 61 64 39 39 36 31 63 34 61 2e 2e 35 63 38 64 64 |ad9961c4a..5c8dd| > 0e00 30 66 65 36 64 37 32 20 31 30 30 36 34 34 0a 2d |0fe6d72 100644.-| > 0e10 2d 2d 20 61 2f 4f 76 6d 66 50 6b 67 2f 50 6c 61 |-- a/OvmfPkg/Pla| > 0e20 74 66 6f 72 6d 50 65 69 2f 50 6c 61 74 66 6f 72 |tformPei/Platfor| > 0e30 6d 50 65 69 2e 69 6e 66 0a 2b 2b 2b 20 62 2f 4f |mPei.inf.+++ b/O| > 0e40 76 6d 66 50 6b 67 2f 50 6c 61 74 66 6f 72 6d 50 |vmfPkg/PlatformP| > 0e50 65 69 2f 50 6c 61 74 66 6f 72 6d 50 65 69 2e 69 |ei/PlatformPei.i| > 0e60 6e 66 0a 40 40 20 2d 33 30 2c 36 20 2b 33 30 2c |nf.@@ -30,6 +30,| > 0e70 37 20 40 40 0a 20 0a 20 5b 53 6f 75 72 63 65 73 |7 @@. . [Sources| > 0e80 5d 0a 20 20 20 41 6d 64 53 65 76 2e 63 0a 2b 20 |]. AmdSev.c.+ | > 0e90 20 43 6c 65 61 72 43 61 63 68 65 2e 63 0a 20 20 | ClearCache.c. | > 0ea0 20 43 6d 6f 73 2e 63 0a 20 20 20 43 6d 6f 73 2e | Cmos.c. Cmos.| > 0eb0 68 0a 20 20 20 46 65 61 74 75 72 65 43 6f 6e 74 |h. FeatureCont| > 0ec0 72 6f 6c 2e 63 0a 40 40 20 2d 35 34 2c 36 20 2b |rol.c.@@ -54,6 +| > 0ed0 35 35 2c 37 20 40 40 0a 20 0a 20 5b 4c 69 62 72 |55,7 @@. . [Libr| > 0ee0 61 72 79 43 6c 61 73 73 65 73 5d 0a 20 20 20 42 |aryClasses]. B| > 0ef0 61 73 65 4c 69 62 0a 2b 20 20 43 61 63 68 65 4d |aseLib.+ CacheM| > 0f00 61 69 6e 74 65 6e 61 6e 63 65 4c 69 62 0a 20 20 |aintenanceLib. | > > Due to this issue, git-am fails to apply the patch. > > I mentioned this earlier (including a request to push your patches being > submitted to a personal repo/branch as well): > > http://mid.mail-archive.com/f5c15a1e-25c2-c2e4-e6db-374107e3b488@redhat.com > > The wiki article at > > > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers > > explains the git settings in order for git not to hide the CRLFs in the > working tree. In particular, the git concept for (a) representing the > source code internally with LFs, and (b) converting to/from CRLF on > checkout/commit, is *not* to be used. > > Is this edk2 peculiarity broken, considering git standards and other > open source repositories? It sure is. > > Can we change it? No, we can't. Hmm, this is weird, the original patch has crlf (see attach file). send-email or the mail server somehow stripped it? I use your recommend git settings, I don't know what I am missing here. > > Thanks, > Laszlo > ___
[edk2] TCG MOR and processor caches
Hi, According to "TCG Platform Reset Attack Mitigation Specification", if MOR bit is set, "it must initiate a vendor-specific method that overwrites all of system memory and the processor caches" In QuarkPlatformPkg/Platform/Pei/PlatformInit/MemoryCallback.c and QuarkPlatformPkg/Platform/Pei/PlatformInit/MrcWrapper.c, there is some code to clear RAM, however I don't see code that would clear the processor caches. For edk2/qemu, Paolo suggested it may be simpler to clear the cache unconditionally. How would you implement that? Using EFI_CPU_ARCH_PROTOCOL.FlushDataCache? (or direct AsmWbinvd call) thanks -- Marc-André Lureau ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
Hi On Mon, Aug 6, 2018 at 5:26 PM, Zhang, Chao B wrote: > Hi Ricardo >I double checked OVMF Debug Build. All the 2 PCDs are already built as > Dynamic PCD. There should be no problem > Setting & Getting these PCD as Dynamic. We also verified this feature on > several real hardware platforms with same configuration. > No issue reported. >Can you share me the boot log? > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Friday, August 3, 2018 10:46 PM > To: Ricardo Araújo ; Zhang, Chao B > > Cc: edk2-devel@lists.01.org; Zeng, Star ; Gao, Liming > > Subject: Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 > with OVMF > > On 08/03/18 15:39, Ricardo Araújo wrote: >> Hi folks, sorry for the delay! >> >> I've just applied the patch and got the same error message and empty PCRs. > > Thanks for the feedback -- although it's not the kind I had hoped for :) > > I have now filed "[regression] SecurityPkg commit f15cb995bb38 breaks > TPM2_ENABLE in OvmfPkg": > > https://bugzilla.tianocore.org/show_bug.cgi?id=1075 > > Ricardo, please consider registering in the TianoCore Bugzilla, and > adding yourself to the CC list of BZ#1075. > > For now, I have assigned the BZ to Marc-André, for triaging / analysis. > swtpm is not set up on my end, and the TPM2 enablement for OvmfPkg was > contributed by Marc-André. Marc-André, are you OK with this? The BZ > assignment is about root-causing the issue, at the moment. That fixes the problem for me: - Constructor= Tpm2DeviceLibConstructor + CONSTRUCTOR= Tpm2DeviceLibConstructor It looks to me like the patch "SecurityPkg: Cache TPM interface type info" could use more reviews. Fwiw, I also question why that change (just the line above) was necessary: - LIBRARY_CLASS = Tpm2DeviceLib + LIBRARY_CLASS = Tpm2DeviceLib|PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER > > Once we know more closely what the problem is, we can decide what to do. > If it's hard to fix, my argument will be that we should roll back > SecurityPkg commit f15cb995bb38 first (it's a regression after all), and > re-apply it only when it no longer breaks OVMF. If the issue is not hard > to fix and we can commit the solution quickly, then I'll be fine with > leaving f15cb995bb38 applied. > > Thanks, > Laszlo > >> >> De: "Zhang, Chao B" mailto:chao.b.zh...@intel.com>> >> Para: "Laszlo Ersek" mailto:ler...@redhat.com>>, "Ricardo >> Araújo" mailto:rica...@lsd.ufcg.edu.br>>, >> "Marc-André Lureau" >> mailto:marcandre.lur...@redhat.com>> >> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>, "Gao, Liming" >> mailto:liming@intel.com>>, "Zeng, Star" >> mailto:star.z...@intel.com>> >> Enviadas: Quinta-feira, 2 de agosto de 2018 21:22:18 >> Assunto: RE: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 >> with OVMF >> >> >> >> Tks Lazslo. And please make sure PcdLib is correctly lined in OVMF >> >> >> >> >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Thursday, August 2, 2018 9:14 PM >> To: Zhang, Chao B mailto:chao.b.zh...@intel.com>>; >> Ricardo Araújo mailto:rica...@lsd.ufcg.edu.br>>; >> Marc-André Lureau >> mailto:marcandre.lur...@redhat.com>> >> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Gao, Liming >> mailto:liming@intel.com>>; Zeng, Star >> mailto:star.z...@intel.com>> >> Subject: Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 >> with OVMF >> >> >> >> >> On 08/02/18 04:04, Zhang, Chao B wrote: >>> Hi Laszlo & Ricardo >>> The patch was intended to reduce the time to read TPM interface ID >>> register. The interface type should not change within boot cycle according >>> to PTP spec. >>> I agree to add some ASSERT after PCDSetxxsS. >>> But It is a core solution without platform change as PCD has been >>> configured as DYN, DYNEx in DEC. I don’t know why you meet Set Failure >>> In OVMF. Here, I include PCD expert to explain this. >> >> As far as I recall, dynamic PCDs have never behaved as actually settable >> for me unless I added dynamic defaults for them in the OVMF DSC files. >> >> I never really researched why this was the case, I just accepted that >> the dynamic defaults were apparently necessary. >>
Re: [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface
Hi On Thu, May 17, 2018 at 9:54 AM, Laszlo Ersek <ler...@redhat.com> wrote: > On 05/15/18 14:30, marcandre.lur...@redhat.com wrote: >> From: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> Hi, >> >> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU >> with TPM2 (I haven't tested TPM1, for lack of interest). >> >> PPI test runs successfully with Windows 10 WHLK, despite the limited >> number of supported funcions (tpm2_ppi_funcs table, in particular, no >> function allows to manipulate Tcg2PhysicalPresenceFlags) >> >> The way it works is relatively simple: a memory region is allocated by >> QEMU to save PPI related variables. An ACPI interface is exposed by >> QEMU to let the guest manipulate those. At boot, ovmf processes and >> updates the PPI qemu region and request variables. >> >> I build edk2 with: >> >> $ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE > > Is -DSECURE_BOOT_ENABLE necessary for *building* with -DTPM2_ENABLE? If > that's the case, we should update the DSC files; users building OVMF > from source shouldn't have to care about "-D" inter-dependencies, if we > can manage that somehow. No, that's only my build setup, because it is likely both will be used together. TPM usage/tests seem to be fine without it. > > If -DSECURE_BOOT_ENABLE is only there because otherwise a guest OS > doesn't really make use of -DTPM2_ENABLE either, that's different. In > that case, it's fine to allow building OVMF with TPM2 support but > without SB support. > > Thanks! > Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface
Hi On Wed, May 16, 2018 at 11:29 AM, Laszlo Ersek <ler...@redhat.com> wrote: > Hi Marc-André, > > On 05/15/18 14:30, marcandre.lur...@redhat.com wrote: >> From: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> Hi, >> >> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU >> with TPM2 (I haven't tested TPM1, for lack of interest). > > I got the review of this patch series added to my TODO list, but I'll > have to ask for your patience. :( > > From an extremely superficial skim: > > * please use the > > TopDirPkg/ModuleName: blah blah blah > > subject format, or more generally, if a module cannot be identified, > > TopDirPkg: blah blah blah > done > * the subject line and the commit message shouldn't be wider than 74 > chars; > that should be ok > * edk2 uses two spaces for general indentation, and I'm noticing some > inconsistency there (4 spaces like in QEMU). yes, I tried to respect that, but sometime fail (emacs c-basic-offset 2 isn't great with comments) > > * Please consider formatting the patches with "--find-copies-harder" > (although I can look at them with the same option after fetching the > series from your repo). This option is usually helpful for reviewers > when cloning and modifying modules cross-package. Hmm, I didn't know that option, ok > > * Please consider adopting the git settings at > > <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>, > in particular: > > - "--stat=1000 --stat-graph-width=20", so that pathnames are not > truncated in the diffstats, > I use git-publish very often. I had to modify it to pass those options (https://github.com/stefanha/git-publish/pull/48) > - the "xfuncname"-related settings, so that git diff hunk headers @@ > are useful for DSC and INF files too, > This is already in my .git/config, I hope it takes it by default in format-patch? > - the diff order file, so that files are listed in patches in logical > order, going from abstract / descriptive (.inf, .h) to concrete / > imperative (.c). > ok > Not much of a review, I know; this is all I can offer right now. If you > have the time to respin just with these superficial changes, that might > make my life easier. If you prefer to delay them, that's 100% fine too. > I am going to resend with the style fixes. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 0/7] ovmf: preliminary TPM2 support
Hi On Fri, Mar 9, 2018 at 2:09 PM, <marcandre.lur...@redhat.com> wrote: > From: Marc-André Lureau <marcandre.lur...@redhat.com> > > Hi, > > The following series adds basic TPM2 support for OVMF-on-QEMU (I > haven't tested TPM1, for lack of interest). It links with the modules > to initializes the device in PEI phase, and do measurements (both PEI > and DXE). The Tcg2Dxe module provides the Tcg2 protocol which allows > the guest to access the measurement log and other facilities. > > DxeTpm2MeasureBootLib seems to do its job at measuring images that are > not measured in PEI phase (such as PCI PXE rom) > > Tcg2ConfigDxe is not included due to its integration with edk2 own PPI > implementation which conflicts with qemu design. PPI design is still > being discussed & experimented at this point. > > Linux guests seem to work fine. But windows guest generally complains > about the lack of PPI interface (most HLK tests require it, tpm.msc > admin interactions too). I haven't done "real" use-cases tests, as I > lack experience with TPM usage. Any help appreciated to test the TPM. > > I build edk2 with: > > $ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE -DMEM_VARSTORE_EMU_ENABLE=FALSE > > I test with qemu & swtpm/libtpms (tpm2 branches, swtpm_setup.sh --tpm2 > --tpm-state tpmstatedir) > > $ swtpm socket --tpmstate tpmstatedir --ctrl type=unixio,path=tpmsock --tpm2 > & > $ qemu .. -chardev socket,id=chrtpm,path=tpmsock -tpmdev > emulator,id=tpm0,chardev=chrtpm -device tpm-crb,tpmdev=tpm0 > > Thanks > > Github tree: > https://github.com/elmarco/edk2/tree/tpm2 (tpm2-v2 tag) I updated the github tree: https://github.com/elmarco/edk2/tree/tpm2 (tpm2-v3 tag) > > Related bug: > https://bugzilla.tianocore.org/show_bug.cgi?id=594 > > v3: after Laszlo review > - many simplifications to "add customized Tcg2ConfigPei clone" patch > - various move of fdf/dsc sections > - modify Ia32 & Ia32x64 fdf/dsc too > - modify commit messages > - add r-b tags > > v2: > - the series can now be applied to master directly, thanks to dropping > PeiReadOnlyVariable requirement > - remove the HOB list workaround, the main fix is now upstream. Add a > preliminary patch to complete it. > - removed traces of TPM1.2 support > - add own OvmfPkg Tcg2ConfigPei, which performs only TPM2 detection > - make PcdTpmInstanceGuid default all-bits-zero > - drop unneeded Pcd values > - explain why SHA1 is still nice to have (for 1.2 log format) > - drop Tcg2ConfigDxe > - more detailed commit messages, thanks to Laszlo explanations! > - rebased > > TODO: > - modify Ia32 and Ia32X64 builds This is now done. thanks > > Marc-André Lureau (7): > SecurityPkg/Tcg2Pei: drop PeiReadOnlyVariable from Depex > MdeModulePkg/Core/Pei: fix REGISITER -> REGISTER typo > OvmfPkg: simplify SecurityStubDxe.inf inclusion > OvmfPkg: add customized Tcg2ConfigPei clone > OvmfPkg: include Tcg2Pei module > OvmfPkg: include Tcg2Dxe module > OvmfPkg: plug DxeTpm2MeasureBootLib into SecurityStubDxe > > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 18 ++-- > MdeModulePkg/Core/Pei/Image/Image.c | 4 +- > MdeModulePkg/Core/Pei/PeiMain.h | 2 +- > MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 2 +- > OvmfPkg/OvmfPkgIa32.dsc | 49 ++- > OvmfPkg/OvmfPkgIa32.fdf | 9 ++ > OvmfPkg/OvmfPkgIa32X64.dsc| 49 ++- > OvmfPkg/OvmfPkgIa32X64.fdf| 9 ++ > OvmfPkg/OvmfPkgX64.dsc| 49 ++- > OvmfPkg/OvmfPkgX64.fdf| 9 ++ > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 53 > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 84 +++ > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 1 - > 13 files changed, 312 insertions(+), 26 deletions(-) > create mode 100644 OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > create mode 100644 OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c > > -- > 2.16.2.346.g9779355e34 > > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel -- Marc-André Lureau ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/8] SecurityPkg/Tcg2Pei: drop PeiReadOnlyVariable from Depex
Hi On Thu, Mar 8, 2018 at 1:36 AM, Zhang, Chao B <chao.b.zh...@intel.com> wrote: > Hi Lureau: >I think we can remove same dependency in TcgPei. > Thanks, feel free to explore that as a separate patch. This is out of scope to me. -- Marc-André Lureau ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/8] RFC: ovmf: preliminary TPM2 support
Hi On Thu, Mar 8, 2018 at 1:31 PM, Shi, Steven <steven@intel.com> wrote: > Hi Marcandre, >> I test with qemu & swtpm/libtpms (tpm2 branches, swtpm_setup.sh --tpm2 >> --tpm-state tpmstatedir) >> $ swtpm socket --tpmstate tpmstatedir --ctrl type=unixio,path=tpmsock >> --tpm2 & > > Where is the swtpm_setup.sh? And could you tell how to build & install the > swtpm? > You need to compile & install libtpms & swtpm : git clone -b tpm2-preview.rev146.v2 https://github.com/stefanberger/libtpms cd libtpms autoreconf -vfi && ./configure --with-tpm2 --with-openssl && make install git clone -b tpm2-preview.v2 https://github.com/stefanberger/swtpm cd swtpm autoreconf -vfi && ./configure --with-openssl && make install Then you can run: mkdir tpmstatedir swtpm_setup.sh --tpm2 --tpm-state tpmstatedir Run the emulator: swtpm socket --tpmstate dir=tpmstatedir --ctrl type=unixio,path=tpmemu.sock --tpm2 Run qemu (from git) with ovmf (with this series): qemu ... -chardev socket,id=chrtpm,path=tpmemu.sock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-crb,tpmdev=tpm0 -drive if=pflash,format=raw,file=OVMF_CODE.fd,readonly -drive if=pflash,format=raw,file=OVMF_VARS.fd .. cheers -- Marc-André Lureau ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 1/1] SecurityPkg: fix ZeroMem HashInterfaceHob
Hi
On Wed, Mar 7, 2018 at 12:24 PM, <marcandre.lur...@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> The ZeroMem() call goes beyond the HashInterfaceHob structure, causing
> HOB list corruption. The intent was to clear all but the Identifier,
> that is starting from HashInterfaceCount.
>
I see v1 is already applied. I'll send another patch to clear the mask
in the Ovmf/TPM series.
Thanks
> Quoting Laszlo Ersek:
> Therefore I think the *first* approach to actually fixing this bug would
> be to simply add the "Count" suffix:
>
> > diff --git
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> > index dbee0f2531bc..a869fffae3fe 100644
> > ---
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> > +++
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> > @@ -424,7 +424,10 @@ HashLibBaseCryptoRouterPeiConstructor (
> > // This is the second execution of this module, clear the hash
> interface
> > // information registered at its first execution.
> > //
> > -ZeroMem (>HashInterface, sizeof
> (*HashInterfaceHob) - sizeof (EFI_GUID));
> > +ZeroMem (
> > + >HashInterfaceCount,
> > + sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)
> > + );
> >}
> >
> >//
>
> On the other hand, I don't think such a fix would be great. First of
> all, the HASH_INTERFACE_HOB struct definition is not wrapped in
>
> #pragma pack (1)
> ...
> #pragma pack ()
>
> in other words, HASH_INTERFACE_HOB is not packed, hence there could be
> an unspecified amount of padding between "Identifier" and
> "HashInterfaceCount". That would lead to the same kind of buffer
> overflow, because "Length" includes the padding, but the "Buffer"
> argument doesn't.
>
> Second, even if there were no padding (and thus the byte count would be
> a perfect match for the full HOB structure), in the "Buffer" argument
> we take the address of the "HashInterfaceCount" field, and with the
> "Length" field, we still overflow that *individual* field. Although we
> wouldn't overflow the containing HOB structure, this is still (arguably)
> undefined behavior -- I seem to remember that some compilers actually
> blow up on this when you use the standard ISO C memcpy() or memset()
> functions in such contexts.
>
> This patch takes Laszlo suggestion to fix the issue.
>
> Cc: Jiewen Yao <jiewen@intel.com>
> Cc: Chao Zhang <chao.b.zh...@intel.com>
> Cc: Star Zeng <star.z...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> ---
> .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git
> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> index dbee0f2531bc..84c1aaae70eb 100644
> --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
> @@ -397,6 +397,7 @@ HashLibBaseCryptoRouterPeiConstructor (
> {
>EFI_STATUSStatus;
>HASH_INTERFACE_HOB*HashInterfaceHob;
> + UINTN ClearStartOffset;
>
>HashInterfaceHob = InternalGetHashInterfaceHob ();
>if (HashInterfaceHob == NULL) {
> @@ -424,7 +425,11 @@ HashLibBaseCryptoRouterPeiConstructor (
> // This is the second execution of this module, clear the hash interface
> // information registered at its first execution.
> //
> -ZeroMem (>HashInterface, sizeof (*HashInterfaceHob) -
> sizeof (EFI_GUID));
> +ClearStartOffset = OFFSET_OF (HASH_INTERFACE_HOB, HashInterfaceCount);
> +ZeroMem (
> + (UINT8 *)HashInterfaceHob + ClearStartOffset,
> + sizeof (*HashInterfaceHob) - ClearStartOffset
> + );
>}
>
>//
> --
> 2.16.2.346.g9779355e34
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
--
Marc-André Lureau
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/1] RFC: SecurityPkg: only clear HashInterface informations
Hi
On Wed, Mar 7, 2018 at 10:06 AM, Laszlo Ersek <ler...@redhat.com> wrote:
> Hi Marc-André,
>
> On 03/06/18 21:27, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lur...@redhat.com>
>>
>> The ZeroMem() call goes beyond the HashInterfaceHob structure, causing
>> HOB list corruption. Instead, just clear the HashInterface fields, as
>> I suppose was originally intended.
>>
>> Cc: Jiewen Yao <jiewen@intel.com>
>> Cc: Chao Zhang <chao.b.zh...@intel.com>
>> Cc: Star Zeng <star.z...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> ---
>> .../HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git
>> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> index dbee0f2531bc..361a4f6508a0 100644
>> ---
>> a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> +++
>> b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c
>> @@ -424,7 +424,8 @@ HashLibBaseCryptoRouterPeiConstructor (
>> // This is the second execution of this module, clear the hash interface
>> // information registered at its first execution.
>> //
>> -ZeroMem (>HashInterface, sizeof (*HashInterfaceHob) -
>> sizeof (EFI_GUID));
>> +ZeroMem (>HashInterface, sizeof
>> (HashInterfaceHob->HashInterface));
>> +HashInterfaceHob->HashInterfaceCount = 0;
>>}
>>
>>//
>>
>
> Excellent catch! :)
>
> I do have a question however, for the other reviewers on this patch, in
> line with your commit message saying, "clear the HashInterface fields,
> as *I suppose* was originally intended".
>
> I don't think the proposed update matches the original intent 100%.
> Namely, this is the definition of the HASH_INTERFACE_HOB type (from the
> same source file):
>
>> typedef struct {
>> //
>> // If gZeroGuid, SupportedHashMask is 0 for FIRST module which consumes
>> HashLib
>> // or the hash algorithm bitmap of LAST module which consumes HashLib.
>> // HashInterfaceCount and HashInterface are all 0.
>> // If gEfiCallerIdGuid, HashInterfaceCount, HashInterface and
>> SupportedHashMask
>> // are the hash interface information of CURRENT module which consumes
>> HashLib.
>> //
>> EFI_GUID Identifier;
>> UINTNHashInterfaceCount;
>> HASH_INTERFACE HashInterface[HASH_COUNT];
>> UINT32 SupportedHashMask;
>> } HASH_INTERFACE_HOB;
>
> Note that it says, if Identifier equals "gEfiCallerIdGuid", then *all
> three* subsequent fields carry information of the current module that
> consumes HashLib. Those three fields include "SupportedHashMask".
>
> Furthermore, the comment just preceding the bug / ZeroMem() call says,
>
>> //
>> // In PEI phase, some modules may call RegisterForShadow and will be
>> // shadowed and executed again after memory is discovered.
>> // This is the second execution of this module, clear the hash interface
>> // information registered at its first execution.
>> //
>
> And note that here we are just past the check
>
>> HashInterfaceHob = InternalGetHashInterfaceHob ();
>> if (HashInterfaceHob != NULL) {
>
> in other words, the "Identifier equals gEfiCallerIdGuid" branch from the
> type definition applies; meaning that "SupportedHashMask" is defined
> too.
>
> Thus, I'd like the other reviewers to confirm whether we should clear
> "SupportedHashMask".
>
> From the original (buggy) code, I think we *should* clear
> SupportedHashMask as well. The "Length" argument of the original
> ZeroMem() call implies precisely that:
>
> sizeof (*HashInterfaceHob) - sizeof (EFI_GUID)
>
> This stands for "all fields in *HashInterfaceHob except the leading
> EFI_GUID Identifier field".
>
> So, the actual bug is in the "Buffer" argument of the ZeroMem() call: it
> is a typo. Certainly the intent must have been "start clearing from the
> first field right after the EFI_GUID Identifier field", namely
> "HashInterfaceCount":
>
> >HashInterfaceCount
> ^
>
> Therefore
Re: [edk2] [PATCH 5/7] ovmf: link with Tcg2Dxe module
Hi On Mon, Feb 26, 2018 at 10:50 AM, Laszlo Ersek <ler...@redhat.com> wrote: > On 02/23/18 14:23, marcandre.lur...@redhat.com wrote: >> From: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> This module measures and log the boot environment. It also produces >> the Tcg2 protocol, which allows for example to read the log from OS: >> >> [0.00] efi: EFI v2.70 by EDK II >> [0.00] efi: SMBIOS=0x3fa1f000 ACPI=0x3fbb6000 ACPI 2.0=0x3fbb6014 >> MEMATTR=0x3e7d4318 TPMEventLog=0x3db21018 >> >> $ python chipsec_util.py tpm parse_log binary_bios_measurements >> >> [CHIPSEC] Version 1.3.5.dev2 >> [CHIPSEC] API mode: using OS native API (not using CHIPSEC kernel module) >> [CHIPSEC] Executing command 'tpm' with args ['parse_log', >> '/tmp/binary_bios_measurements'] >> >> PCR: 0type: EV_S_CRTM_VERSION size: 0x2 >> digest: 1489f923c4dca729178b3e3233458550d8dddf29 >> + version: >> PCR: 0type: EV_EFI_PLATFORM_FIRMWARE_BLOB size: 0x10 >> digest: fd39ced7c0d2a61f6830c78c7625f94826b05bcc >> + base: 0x82length: 0xe >> PCR: 0type: EV_EFI_PLATFORM_FIRMWARE_BLOB size: 0x10 >> digest: 39ebc6783b72bc1e73c7d5bcfeb5f54a3f105d4c >> + base: 0x90length: 0xa0 >> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x35 >> digest: 57cd4dc19442475aa82743484f3b1caa88e142b8 >> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x24 >> digest: 9b1387306ebb7ff8e795e7be77563666bbf4516e >> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x26 >> digest: 9afa86c507419b8570c62167cb9486d9fc809758 >> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x24 >> digest: 5bf8faa078d40ffbd03317c93398b01229a0e1e0 >> PCR: 7type: EV_EFI_VARIABLE_DRIVER_CONFIG size: 0x26 >> digest: 734424c9fe8fc71716c42096f4b74c88733b175e >> PCR: 7type: EV_SEPARATOR size: 0x4 >> digest: 9069ca78e7450a285173431b3e52c5c25299e473 >> PCR: 1type: EV_EFI_VARIABLE_BOOT size: 0x3e >> digest: 252f8ebb85340290b64f4b06a001742be8e5cab6 >> PCR: 1type: EV_EFI_VARIABLE_BOOT size: 0x6e >> digest: 22a4f6ee9af6dba01d3528deb64b74b582fc182b >> PCR: 1type: EV_EFI_VARIABLE_BOOT size: 0x80 >> digest: b7811d5bf30a7efd4e385c6179fe10d9290bb9e8 >> PCR: 1type: EV_EFI_VARIABLE_BOOT size: 0x84 >> digest: 425e502c24fc924e231e0a62327b6b7d1f704573 >> PCR: 1type: EV_EFI_VARIABLE_BOOT size: 0x9a >> digest: 0b5d2c98ac5de6148a4a1490ff9d5df69039f04e >> PCR: 1type: EV_EFI_VARIABLE_BOOT size: 0xbd >> digest: 20bd5f402271d57a88ea314fe35c1705956b1f74 >> PCR: 1type: EV_EFI_VARIABLE_BOOT size: 0x88 >> digest: df5d6605cb8f4366d745a8464cfb26c1efdc305c >> PCR: 4type: EV_EFI_ACTION size: 0x28 >> digest: cd0fdb4531a6ec41be2753ba042637d6e5f7f256 >> PCR: 0type: EV_SEPARATOR size: 0x4 >> digest: 9069ca78e7450a285173431b3e52c5c25299e473 >> PCR: 1type: EV_SEPARATOR size: 0x4 >> digest: 9069ca78e7450a285173431b3e52c5c25299e473 >> PCR: 2type: EV_SEPARATOR size: 0x4 >> digest: 9069ca78e7450a285173431b3e52c5c25299e473 >> PCR: 3type: EV_SEPARATOR size: 0x4 >> digest: 9069ca78e7450a285173431b3e52c5c25299e473 >> PCR: 4type: EV_SEPARATOR size: 0x4 >> digest: 9069ca78e7450a285173431b3e52c5c25299e473 >> PCR: 5type: EV_SEPARATOR size: 0x4 >> digest: 9069ca78e7450a285173431b3e52c5c25299e473 >> >> $ tpm2_pcrlist >> sha1 : >> 0 : 35bd1786b6909daad610d7598b1d620352d33b8a >> 1 : ec0511e860206e0af13c31da2f9e943fb6ca353d >> 2 : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236 >> 3 : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236 >> 4 : 45a323382bd933f08e7f0e256bc8249e4095b1ec >> 5 : d16d7e629fd8d08ca256f9ad3a3a1587c9e6cc1b >> 6 : b2a83b0ebf2f8374299a5b2bdfc31ea955ad7236 >> 7 : 518bd167271fbb64589c61e43d8c0165861431d8 >> 8 : >> 9 : >> 10 : >> 11 : >> 12 : 00
Re: [edk2] [PATCH 3/7] HACK: HobLib: workaround infinite loop
Hi
On Fri, Feb 23, 2018 at 8:45 PM, Andrew Fish <af...@apple.com> wrote:
>
>
>> On Feb 23, 2018, at 5:23 AM, marcandre.lur...@redhat.com wrote:
>>
>> From: Marc-André Lureau <marcandre.lur...@redhat.com>
>>
>> Without this hack, GetNextHob() loops infinitely with the next patch.
>> I don't understand the reason.
>>
>> The loop is triggered by the GetFirstGuidHob () call.
>>
>> CC: Laszlo Ersek <ler...@redhat.com>
>> CC: Stefan Berger <stef...@linux.vnet.ibm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> ---
>> MdePkg/Library/PeiHobLib/HobLib.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/MdePkg/Library/PeiHobLib/HobLib.c
>> b/MdePkg/Library/PeiHobLib/HobLib.c
>> index 5c0eeb992f..ed3c5fbd6d 100644
>> --- a/MdePkg/Library/PeiHobLib/HobLib.c
>> +++ b/MdePkg/Library/PeiHobLib/HobLib.c
>> @@ -89,6 +89,10 @@ GetNextHob (
>> if (Hob.Header->HobType == Type) {
>> return Hob.Raw;
>> }
>> +if (GET_HOB_LENGTH (HobStart) == 0) {
>
> As Laszlo points out this error condition is likely memory corruption. Thus
> it would be better to check for all know illegal values?
>
> if (GET_HOB_LENGTH(HobStart) < sizeof (EFI_HOB_GENERIC_HEADER)
>
Thanks, I have adjusted the check.
With manual calls and printf (I don't know a better way to debug ovmf
;), I try to locate the issue. It's somehow related to
RegisterForShadow(). The "corruption" seems to happen during the
second call. After the
PeiLoadImage(...,PEIM_STATE_REGISTER_FOR_SHADOW,..), right before
calling PeimEntryPoint(), a GetFirstGuidHob() succeed, but inside the
function, it fails (with the same arguments). Right after it succeeds
again... The PeimEntryPoint() is not the Tcg2Pei:PeimEntryMA(), I
suppose there is some kind of wrapping code, but I fail to find where.
Any idea?
thanks for your help
--
Marc-André Lureau
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 4/7] ovmf: link with Tcg2Pei module
Hi
On Mon, Feb 26, 2018 at 10:38 AM, Laszlo Ersek <ler...@redhat.com> wrote:
> On 02/23/18 14:23, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lur...@redhat.com>
>>
>> This module will initialize TPM device, measure reported FVs and BIOS
>> version.
>>
>> CC: Laszlo Ersek <ler...@redhat.com>
>> CC: Stefan Berger <stef...@linux.vnet.ibm.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> ---
>> OvmfPkg/OvmfPkgX64.dsc | 7 +++
>> OvmfPkg/OvmfPkgX64.fdf | 1 +
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index b5cbe8430f..34a7c2778e 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -279,6 +279,8 @@
>>PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>>QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
>> !if $(TPM2_ENABLE)
>> + BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
>> +
>> HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
>>
>> Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
>>Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
>> !endif
>> @@ -647,6 +649,11 @@
>>
>> !if $(TPM2_ENABLE) == TRUE
>>SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> + SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>> +
>> + NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
>> +
>> NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>> + }
>> !endif
>>
>> !if $(SECURE_BOOT_ENABLE) == TRUE
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index dc35d0a1f7..9558142a42 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -170,6 +170,7 @@ INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>> !endif
>> !if $(TPM2_ENABLE) == TRUE
>> INF SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>> +INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>> !endif
>>
>>
>>
>>
>
> Would it be possible to drop SHA1 (include SHA256 only) by setting
> PcdTpm2HashMask to value 2? Or SHA1 required for some other reason? (If
> so please mention it in the commit message.)
>
afaik, it's not strictly required, and apparently the support is being
dropped. I'll remove it.
btw, now I understand your comment about read-only variable not being
used by PEI module. I'll add a preliminary patch dropping it from
depex ;)
thanks
--
Marc-André Lureau
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 2/7] ovmf: link with Tcg2ConfigPei module
Hi On Fri, Feb 23, 2018 at 6:31 PM, Laszlo Ersek <ler...@redhat.com> wrote: > On 02/23/18 14:23, marcandre.lur...@redhat.com wrote: >> From: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> This module initializes TPM device type based on variable and >> detection. > > (1) I suggest we say the following here: > > "The Tcg2ConfigPei module informs the firmware globally about the TPM > device type, by setting the PcdTpmInstanceGuid PCD to the appropriate > GUID value. The original module under SecurityPkg can perform device > detection, or read a cached value from a non-volatile UEFI variable. > OvmfPkg's clone of the module always performs the hardware detection." > ok > Becase... > >> The module requires VariablePei, which is built with >> MEM_VARSTORE_EMU_ENABLE=FALSE. > > (2) ... as I hinted in my response to your blurb, and also as suggested > by "Tcg2ConfigPei.inf", we should clone Tcg2ConfigPei for OVMF, and > *trim it* quite a bit. > > - The new location should be "OvmfPkg/Tcg/Tcg2Config/". > > - We need not copy the ".uni" file (also drop MODULE_UNI_FILE from the > INF file) > > - Re-generate FILE_GUID in the INF file with "uuidgen" > > - Remove all PEI-phase variable access; always perform the hw detection. > > - I would even suggest removing support for TPM1.2. Just check whether > TPM2 is available or not. > ok > > (3) Ultimately, this is what the module should do: > > - Check the QEMU hardware for TPM2 availability only > > - If found, set the dynamic PCD "PcdTpmInstanceGuid" to >This is what informs the rest of > the firmware about the TPM type. > > - Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the > PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD. > In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting > and the consumption of the "TPM type" PCD. > > - If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid. > (Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if > no TPM2 is available. So in that case our Tcg2ConfigPei must do it.) ok > > (4) Regarding the TPM detection itself. It looks like DetectTpmDevice() > [SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c] calls a number of TPM1.2 > functions. If the earliest one fails, it assumes "no TPM" at all, but if > only a later call fails, it deduces, from the 1.2 failure, that TPM2 exists. > > I think we can do better than this, in our Tcg2ConfigPei clone: > > - We should call Tpm2RequestUseTpm() directly, from > "SecurityPkg/Include/Library/Tpm2DeviceLib.h". > > - And, Tpm2Startup(), from > "SecurityPkg/Include/Library/Tpm2CommandLib.h", will be called by Tcg2Pei. ok > > (5) Finally, there's no need to set "PcdTpmInitializationPolicy" to > anything. I don't see it consumed by any module that we should include > in OVMF. (More on this below.) > ok > > (6) Now, I realize Tcg2Pei *apparently* depends on > gEfiPeiReadOnlyVariable2PpiGuid (i.e., read-only variable access in the > PEI phase) as well. That's a bug in the INF file (the [depex] section). > If you grep the Tcg2Pei module source for the GUID, the [depex] section > is the only hit. Can you please submit a separate patch that removes it > from the depex? I don't get how you came to that conclusion, both SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c and SecurityPkg/Tcg/Tcg2Config/TpmDetection.c match. Apparently, the variable is used in s3 mode, in DetectTpmDevice(). I'll drop it from OvmfPkg version for now. > > >> CC: Laszlo Ersek <ler...@redhat.com> >> CC: Stefan Berger <stef...@linux.vnet.ibm.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >> --- >> OvmfPkg/OvmfPkgX64.dsc | 20 >> OvmfPkg/OvmfPkgX64.fdf | 3 +++ >> 2 files changed, 23 insertions(+) > > Is there any particular reason to exclude the Ia32 and Ia32X64 builds? > > If not, then please modify all three sets of dsc/fdf files identically. I'd rather keep this as a TODO item for now, since we are not close to a final version, and it's annoying to have to fix each files etc.. > >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index 32c57b04e1..b5cbe8430f 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -40,6 +40,7 @@ > > (7) Please implement the following git settings in your edk2 clone: > > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for
[edk2] Enabling TPM support in ovmf & hang during qemu boot
Hi,
I use the attached patch to build OVMF with TPM support.
Even without any TPM device configured (with the following qemu
command line) the VM hangs early:
qemu-system-x86_64 -enable-kvm -m 1024 -global
isa-debugcon.iobase=0x402 -debugcon file:ovmf.log -drive
if=pflash,format=raw,file=...src/edk2/Build/OvmfX64/DEBUG_GCC5/FV/OVMF_CODE.fd,readonly
-drive
if=pflash,format=raw,file=...src/edk2/Build/OvmfX64/DEBUG_GCC5/FV/OVMF_VARS.fd
I don't have much clue how to debug OVMF, but adding DEBUG lines, I
could learn that during ReserveEmuVariableNvStore(), GetNextHob() runs
an infinite loop, looking for EFI_HOB_TYPE_UNUSED.
How is the HobList populated? Is it possible to add more of the UNUSED entries?
Any help welcome
--
Marc-André Lureau
commit 9e13683ae1351054bf14a087bfb89a14009b38e5
Author: Marc-André Lureau <marcandre.lur...@redhat.com>
Date: Fri Nov 10 14:49:02 2017 +0100
Add TPM2
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 1ffcf37f8b..ba73c250c6 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -39,6 +39,7 @@
DEFINE HTTP_BOOT_ENABLE= FALSE
DEFINE SMM_REQUIRE = FALSE
DEFINE TLS_ENABLE = FALSE
+ DEFINE TPM2_ENABLE = FALSE
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
@@ -203,6 +204,13 @@
OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
+!if $(TPM2_ENABLE) == TRUE
+ Tpm12CommandLib|SecurityPkg/Library/Tpm12CommandLib/Tpm12CommandLib.inf
+ Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf
+ Tcg2PhysicalPresenceLib|SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
+ Tcg2PpVendorLib|SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.inf
+!endif
+
[LibraryClasses.common]
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
@@ -266,6 +274,13 @@
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
+!ifdef $(TPM2_ENABLE)
+ BaseCryptLib|CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf
+ HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
+ Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibDTpm/Tpm12DeviceLibDTpm.inf
+ Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
+ Tcg2PhysicalPresenceLib|SecurityPkg/Library/PeiTcg2PhysicalPresenceLib/PeiTcg2PhysicalPresenceLib.inf
+!endif
[LibraryClasses.common.DXE_CORE]
HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -346,6 +361,11 @@
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
+!ifdef $(TPM2_ENABLE)
+ HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
+ Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
+ Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
+!endif
[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -456,7 +476,7 @@
# DEBUG_VERBOSE 0x0040 // Detailed debug messages that may
# // significantly impact boot performance
# DEBUG_ERROR 0x8000 // Error
- gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x804F
+ gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x8040004F
!ifdef $(SOURCE_DEBUG_ENABLE)
gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17
@@ -549,6 +569,21 @@
gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x00
+!if $(TPM2_ENABLE) == TRUE
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0xb6, 0xe5, 0x01, 0x8b, 0x19, 0x4f, 0xe8, 0x46, 0xab, 0x93, 0x1c, 0x53, 0x67, 0x1b, 0x90, 0xcc}
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2SelfTestPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2ScrtmPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmInitializationPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmScrtmPolicy|1
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2HashMask|3
+ gEfiSecurityPkgTokenSpaceGuid.PcdTcg2HashAlgorithmBitmap|3
+!endif
+
+[PcdsDynamicHii.common.DEFAULT]
+ gEfiSecurityPkgTokenSpaceGuid.PcdTcgPhysicalPresenceInterfaceVer|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x0|"1.3"|NV,BS
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpm2AcpiTableRev|L"TCG2_VERSION"|gTcg2ConfigFormSetGuid|0x8|3|NV,BS
+
#
# Components Section - list of all EDK II Modules needed by this Platform.
@@ -613,6 +648,39 @@
MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
+!if $
