On 05/18/18 14:23, marcandre.lur...@redhat.com wrote: > +/** > + Initializes QEMU PPI memory region. > + > + @retval EFI_SUCCESS Operation completed successfully. > + @retval EFI_PROTOCOL_ERROR PPI address is invalid. > +**/ > +STATIC > +EFI_STATUS > +QemuTpmInitPPI ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + QEMU_FWCFG_TPM_CONFIG Config; > + EFI_PHYSICAL_ADDRESS PpiAddress64; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor; > + int i;
(1) This should be "UINTN Idx". > + > + if (mPpi != NULL) { > + return EFI_SUCCESS; > + } > + > + Status = QemuTpmReadConfig (&Config); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + mPpi = (QEMU_TPM_PPI *)(UINTN)Config.PpiAddress; > + if (mPpi == NULL) { > + return EFI_PROTOCOL_ERROR; > + } > + > + DEBUG ((DEBUG_INFO, "[TPM2PP] mPpi=%p version=%d\n", mPpi, > Config.TpmVersion)); > + > + PpiAddress64 = (UINTN)mPpi; > + if ((PpiAddress64 & ~(UINT64)EFI_PAGE_MASK) != > + ((PpiAddress64 + sizeof *mPpi - 1) & ~(UINT64)EFI_PAGE_MASK)) { > + DEBUG ((DEBUG_ERROR, "[TPM2PP] mPpi crosses a page boundary\n")); > + return EFI_PROTOCOL_ERROR; > + } > + > + Status = gDS->GetMemorySpaceDescriptor (PpiAddress64, &Descriptor); > + if (Status != EFI_SUCCESS && Status != EFI_NOT_FOUND) { > + ASSERT_EFI_ERROR (Status); > + return EFI_PROTOCOL_ERROR; > + } > + if (Status == EFI_SUCCESS && ( > + Descriptor.GcdMemoryType != EfiGcdMemoryTypeMemoryMappedIo && > + Descriptor.GcdMemoryType != EfiGcdMemoryTypeNonExistent)) { > + DEBUG ((DEBUG_ERROR, "[TPM2PP] mPpi has an invalid memory type\n")); > + return EFI_PROTOCOL_ERROR; > + } (2) In edk2 we generally don't compare EFI_STATUS variables against EFI_SUCCESS, instead we write [!]EFI_ERROR (Status). Whether that's easier or harder to understand is anyone's guess, but either way that's the idiom. (3) The indentation / parentheses around "Descriptor.GcdMemoryType" aren't idiomatic. (4) I think the new checks introduce a new logic bug in this patch -- if we return EFI_PROTOCOL_ERROR, we should re-set mPpi to NULL. Otherwise the next call to QemuTpmInitPPI() will succeed, using a bogus mPpi value. > + > + for (i = 0; i < sizeof (mPpi->Func); i++) { > + mPpi->Func[i] = 0; > + } (5) Functionally this is OK, because mPpi->Func has UINT8 elements; however we should really use ARRAY_SIZE() here. > + if (Config.TpmVersion == QEMU_TPM_VERSION_2) { > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_NO_ACTION] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_CLEAR] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_ENABLE_CLEAR] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_ENABLE_CLEAR_2] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_ENABLE_CLEAR_3] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_SET_PCR_BANKS] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_CHANGE_EPS] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_LOG_ALL_DIGESTS] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_ENABLE_BLOCK_SID] = TPM_PPI_FLAGS; > + mPpi->Func[TCG2_PHYSICAL_PRESENCE_DISABLE_BLOCK_SID] = TPM_PPI_FLAGS; > + } > + > + if (mPpi->In == 0) { > + mPpi->In = 1; > + mPpi->Request = TCG2_PHYSICAL_PRESENCE_NO_ACTION; > + mPpi->LastRequest = TCG2_PHYSICAL_PRESENCE_NO_ACTION; > + mPpi->NextStep = TCG2_PHYSICAL_PRESENCE_NO_ACTION; > + } > + > + return EFI_SUCCESS; > +} Thus, I've squashed the following patch into yours, and build-tested it: > diff --git > a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c > b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c > index cc4046228ba3..52913f573676 100644 > --- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c > +++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c > @@ -99,7 +99,7 @@ QemuTpmInitPPI ( > QEMU_FWCFG_TPM_CONFIG Config; > EFI_PHYSICAL_ADDRESS PpiAddress64; > EFI_GCD_MEMORY_SPACE_DESCRIPTOR Descriptor; > - int i; > + UINTN Idx; > > if (mPpi != NULL) { > return EFI_SUCCESS; > @@ -121,23 +121,23 @@ QemuTpmInitPPI ( > if ((PpiAddress64 & ~(UINT64)EFI_PAGE_MASK) != > ((PpiAddress64 + sizeof *mPpi - 1) & ~(UINT64)EFI_PAGE_MASK)) { > DEBUG ((DEBUG_ERROR, "[TPM2PP] mPpi crosses a page boundary\n")); > - return EFI_PROTOCOL_ERROR; > + goto InvalidPpiAddress; > } > > Status = gDS->GetMemorySpaceDescriptor (PpiAddress64, &Descriptor); > - if (Status != EFI_SUCCESS && Status != EFI_NOT_FOUND) { > + if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND) { > ASSERT_EFI_ERROR (Status); > - return EFI_PROTOCOL_ERROR; > + goto InvalidPpiAddress; > } > - if (Status == EFI_SUCCESS && ( > - Descriptor.GcdMemoryType != EfiGcdMemoryTypeMemoryMappedIo && > - Descriptor.GcdMemoryType != EfiGcdMemoryTypeNonExistent)) { > + if (!EFI_ERROR (Status) && > + (Descriptor.GcdMemoryType != EfiGcdMemoryTypeMemoryMappedIo && > + Descriptor.GcdMemoryType != EfiGcdMemoryTypeNonExistent)) { > DEBUG ((DEBUG_ERROR, "[TPM2PP] mPpi has an invalid memory type\n")); > - return EFI_PROTOCOL_ERROR; > + goto InvalidPpiAddress; > } > > - for (i = 0; i < sizeof (mPpi->Func); i++) { > - mPpi->Func[i] = 0; > + for (Idx = 0; Idx < ARRAY_SIZE (mPpi->Func); Idx++) { > + mPpi->Func[Idx] = 0; > } > if (Config.TpmVersion == QEMU_TPM_VERSION_2) { > mPpi->Func[TCG2_PHYSICAL_PRESENCE_NO_ACTION] = TPM_PPI_FLAGS; > @@ -160,6 +160,10 @@ QemuTpmInitPPI ( > } > > return EFI_SUCCESS; > + > +InvalidPpiAddress: > + mPpi = NULL; > + return EFI_PROTOCOL_ERROR; > } With that: Reviewed-by: Laszlo Ersek <ler...@redhat.com> Thanks Laszlo