Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF

["Ricardo Araújo" <ricardo@lsd.ufcg.edu.br>]

Hi folks, sorry for the delay! 

I've just applied the patch and got the same error message and empty PCRs. 

Regards, 

Ricardo Araújo Santos - 
www.lsd.ufcg.edu.br/~ricardo 



De: "Zhang, Chao B" <chao.b.zhang@intel.com> 
Para: "Laszlo Ersek" <lersek@redhat.com>, "Ricardo Araújo" <ricardo@lsd.ufcg.edu.br>, "Marc-André Lureau" <marcandre.lureau@redhat.com> 
Cc: edk2-devel@lists.01.org, "Gao, Liming" <liming.gao@intel.com>, "Zeng, Star" <star.zeng@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:lersek@redhat.com] 
Sent: Thursday, August 2, 2018 9:14 PM 
To: Zhang, Chao B <chao.b.zhang@intel.com>; Ricardo Araújo <ricardo@lsd.ufcg.edu.br>; Marc-André Lureau <marcandre.lureau@redhat.com> 
Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@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:lersek@redhat.com ] 
> Sent: Thursday, August 2, 2018 5:49 AM 
> To: Ricardo Araújo < ricardo@lsd.ufcg.edu.br >; Zhang, Chao B < chao.b.zhang@intel.com >; Marc-André Lureau < marcandre.lureau@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" < ricardo@lsd.ufcg.edu.br<mailto:ricardo@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 
>