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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Mon, Aug 6, 2018 at 5:26 PM, Zhang, Chao B <chao.b.zhang@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-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Friday, August 3, 2018 10:46 PM
> To: Ricardo Araújo <ricardo@lsd.ufcg.edu.br>; Zhang, Chao B <chao.b.zhang@intel.com>
> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@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.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
>> Para: "Laszlo Ersek" <lersek@redhat.com<mailto:lersek@redhat.com>>, "Ricardo Araújo" <ricardo@lsd.ufcg.edu.br<mailto:ricardo@lsd.ufcg.edu.br>>, "Marc-André Lureau" <marcandre.lureau@redhat.com<mailto:marcandre.lureau@redhat.com>>
>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>, "Gao, Liming" <liming.gao@intel.com<mailto:liming.gao@intel.com>>, "Zeng, Star" <star.zeng@intel.com<mailto: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<mailto:chao.b.zhang@intel.com>>; Ricardo Araújo <ricardo@lsd.ufcg.edu.br<mailto:ricardo@lsd.ufcg.edu.br>>; Marc-André Lureau <marcandre.lureau@redhat.com<mailto:marcandre.lureau@redhat.com>>
>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto: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
>>>
>>
>>
>
> _______________________________________________
> 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