Hi On Mon, Aug 6, 2018 at 5:26 PM, Zhang, Chao B <chao.b.zh...@intel.com> 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 <rica...@lsd.ufcg.edu.br>; Zhang, Chao B > <chao.b.zh...@intel.com> > Cc: edk2-devel@lists.01.org; Zeng, Star <star.z...@intel.com>; Gao, Liming > <liming....@intel.com> > 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" <chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com>> >> Para: "Laszlo Ersek" <ler...@redhat.com<mailto:ler...@redhat.com>>, "Ricardo >> Araújo" <rica...@lsd.ufcg.edu.br<mailto:rica...@lsd.ufcg.edu.br>>, >> "Marc-André Lureau" >> <marcandre.lur...@redhat.com<mailto:marcandre.lur...@redhat.com>> >> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>, "Gao, Liming" >> <liming....@intel.com<mailto:liming....@intel.com>>, "Zeng, Star" >> <star.z...@intel.com<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 <chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com>>; >> Ricardo Araújo <rica...@lsd.ufcg.edu.br<mailto:rica...@lsd.ufcg.edu.br>>; >> Marc-André Lureau >> <marcandre.lur...@redhat.com<mailto:marcandre.lur...@redhat.com>> >> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Gao, Liming >> <liming....@intel.com<mailto:liming....@intel.com>>; Zeng, Star >> <star.z...@intel.com<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. >> >> Let's wait for Ricardo's response. Perhaps my analysis / suspicion were >> incorrect and it's not actually the "dynamism" of the PCD that's missing >> for OVMF. Ricardo's answer will tell us if there's another issue. >> >> Thanks >> Laszlo >> >>> From: Laszlo Ersek [ mailto:ler...@redhat.com ] >>> Sent: Thursday, August 2, 2018 5:49 AM >>> To: Ricardo Araújo < >>> rica...@lsd.ufcg.edu.br<mailto:rica...@lsd.ufcg.edu.br> >; Zhang, Chao B < >>> chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com> >; Marc-André Lureau >>> < marcandre.lur...@redhat.com<mailto:marcandre.lur...@redhat.com> > >>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> >>> Subject: Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 >>> with OVMF >>> >>> On 08/01/18 19:50, Ricardo Araújo wrote: >>>> The commit I was referring to is: >>>> https://github.com/tianocore/edk2/commit/f15cb995bb3880b77e15afe6facd3da05e599a17 >>>> >>>> Regards, >>>> >>>> Ricardo Araujo - >>>> www.lsd.ufcg.edu.br/~ricardo<http://www.lsd.ufcg.edu.br/~ricardo<http://www.lsd.ufcg.edu.br/~ricardo%3chttp:/www.lsd.ufcg.edu.br/~ricardo> >>>> > >>>> >>>> ----- Mensagem original ----- >>>> De: "Ricardo Araújo" < >>>> rica...@lsd.ufcg.edu.br<mailto:rica...@lsd.ufcg.edu.br<mailto:rica...@lsd.ufcg.edu.br%3cmailto:rica...@lsd.ufcg.edu.br> >>>> >> >>>> Para: >>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org> >>>> > >>>> Enviadas: Quarta-feira, 1 de agosto de 2018 14:33:45 >>>> Assunto: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 >>>> with OVMF >>>> >>>> Hi everyone, >>>> >>>> I'm using OVMF with a simulated TPM 2.0 (from >>>> https://github.com/stefanberger/swtpm ) and I noticed lately that PCRs >>>> 0-7 are zeroed after booting the vm (ubuntu 18.04) and the only >>>> message related to this in dmesg is: >>>> >>>> [ 2.286690] tpm_tis 00:06: 2.0 TPM (device-id 0x1, rev-id 1) >>>> [ 2.303753] tpm tpm0: A TPM error (256) occurred continue selftest >>>> [ 2.314199] tpm tpm0: starting up the TPM manually >>>> >>>> I found this started to happen after this commit , previous commits to >>>> that are showing boot time measurements on PCR 0-7 normally and the >>>> error message is gone. Has anyone experienced the same behavior? I >>>> followed the instructions here for building OVMF but I added the >>>> parameters -D TPM2_ENABLE=TRUE -D SECURE_BOOT_ENABLE=TRUE -D >>>> HTTP_BOOT_ENABLE=TRUE. Is there anything else I need to add to enable >>>> these measurements? >>>> >>>> Regards, >>>> >>>> Ricardo Araujo >>>> www.lsd.ufcg.edu.br/~ricardo<http://www.lsd.ufcg.edu.br/~ricardo<http://www.lsd.ufcg.edu.br/~ricardo%3chttp:/www.lsd.ufcg.edu.br/~ricardo> >>>> > >>> >>> Thank you for the bug report. It looks like a regression to me, but the >>> details aren't immediately clear. >>> >>> Adding Marc-André who contributed the TPM enablement for OVMF, and Chao >>> Zhang who authored the commit in question. >>> >>> If I recall correctly, in OVMF we decided to never cache the TPM type >>> but always detect it. I could be remembering wrong though. See commit >>> 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone", >>> 2018-03-09). >>> >>> Chao Zhang: can you please explain what additional requirements are >>> presented for a platform by commit f15cb995bb38? In OVMF we use a >>> customized Tcg2ConfigPei module (see the commit above). >>> >>> >>> Oh wait, I suspect what's wrong. I believe there are two bugs in commit >>> f15cb995bb38 ("SecurityPkg: Cache TPM interface type info", 2018-06-25): >>> >>> * Bug#1: >>> >>> Commit f15cb995bb38 introduces a new PCD, called >>> "PcdActiveTpmInterfaceType", in section [PcdsDynamic, PcdsDynamicEx] of >>> "SecurityPkg.dec", and makes core modules from SecurityPkg dependent on >>> it. >>> >>> Obviously this means that platforms are required to provide a Dynamic >>> Default for the new PCD in their DSC files, if they include those core >>> modules from SecurityPkg, otherwise the PCD won't actually behave >>> dynamically -- "set" operations will fail, and "get" operations will >>> just return the central default from the SecurityPkg.dec file. As a >>> result, the cached TPM type will always be wrong (it will look like >>> "undetected", 0xFF). >>> >>> This could have been avoided by grepping all "*dsc*" files in the edk2 >>> tree for references to the SecurityPkg module INF files that were about >>> to receive a dependency on the PCD. Such as: >>> >>> git grep -l -F \ >>> -e SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf \ >>> --or -e SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf \ >>> --or -e SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf \ >>> --or -e SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.inf \ >>> '*dsc*' >>> >>> This would have listed all platforms in-tree that were going to depend >>> on the new dynamic PCD via inclusion of the affected SecurityPkg >>> modules. >>> >>> Running this command now, I get the following output: >>> >>> OvmfPkg/OvmfPkgIa32.dsc >>> OvmfPkg/OvmfPkgIa32X64.dsc >>> OvmfPkg/OvmfPkgX64.dsc >>> SecurityPkg/SecurityPkg.dsc >>> >>> Open source hygiene dictates that modifications to infrastructure code >>> or otherwise central code be accompanied by necessary updates to *ALL* >>> in-tree subsystems that depend on said core code. (Out-of-tree >>> subsystems are a different matter.) It's OK if a single contributor >>> cannot test every single platform -- but we can still use the mailing >>> list and the bug tracker for raising the issue and expose the new >>> dependency for platforms that we can't test, but see as affected. >>> >>> Ricardo, Marc-André: does the following patch work for you guys >>> (build-tested only): >>> >>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >>>> index a28b511d5c2f..b0153f66b710 100644 >>>> --- a/OvmfPkg/OvmfPkgIa32.dsc >>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc >>>> @@ -579,6 +579,7 @@ [PcdsDynamicDefault] >>>> >>>> !if $(TPM2_ENABLE) == TRUE >>>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, >>>> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} >>>> + gEfiSecurityPkgTokenSpaceGuid.PcdActiveTpmInterfaceType|0xFF >>>> !endif >>>> >>>> ################################################################################ >>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >>>> index 115d0c01ff5c..fcce846ab9a5 100644 >>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >>>> @@ -587,6 +587,7 @@ [PcdsDynamicDefault] >>>> >>>> !if $(TPM2_ENABLE) == TRUE >>>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, >>>> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} >>>> + gEfiSecurityPkgTokenSpaceGuid.PcdActiveTpmInterfaceType|0xFF >>>> !endif >>>> >>>> ################################################################################ >>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >>>> index 362eb789c712..3eda1b3013f7 100644 >>>> --- a/OvmfPkg/OvmfPkgX64.dsc >>>> +++ b/OvmfPkg/OvmfPkgX64.dsc >>>> @@ -586,6 +586,7 @@ [PcdsDynamicDefault] >>>> >>>> !if $(TPM2_ENABLE) == TRUE >>>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x00, >>>> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} >>>> + gEfiSecurityPkgTokenSpaceGuid.PcdActiveTpmInterfaceType|0xFF >>>> !endif >>>> >>>> ################################################################################ >>> >>> If it works, I'll submit it later as a standalone patch. >>> >>> >>> * Bug#2: >>> >>> The PcdSet8S() calls added by commit f15cb995bb38 are not error-checked; >>> their return values are ignored. >>> >>> Honestly, if we ignore the return values of PcdSetXxxS() calls, then it >>> has been a wasted effort to introduce those "safe" APIs in the first >>> place, in commit 9a3558419509. At the bare minimum, an >>> ASSERT_RETURN_ERROR() should be added after every invocation. >>> >>> I've filed the following TianoCore BZ about Bug#2 now: >>> >>> https://bugzilla.tianocore.org/show_bug.cgi?id=1070 >>> >>> Thanks >>> Laszlo >>> >> >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org<mailto: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 -- Marc-André Lureau _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
