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 A83B8210D7F2A for ; Thu, 2 Aug 2018 06:14:30 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 33F2A7DAC9; Thu, 2 Aug 2018 13:14:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-32.rdu2.redhat.com [10.10.123.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id C8F632026D69; Thu, 2 Aug 2018 13:14:27 +0000 (UTC) To: "Zhang, Chao B" , =?UTF-8?Q?Ricardo_Ara=c3=bajo?= , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: "edk2-devel@lists.01.org" , "Gao, Liming" , "Zeng, Star" References: <551258016.93465.1533144825411.JavaMail.zimbra@lsd.ufcg.edu.br> <1169526465.93534.1533145845454.JavaMail.zimbra@lsd.ufcg.edu.br> <00859174-589e-92d8-5f97-dcf9d735424c@redhat.com> From: Laszlo Ersek Message-ID: <23c3c4c3-461f-7e6a-a8ca-0574021c1ccf@redhat.com> Date: Thu, 2 Aug 2018 15:14:26 +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: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 02 Aug 2018 13:14:29 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 02 Aug 2018 13:14:29 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.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: Thu, 02 Aug 2018 13:14:30 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 ; Zhang, Chao B ; Marc-André Lureau > 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 >> >> ----- 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 >