From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::42a; helo=mail-wr1-x42a.google.com; envelope-from=marcandre.lureau@gmail.com; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7F0B7210C5138 for ; Thu, 9 Aug 2018 07:09:06 -0700 (PDT) Received: by mail-wr1-x42a.google.com with SMTP id j5-v6so5267990wrr.8 for ; Thu, 09 Aug 2018 07:09:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=8BgDQPARfinlFF9I/FJmkQQrngDgj5/y3+xb9Br+haQ=; b=rpOs7Ac8OTHhDRfOGfKk59QKTL7stF/xQv9yMffJCC+2oY7ZiWwJUmtibwTGMJ6tQR 44sBQlbmBWgwR9hiNojkLUUBS7soP8+agCzlUgb0KiBB5EKkdeEnJUx/fSU6HfxIU7jc omdX4GavVAUkIlEUjagU8xZ5bfwyWHRHTNrWi6QJeapuh/se9uH1cIsgXJKCmpoQlPxp dzeaP9vauTozl6RRpTa3XfXaotcf3xLe05dWxoX7A8ujAA0Y+t9j0woRSXWDpbpT9s2k jtdImamnFYFY5UsWeqeUu5bCzMQfWwOYBRTHcUvb2SAJ4Fd5ohBZLWGDq0UfLcVEgHSk jDQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=8BgDQPARfinlFF9I/FJmkQQrngDgj5/y3+xb9Br+haQ=; b=TS5c+n6M1bZPped2j9NNvQzI++wu5fZsq/qlG6LIrNFZP/RrV66kODSa7XClkZHub5 KiVPYnoF/fTo0IAZ55DIpBlFlR3+E8UP3ZopvbPT1Dy7y5RnDC7pTjUUxqCi4DzQimSz OxV6xHTtSN0VFczFmhRN7GT/y+aQ+laL1akaabyg6vvB+bxHJvHmCEt/pEuLC/6jtZM/ Cp9r2RkRnj1rrybw9VC/1trSbxDw0xvpQaoOCxdutPMW+rCk7WNL3XFnpxZUTqWO9YSz n1TxlFL4lRCLoRvt8eKDhH4Q/UkhikABVqmp3Z4J+lvr4mV/BPavfbEnl77Fy2i+ZrTk Mr4A== X-Gm-Message-State: AOUpUlFvEBJt9eIxmy5vTj+lm3R2W9T1tZVgvgP78XdlypbjXGSGVGXr 2PKZy+5f0Fwoiuq6R2hsEyVUSQOJQ3hs+MuGf6Tx1Njp X-Google-Smtp-Source: AA+uWPzFkF1krr4JGdEoO510ZODsTuqYRQGSFJlNsARhhJ0UggPwuC14utNrcj0vgbtCegWITYqyIJbpqDhfCrlqt+g= X-Received: by 2002:adf:fec8:: with SMTP id q8-v6mr1601993wrs.164.1533823743553; Thu, 09 Aug 2018 07:09:03 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a5d:6912:0:0:0:0:0 with HTTP; Thu, 9 Aug 2018 07:09:02 -0700 (PDT) In-Reply-To: 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> <23c3c4c3-461f-7e6a-a8ca-0574021c1ccf@redhat.com> <1029983813.116236.1533303568344.JavaMail.zimbra@lsd.ufcg.edu.br> <70d053a8-eeef-8a9f-0454-9374f716f28e@redhat.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Thu, 9 Aug 2018 16:09:02 +0200 Message-ID: To: "Zhang, Chao B" Cc: Laszlo Ersek , =?UTF-8?Q?Ricardo_Ara=C3=BAjo?= , "edk2-devel@lists.01.org" , "Gao, Liming" , "Zeng, Star" 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, 09 Aug 2018 14:09:07 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Mon, Aug 6, 2018 at 5:26 PM, Zhang, Chao B wrot= e: > 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 La= szlo Ersek > Sent: Friday, August 3, 2018 10:46 PM > To: Ricardo Ara=C3=BAjo ; Zhang, Chao B > Cc: edk2-devel@lists.01.org; Zeng, Star ; Gao, Limin= g > Subject: Re: [edk2] Missing boot related measurements at TPM 2.0 PCRs 0-7= with OVMF > > On 08/03/18 15:39, Ricardo Ara=C3=BAjo wrote: >> Hi folks, sorry for the delay! >> >> I've just applied the patch and got the same error message and empty PCR= s. > > 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=3D1075 > > 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=C3=A9, for triaging / analys= is. > swtpm is not set up on my end, and the TPM2 enablement for OvmfPkg was > contributed by Marc-Andr=C3=A9. Marc-Andr=C3=A9, are you OK with this? Th= e BZ > assignment is about root-causing the issue, at the moment. That fixes the problem for me: - Constructor =3D Tpm2DeviceLibConstructor + CONSTRUCTOR =3D 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 =3D Tpm2DeviceLib + LIBRARY_CLASS =3D 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" > >> Para: "Laszlo Ersek" >, "Ric= ardo Ara=C3=BAjo" >= , "Marc-Andr=C3=A9 Lureau" > >> Cc: edk2-devel@lists.01.org, "Gao, Limin= g" >, "Zeng, Star" > >> 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 = >; Ricardo Ara=C3=BAjo >; Marc-Andr=C3=A9 Lureau > >> Cc: edk2-devel@lists.01.org; Gao, Liming= >; Zeng, Star > >> 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 regi= ster. The interface type should not change within boot cycle according to P= TP spec. >>> I agree to add some ASSERT after PCDSetxxsS. >>> But It is a core solution without platform change as PCD has been confi= gured as DYN, DYNEx in DEC. I don=E2=80=99t 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=C3=BAjo < ricardo@lsd.ufcg.edu.br >; Zhang, Chao B < chao.b.zhang@intel.com >; Marc-Andr=C3=A9 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=C3=BAjo wrote: >>>> The commit I was referring to is: >>>> https://github.com/tianocore/edk2/commit/f15cb995bb3880b77e15afe6facd3= da05e599a17 >>>> >>>> Regards, >>>> >>>> Ricardo Araujo - >>>> www.lsd.ufcg.edu.br/~ricardo > >>>> >>>> ----- Mensagem original ----- >>>> De: "Ricardo Ara=C3=BAjo" < 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=3DTRUE -D SECURE_BOOT_ENABLE=3DTRUE -D >>>> HTTP_BOOT_ENABLE=3DTRUE. Is there anything else I need to add to enabl= e >>>> 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=C3=A9 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=C3=A9: 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) =3D=3D TRUE >>>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x= 00, 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) =3D=3D TRUE >>>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x= 00, 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) =3D=3D TRUE >>>> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0x00, 0x00, 0x00, 0x= 00, 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=3D1070 >>> >>> Thanks >>> Laszlo >>> >> >> > > _______________________________________________ > edk2-devel mailing list > 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 --=20 Marc-Andr=C3=A9 Lureau