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

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

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