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....@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.jie...@intel.com; Zeng, Star <star.z...@intel.com>; Yao, Jiewen > <jiewen....@intel.com>; Zhang, Chao B <chao.b.zh...@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.z...@intel.com> > Cc: Yao Jiewen <jiewen....@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Chao Zhang <chao.b.zh...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
