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 > > ----- Mensagem original ----- > De: "Ricardo Araújo" <rica...@lsd.ufcg.edu.br> > Para: 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
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