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>; Zhang, Chao B > <chao.b.zh...@intel.com>; Marc-André Lureau <marcandre.lur...@redhat.com> > Cc: 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> >> >> ----- Mensagem original ----- >> De: "Ricardo Araújo" >> <rica...@lsd.ufcg.edu.br<mailto:rica...@lsd.ufcg.edu.br>> >> Para: edk2-devel@lists.01.org<mailto: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> > > 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 https://lists.01.org/mailman/listinfo/edk2-devel
