* Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
@ 2018-08-01 17:33 Ricardo Araújo
2018-08-01 17:50 ` Ricardo Araújo
0 siblings, 1 reply; 13+ messages in thread
From: Ricardo Araújo @ 2018-08-01 17:33 UTC (permalink / raw)
To: edk2-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-01 17:33 Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF Ricardo Araújo
@ 2018-08-01 17:50 ` Ricardo Araújo
2018-08-01 21:49 ` Laszlo Ersek
0 siblings, 1 reply; 13+ messages in thread
From: Ricardo Araújo @ 2018-08-01 17:50 UTC (permalink / raw)
To: Ricardo Araújo; +Cc: edk2-devel
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" <ricardo@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-01 17:50 ` Ricardo Araújo
@ 2018-08-01 21:49 ` Laszlo Ersek
2018-08-02 2:04 ` Zhang, Chao B
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2018-08-01 21:49 UTC (permalink / raw)
To: Ricardo Araújo, Chao Zhang, Marc-André Lureau; +Cc: edk2-devel
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" <ricardo@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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-01 21:49 ` Laszlo Ersek
@ 2018-08-02 2:04 ` Zhang, Chao B
2018-08-02 13:14 ` Laszlo Ersek
0 siblings, 1 reply; 13+ messages in thread
From: Zhang, Chao B @ 2018-08-02 2:04 UTC (permalink / raw)
To: Laszlo Ersek, Ricardo Araújo, Marc-André Lureau
Cc: edk2-devel@lists.01.org, Gao, Liming, Zeng, Star
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.
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-02 2:04 ` Zhang, Chao B
@ 2018-08-02 13:14 ` Laszlo Ersek
2018-08-03 0:22 ` Zhang, Chao B
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2018-08-02 13:14 UTC (permalink / raw)
To: Zhang, Chao B, Ricardo Araújo, Marc-André Lureau
Cc: edk2-devel@lists.01.org, Gao, Liming, Zeng, Star
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-02 13:14 ` Laszlo Ersek
@ 2018-08-03 0:22 ` Zhang, Chao B
2018-08-03 13:39 ` Ricardo Araújo
0 siblings, 1 reply; 13+ messages in thread
From: Zhang, Chao B @ 2018-08-03 0:22 UTC (permalink / raw)
To: Laszlo Ersek, Ricardo Araújo, Marc-André Lureau
Cc: edk2-devel@lists.01.org, Gao, Liming, Zeng, Star
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-03 0:22 ` Zhang, Chao B
@ 2018-08-03 13:39 ` Ricardo Araújo
2018-08-03 14:45 ` Laszlo Ersek
0 siblings, 1 reply; 13+ messages in thread
From: Ricardo Araújo @ 2018-08-03 13:39 UTC (permalink / raw)
To: Zhang, Chao B
Cc: Laszlo Ersek, Marc-André Lureau, edk2-devel, Gao, Liming,
Zeng, Star
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-03 13:39 ` Ricardo Araújo
@ 2018-08-03 14:45 ` Laszlo Ersek
2018-08-06 15:26 ` Zhang, Chao B
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2018-08-03 14:45 UTC (permalink / raw)
To: Ricardo Araújo, Zhang, Chao B
Cc: Marc-André Lureau, edk2-devel, Gao, Liming, Zeng, Star
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.
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>
> 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
>>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-03 14:45 ` Laszlo Ersek
@ 2018-08-06 15:26 ` Zhang, Chao B
2018-08-09 14:09 ` Marc-André Lureau
0 siblings, 1 reply; 13+ messages in thread
From: Zhang, Chao B @ 2018-08-06 15:26 UTC (permalink / raw)
To: Laszlo Ersek, Ricardo Araújo
Cc: edk2-devel@lists.01.org, Zeng, Star, Gao, Liming
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.
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-06 15:26 ` Zhang, Chao B
@ 2018-08-09 14:09 ` Marc-André Lureau
2018-08-09 14:55 ` Laszlo Ersek
0 siblings, 1 reply; 13+ messages in thread
From: Marc-André Lureau @ 2018-08-09 14:09 UTC (permalink / raw)
To: Zhang, Chao B
Cc: Laszlo Ersek, Ricardo Araújo, edk2-devel@lists.01.org,
Gao, Liming, Zeng, Star
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-09 14:09 ` Marc-André Lureau
@ 2018-08-09 14:55 ` Laszlo Ersek
2018-08-09 15:46 ` Zhang, Chao B
0 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2018-08-09 14:55 UTC (permalink / raw)
To: Marc-André Lureau, Zhang, Chao B
Cc: Ricardo Araújo, edk2-devel@lists.01.org, Gao, Liming,
Zeng, Star
On 08/09/18 16:09, Marc-André Lureau wrote:
> 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
Nice! \o/
>
> 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
It's usually a good idea to spell out the client module types that are
permitted to consume the specific library instance, for a given library
class requirement. Different module types have different restrictions
and devices at their disposal, in their respective environments /
firmware phases, and library instances may be specific to those
restrictions / devices.
In this specific case, a PcdLib dependency (more, precisely, a dynamic
PCD dependency) was added to the library instance, and so it might make
sense to restrict the library instance to module types whose
environments (their entry point functions anyway) support dynamic PCDs.
I do agree though that this change should have been made either in a
separate patch (if the change isn't closely related to the PCD
dependency), *or* (if it is) it should have been explained / justified
specifically, in the commit message. The commit message is very lacking
indeed.
Thank you for tracking this down!
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-09 14:55 ` Laszlo Ersek
@ 2018-08-09 15:46 ` Zhang, Chao B
2018-08-09 16:09 ` Laszlo Ersek
0 siblings, 1 reply; 13+ messages in thread
From: Zhang, Chao B @ 2018-08-09 15:46 UTC (permalink / raw)
To: Laszlo Ersek, Marc-André Lureau
Cc: Zeng, Star, edk2-devel@lists.01.org, Gao, Liming
Hi Laszlo:
We seriously considered such dependency change in design. The library is shared between DXE & PEI. So PCD is the generic way to share the data and reduce real register touch.
Therefore, the problem is that Library can’t be used by SEC anymore. The decision is, since there is no SEC usage so far, we’d like to change Lib type first & Split it when there is real usage.
I think your suggestion to split the patch & provide more detailed information in log is good. I will follow this rule later on.
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, August 9, 2018 10:56 PM
To: Marc-André Lureau <marcandre.lureau@gmail.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
On 08/09/18 16:09, Marc-André Lureau wrote:
> Hi
>
> On Mon, Aug 6, 2018 at 5:26 PM, Zhang, Chao B <chao.b.zhang@intel.com<mailto: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<mailto:ricardo@lsd.ufcg.edu.br>>; Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto: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
Nice! \o/
>
> 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
It's usually a good idea to spell out the client module types that are
permitted to consume the specific library instance, for a given library
class requirement. Different module types have different restrictions
and devices at their disposal, in their respective environments /
firmware phases, and library instances may be specific to those
restrictions / devices.
In this specific case, a PcdLib dependency (more, precisely, a dynamic
PCD dependency) was added to the library instance, and so it might make
sense to restrict the library instance to module types whose
environments (their entry point functions anyway) support dynamic PCDs.
I do agree though that this change should have been made either in a
separate patch (if the change isn't closely related to the PCD
dependency), *or* (if it is) it should have been explained / justified
specifically, in the commit message. The commit message is very lacking
indeed.
Thank you for tracking this down!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
2018-08-09 15:46 ` Zhang, Chao B
@ 2018-08-09 16:09 ` Laszlo Ersek
0 siblings, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2018-08-09 16:09 UTC (permalink / raw)
To: Zhang, Chao B, Marc-André Lureau
Cc: Zeng, Star, edk2-devel@lists.01.org, Gao, Liming
On 08/09/18 17:46, Zhang, Chao B wrote:
> Hi Laszlo:
> We seriously considered such dependency change in design. The library is shared between DXE & PEI. So PCD is the generic way to share the data and reduce real register touch.
> Therefore, the problem is that Library can’t be used by SEC anymore. The decision is, since there is no SEC usage so far, we’d like to change Lib type first & Split it when there is real usage.
> I think your suggestion to split the patch & provide more detailed information in log is good. I will follow this rule later on.
Right, I agree that, if the goal was to exclude SEC from the permitted
client module types, then the LIBRARY_CLASS restriction change was valid.
I also agree that placing the LIBRARY_CLASS change into the exact same
patch was correct, as the introduction of the PCD dependency. In fact,
in this case, separating the LIBRARY_CLASS restrictions to a different
patch would have been wrong -- because then a commit would exist where
the lib instance is no longer suitable for SEC, but the LIBRARY_CLASS
restriction wouldn't enforce that.
Therefore, in this case, the missing bit was really only a better the
commit message -- basically, your above argument, about excluding SEC.
... A workflow comment again: this is now the 2nd time in a short
timeframe that I realize that Intel performs careful investigation in
the background, writes the corresponding patch, and then omits the
entire investigation from the commit message. PLEASE, don't do that. You
are throwing away your own work, and you are wasting the time of the
community.
Commit messages embody communication with people. That is the important
part. That is what makes or breaks Open Development. The code in the
body of the patch is just a consequence of the commit message.
Thanks,
Laszlo
>
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, August 9, 2018 10:56 PM
> To: Marc-André Lureau <marcandre.lureau@gmail.com>; Zhang, Chao B <chao.b.zhang@intel.com>
> Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF
>
> On 08/09/18 16:09, Marc-André Lureau wrote:
>> Hi
>>
>> On Mon, Aug 6, 2018 at 5:26 PM, Zhang, Chao B <chao.b.zhang@intel.com<mailto: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<mailto:ricardo@lsd.ufcg.edu.br>>; Zhang, Chao B <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
>>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto: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
>
> Nice! \o/
>
>>
>> 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
>
> It's usually a good idea to spell out the client module types that are
> permitted to consume the specific library instance, for a given library
> class requirement. Different module types have different restrictions
> and devices at their disposal, in their respective environments /
> firmware phases, and library instances may be specific to those
> restrictions / devices.
>
> In this specific case, a PcdLib dependency (more, precisely, a dynamic
> PCD dependency) was added to the library instance, and so it might make
> sense to restrict the library instance to module types whose
> environments (their entry point functions anyway) support dynamic PCDs.
>
> I do agree though that this change should have been made either in a
> separate patch (if the change isn't closely related to the PCD
> dependency), *or* (if it is) it should have been explained / justified
> specifically, in the commit message. The commit message is very lacking
> indeed.
>
> Thank you for tracking this down!
>
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-08-09 16:09 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-01 17:33 Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF Ricardo Araújo
2018-08-01 17:50 ` Ricardo Araújo
2018-08-01 21:49 ` Laszlo Ersek
2018-08-02 2:04 ` Zhang, Chao B
2018-08-02 13:14 ` Laszlo Ersek
2018-08-03 0:22 ` Zhang, Chao B
2018-08-03 13:39 ` Ricardo Araújo
2018-08-03 14:45 ` Laszlo Ersek
2018-08-06 15:26 ` Zhang, Chao B
2018-08-09 14:09 ` Marc-André Lureau
2018-08-09 14:55 ` Laszlo Ersek
2018-08-09 15:46 ` Zhang, Chao B
2018-08-09 16:09 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox