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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82729): https://edk2.groups.io/g/devel/message/82729
Mute This Topic: https://groups.io/mt/86163959/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
