From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 30D36819E7 for ; Sat, 21 Jan 2017 18:20:35 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP; 21 Jan 2017 18:20:35 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,267,1477983600"; d="scan'208";a="33732094" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga002.jf.intel.com with ESMTP; 21 Jan 2017 18:20:34 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sat, 21 Jan 2017 18:20:34 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sat, 21 Jan 2017 18:20:34 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX103.ccr.corp.intel.com ([10.239.4.69]) with mapi id 14.03.0248.002; Sun, 22 Jan 2017 10:20:32 +0800 From: "Yao, Jiewen" To: "Zhang, Chao B" , "edk2-devel@lists.01.org" CC: "Zeng, Star" Thread-Topic: [PATCH V2 3/3] MdeModulePkg: Variable: Update PCR[7] measure for new TCG spec Thread-Index: AQHSchLeHTIk6ClILU+4HElXALbzYaFDx5Sw Date: Sun, 22 Jan 2017 02:20:31 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A8E3F92@shsmsx102.ccr.corp.intel.com> References: <20170119051359.14044-1-chao.b.zhang@intel.com> <20170119051359.14044-3-chao.b.zhang@intel.com> In-Reply-To: <20170119051359.14044-3-chao.b.zhang@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH V2 3/3] MdeModulePkg: Variable: Update PCR[7] measure for new TCG spec X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 22 Jan 2017 02:20:35 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I think below check is unnecessary, because if we can set PK, secure boot v= ariable must exist. "if ((SecureBootFollowUpdate.VarData !=3D NULL)" I believe we can use "ASSERT (SecureBootFollowUpdate.VarData !=3D 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 ; Yao, Jiewen > ; Zhang, Chao B > Subject: [PATCH V2 3/3] MdeModulePkg: Variable: Update PCR[7] measure for > new TCG spec >=20 > 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 >=20 > Cc: Star Zeng > Cc: Yao Jiewen > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Chao Zhang > --- > .../Universal/Variable/RuntimeDxe/Measurement.c | 88 > +++++++++++++++++++++- > .../Universal/Variable/RuntimeDxe/VariableDxe.c | 17 +++++ > .../Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 17 +++++ > 3 files changed, 121 insertions(+), 1 deletion(-) >=20 > 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. >=20 > -Copyright (c) 2013 - 2014, Intel Corporation. All rights reserved.
> +Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e found > at > @@ -36,6 +36,24 @@ VARIABLE_TYPE mVariableType[] =3D { > {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 =3D { > + EFI_SECURE_BOOT_MODE_NAME, > + &gEfiGlobalVariableGuid, > + NULL, > + 0, > }; >=20 > /** > @@ -251,5 +269,73 @@ SecureBootHook ( > FreePool (VariableData); > } >=20 > + // > + // "SecureBoot" is 8bit & read-only. It can only be changed according = to PK > update > + // > + if ((StrCmp (VariableName, EFI_PLATFORM_KEY_NAME) =3D=3D 0) && > + CompareGuid (VendorGuid, &gEfiGlobalVariableGuid)) { > + Status =3D InternalGetVariable ( > + SecureBootFollowUpdate.VariableName, > + SecureBootFollowUpdate.VendorGuid, > + &VariableData, > + &VariableDataSize > + ); > + if (EFI_ERROR (Status)) { > + return; > + } > + > + if ((SecureBootFollowUpdate.VarData !=3D NULL) && > + (CompareMem(SecureBootFollowUpdate.VarData, VariableData, > VariableDataSize) !=3D 0)) { > + FreePool(SecureBootFollowUpdate.VarData); > + SecureBootFollowUpdate.VarData =3D VariableData; > + SecureBootFollowUpdate.VarDataSize =3D VariableDataSize; > + > + DEBUG((DEBUG_INFO, "%s variable updated according to PK change. > Remeasure the value!\n", SecureBootFollowUpdate.VariableName)); > + Status =3D 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 =3D 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 > =3D { VarCheckRegis >=20 > VarCheckVariablePropertyGet }; >=20 > /** > + 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. >=20 > @retval TRUE If ExitBootServices () has been called. > @@ -415,6 +426,12 @@ FtwNotificationEvent ( > } >=20 > // > + // Some Secure Boot Policy Var (SecureBoot, etc) updates following oth= er > + // Secure Boot Policy Variable change. Record their initial value. > + // > + RecordSecureBootPolicyVarFollow(); > + > + // > // Install the Variable Write Architectural protocol. > // > Status =3D 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 ( > ); >=20 > /** > + 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. >=20 > This is a temperary function that will be removed when > @@ -1079,6 +1090,12 @@ SmmVariableWriteReady ( > return; > } >=20 > + // > + // Some Secure Boot Policy Var (SecureBoot, etc) updates following oth= er > + // Secure Boot Policy Variable change. Record their initial value. > + // > + RecordSecureBootPolicyVarFollow(); > + > Status =3D gBS->InstallProtocolInterface ( > &mHandle, > &gEfiVariableWriteArchProtocolGuid, > -- > 1.9.5.msysgit.1