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

["Zhang, Chao B" <chao.b.zhang@intel.com>]

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<mailto:ricardo@lsd.ufcg.edu.br>>; Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Marc-André Lureau <marcandre.lureau@redhat.com<mailto:marcandre.lureau@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" <ricardo@lsd.ufcg.edu.br<mailto:ricardo@lsd.ufcg.edu.br<mailto:ricardo@lsd.ufcg.edu.br%3cmailto:ricardo@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
>