Re: [PATCH V2 3/3] MdeModulePkg: Variable: Update PCR[7] measure for new TCG spec

["Yao, Jiewen" <jiewen.yao@intel.com>]

I think below check is unnecessary, because if we can set PK, secure boot variable must exist.
"if ((SecureBootFollowUpdate.VarData != NULL)"

I believe we can use "ASSERT (SecureBootFollowUpdate.VarData != NULL);"

With that change, reviewed-by: Jiewen.yao@intel.com

Thank you
Yao Jiewen

> -----Original Message-----
> From: Zhang, Chao B
> Sent: Thursday, January 19, 2017 1:14 PM
> To: edk2-devel@lists.01.org
> Cc: yao.jiewen@intel.com; Zeng, Star <star.zeng@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
> Subject: [PATCH V2 3/3] MdeModulePkg: Variable: Update PCR[7] measure for
> new TCG spec
> 
> Measure DBT into PCR[7] when it is updated between initial measure and
> ExitBootService. Measure "SecureBoot" change after PK update.
> Spec version : TCG PC Client PFP 00.37.
> http://www.trustedcomputinggroup.org/wp-content/uploads/PC-ClientSpecific
> _Platform_Profile_for_TPM_2p0_Systems_v21.pdf
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Yao Jiewen <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
> ---
>  .../Universal/Variable/RuntimeDxe/Measurement.c    | 88
> +++++++++++++++++++++-
>  .../Universal/Variable/RuntimeDxe/VariableDxe.c    | 17 +++++
>  .../Variable/RuntimeDxe/VariableSmmRuntimeDxe.c    | 17 +++++
>  3 files changed, 121 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> index 2f92fae..707f988 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Measure TrEE required variable.
> 
> -Copyright (c) 2013 - 2014, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be found
> at
> @@ -36,6 +36,24 @@ VARIABLE_TYPE  mVariableType[] = {
>    {EFI_KEY_EXCHANGE_KEY_NAME,    &gEfiGlobalVariableGuid},
>    {EFI_IMAGE_SECURITY_DATABASE,  &gEfiImageSecurityDatabaseGuid},
>    {EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid},
> +  {EFI_IMAGE_SECURITY_DATABASE2, &gEfiImageSecurityDatabaseGuid},
> +};
> +
> +typedef struct {
> +  CHAR16                                 *VariableName;
> +  EFI_GUID                               *VendorGuid;
> +  UINT8                                  *VarData;
> +  UINTN                                  VarDataSize;
> +} VARIABLE_FOLLOW_TYPE;
> +
> +//
> +// "SecureBoot" may update following PK Del/Add
> +//
> +static VARIABLE_FOLLOW_TYPE  SecureBootFollowUpdate = {
> +   EFI_SECURE_BOOT_MODE_NAME,
> +   &gEfiGlobalVariableGuid,
> +   NULL,
> +   0,
>  };
> 
>  /**
> @@ -251,5 +269,73 @@ SecureBootHook (
>      FreePool (VariableData);
>    }
> 
> +  //
> +  // "SecureBoot" is 8bit & read-only. It can only be changed according to PK
> update
> +  //
> +  if ((StrCmp (VariableName, EFI_PLATFORM_KEY_NAME) == 0) &&
> +       CompareGuid (VendorGuid, &gEfiGlobalVariableGuid)) {
> +     Status = InternalGetVariable (
> +                SecureBootFollowUpdate.VariableName,
> +                SecureBootFollowUpdate.VendorGuid,
> +                &VariableData,
> +                &VariableDataSize
> +                );
> +     if (EFI_ERROR (Status)) {
> +       return;
> +     }
> +
> +     if ((SecureBootFollowUpdate.VarData != NULL) &&
> +         (CompareMem(SecureBootFollowUpdate.VarData, VariableData,
> VariableDataSize) != 0)) {
> +       FreePool(SecureBootFollowUpdate.VarData);
> +       SecureBootFollowUpdate.VarData = VariableData;
> +       SecureBootFollowUpdate.VarDataSize = VariableDataSize;
> +
> +       DEBUG((DEBUG_INFO, "%s variable updated according to PK change.
> Remeasure the value!\n", SecureBootFollowUpdate.VariableName));
> +       Status = MeasureVariable (
> +                  SecureBootFollowUpdate.VariableName,
> +                  SecureBootFollowUpdate.VendorGuid,
> +                  SecureBootFollowUpdate.VarData,
> +                  SecureBootFollowUpdate.VarDataSize
> +                  );
> +       DEBUG ((DEBUG_INFO, "MeasureBootPolicyVariable - %r\n", Status));
> +     } else {
> +       //
> +       // "SecureBoot" variable is not changed
> +       //
> +       FreePool(VariableData);
> +     }
> +  }
> +
>    return ;
>  }
> +
> +/**
> +  Some Secure Boot Policy Variable may update following other variable
> changes(SecureBoot follows PK change, etc).
> +  Record their initial State when variable write service is ready.
> +
> +**/
> +VOID
> +EFIAPI
> +RecordSecureBootPolicyVarFollow(
> +  VOID
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  //
> +  // Record initial "SecureBoot" variable value.
> +  // It is used to detect SecureBoot variable change in SecureBootHook.
> +  //
> +  Status = InternalGetVariable (
> +             SecureBootFollowUpdate.VariableName,
> +             SecureBootFollowUpdate.VendorGuid,
> +             (VOID **)&SecureBootFollowUpdate.VarData,
> +             &SecureBootFollowUpdate.VarDataSize
> +             );
> +  if (EFI_ERROR(Status)) {
> +    //
> +    // Read could fail when Auth Variable solution is not supported
> +    //
> +    DEBUG((DEBUG_INFO, "RecordSecureBootPolicyVarFollow GetVariable %s
> Status %x\n", SecureBootFollowUpdate.VariableName, Status));
> +  }
> +}
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> index 3d3cd24..5d81f87 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> @@ -32,6 +32,17 @@ EDKII_VAR_CHECK_PROTOCOL            mVarCheck
> = { VarCheckRegis
> 
> VarCheckVariablePropertyGet };
> 
>  /**
> +  Some Secure Boot Policy Variable may update following other variable
> changes(SecureBoot follows PK change, etc).
> +  Record their initial State when variable write service is ready.
> +
> +**/
> +VOID
> +EFIAPI
> +RecordSecureBootPolicyVarFollow(
> +  VOID
> +  );
> +
> +/**
>    Return TRUE if ExitBootServices () has been called.
> 
>    @retval TRUE If ExitBootServices () has been called.
> @@ -415,6 +426,12 @@ FtwNotificationEvent (
>    }
> 
>    //
> +  // Some Secure Boot Policy Var (SecureBoot, etc) updates following other
> +  // Secure Boot Policy Variable change. Record their initial value.
> +  //
> +  RecordSecureBootPolicyVarFollow();
> +
> +  //
>    // Install the Variable Write Architectural protocol.
>    //
>    Status = gBS->InstallProtocolInterface (
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> index 0a076ae..3d0925d 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> @@ -71,6 +71,17 @@ SecureBootHook (
>    );
> 
>  /**
> +  Some Secure Boot Policy Variable may update following other variable
> changes(SecureBoot follows PK change, etc).
> +  Record their initial State when variable write service is ready.
> +
> +**/
> +VOID
> +EFIAPI
> +RecordSecureBootPolicyVarFollow(
> +  VOID
> +  );
> +
> +/**
>    Acquires lock only at boot time. Simply returns at runtime.
> 
>    This is a temperary function that will be removed when
> @@ -1079,6 +1090,12 @@ SmmVariableWriteReady (
>      return;
>    }
> 
> +  //
> +  // Some Secure Boot Policy Var (SecureBoot, etc) updates following other
> +  // Secure Boot Policy Variable change.  Record their initial value.
> +  //
> +  RecordSecureBootPolicyVarFollow();
> +
>    Status = gBS->InstallProtocolInterface (
>                    &mHandle,
>                    &gEfiVariableWriteArchProtocolGuid,
> --
> 1.9.5.msysgit.1