Re: [edk2-devel] [PATCH V2 2/3] SecurityPkg: Support TdProtocol in DxeTpm2MeasureBootLib

["Min Xu" <min.m.xu@intel.com>]

On October 19, 2021 9:23 PM, Sami Mujawar wrote:
> >     //
> >     // Read the EFI Partition Table Header
> >     //
> > @@ -156,6 +224,15 @@ Tcg2MeasureGptTable (
> >     if (PrimaryHeader == NULL) {
> >       return EFI_OUT_OF_RESOURCES;
> >     }
> > +
> > +  //
> > +  // PrimaryHeader->SizeOfPartitionEntry should not be zero  //  if
> > + (PrimaryHeader->SizeOfPartitionEntry == 0) {
> > +    DEBUG ((DEBUG_ERROR, "SizeOfPartitionEntry should not be zero!\n"));
> > +    return EFI_BAD_BUFFER_SIZE;
> > +  }
> [SAMI] I think this check is at an incorrect location. Should this be after the
> ReadDisk() below? Also, PrimaryHeader would need to be freed in the error
> scenario above.
Good capture! It will be fixed in the next version.

> >
> > +  if (TdProtocol != NULL) {
> > +    TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event,
> EventSize);
> > +    if (TdEvent == NULL) {
> > +      goto Exit;
> [SAMI] I think Status should be set to reflect an appropriate error code here.
I am thinking if TCG2_PROTOCOL and TEE_PROTOCOL will be installed in the same time?
1) If these 2 protocols are NOT installed in the same time, then the returned status reflect the actual operation result of the protocol.
2) If these 2 protocols can be installed in the same time, then it will be a problem that the how to reflect the operation result of the protocols by the status?
I prefer 1) that these 2 protocols are NOT installed in the same time. Because it doesn't make sense to measure the boot in 2 times.
What's your suggestion?
BTW, CreateTdEventFromTcg2Event will be updated to return a status to indicate the operation result. So that the status can reflect an appropriate error code.

> Also would it be possible to create this event just before calling
> TdProtocol->HashLogExtendEvent at line 351?
> I am trying to understand why is this done differently in
> Tcg2MeasurePeImage() i.e. The TdEvent is created and extended in the same
> if (TdProtocol != NULL) block.
You're right. TdEvent should be created and extended in the same if block.  It will be fixed in the next version.
> [/SAMI]

> > +
> > +  if (TdProtocol != NULL) {
> > +    TdEvent = CreateTdEventFromTcg2Event (TdProtocol, Tcg2Event,
> EventSize);
> > +    if (TdEvent == NULL) {
> > +      goto Finish;
> [SAMI] I think Status should be set to reflect an appropriate error code here.
It will be fixed in the next version.
> > **) &Tcg2Protocol);
> > +  MeasureBootProtocols.Tcg2Protocol = NULL;
> > +  MeasureBootProtocols.TdProtocol   = NULL;
> > +
> > +  Status = GetMeasureBootProtocols(&MeasureBootProtocols);
> > +
> >     if (EFI_ERROR (Status)) {
> > -    //
> > -    // Tcg2 protocol is not installed. So, TPM2 is not present.
> > -    // Don't do any measurement, and directly return EFI_SUCCESS.
> > -    //
> [SAMI] It may be helpful to retain the oirginal comment with slight
> rewording.
Sure. It will be added and reworded in the next version. Thanks for reminder.

Thanks
Min