Re: [Qemu-devel] [edk2] [PATCH v3 3/4] OvmfPkg: add Tcg2PhysicalPresenceLibQemu

[Laszlo Ersek]

On 05/18/18 14:23, address@hidden 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 <address@hidden>

Thanks
Laszlo