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

[Laszlo Ersek <lersek@redhat.com>]

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