From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E2C93210C6431 for ; Wed, 1 Aug 2018 14:49:07 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CAC837262D; Wed, 1 Aug 2018 21:49:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-129.rdu2.redhat.com [10.10.122.129]) by smtp.corp.redhat.com (Postfix) with ESMTP id F346710DCF73; Wed, 1 Aug 2018 21:49:02 +0000 (UTC) To: =?UTF-8?Q?Ricardo_Ara=c3=bajo?= , Chao Zhang , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: edk2-devel@lists.01.org References: <551258016.93465.1533144825411.JavaMail.zimbra@lsd.ufcg.edu.br> <1169526465.93534.1533145845454.JavaMail.zimbra@lsd.ufcg.edu.br> From: Laszlo Ersek Message-ID: <00859174-589e-92d8-5f97-dcf9d735424c@redhat.com> Date: Wed, 1 Aug 2018 23:49:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1169526465.93534.1533145845454.JavaMail.zimbra@lsd.ufcg.edu.br> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Wed, 01 Aug 2018 21:49:06 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Wed, 01 Aug 2018 21:49:06 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: Missing boot related measurements at TPM 2.0 PCRs 0-7 with OVMF X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Aug 2018 21:49:09 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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" > 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