From: "Wang, Jian J" <jian.j.wang@intel.com>
To: "Zhang, Qi1" <qi1.zhang@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>,
"Kumar, Rahul1" <rahul1.kumar@intel.com>
Subject: Re: [PATCH v4 4/6] SecurityPkg/Tcg2: Add TcgPpi
Date: Tue, 21 Jul 2020 08:52:33 +0000 [thread overview]
Message-ID: <BYAPR11MB33032DB48916D3F45F54440FB6780@BYAPR11MB3303.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20200717084935.11067-5-qi1.zhang@intel.com>
Qi,
A few comments below.
Regards,
Jian
> -----Original Message-----
> From: Zhang, Qi1 <qi1.zhang@intel.com>
> Sent: Friday, July 17, 2020 4:50 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Zhang, Qi1 <qi1.zhang@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: [PATCH v4 4/6] SecurityPkg/Tcg2: Add TcgPpi
>
> From: Jiewen Yao <jiewen.yao@intel.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2841
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Qi Zhang <qi1.zhang@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
> SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c | 110 +++++++++++++++++++++-------
> SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf | 3 +-
> 2 files changed, 86 insertions(+), 27 deletions(-)
>
> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> index 19b8e4b318..592f760057 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.c
> @@ -1,7 +1,7 @@
> /** @file
>
> Initialize TPM2 device and measure FVs before handing off control to DXE.
>
>
>
> -Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
>
> +Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
>
> Copyright (c) 2017, Microsoft Corporation. All rights reserved. <BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -17,6 +17,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Ppi/EndOfPeiPhase.h>
>
> #include <Ppi/FirmwareVolumeInfoMeasurementExcluded.h>
>
> #include <Ppi/FirmwareVolumeInfoPrehashedFV.h>
>
> +#include <Ppi/Tcg.h>
>
>
>
> #include <Guid/TcgEventHob.h>
>
> #include <Guid/MeasuredFvHob.h>
>
> @@ -66,6 +67,48 @@ EFI_PEI_PPI_DESCRIPTOR mTpmInitializationDonePpiList
> = {
> NULL
>
> };
>
>
>
> +/**
>
> + Do a hash operation on a data buffer, extend a specific TPM PCR with the
> hash result,
>
> + and build a GUIDed HOB recording the event which will be passed to the DXE
> phase and
>
> + added into the Event Log.
>
> +
>
> + @param[in] This Indicates the calling context
>
> + @param[in] Flags Bitmap providing additional information.
>
> + @param[in] HashData If BIT0 of Flags is 0, it is physical address of the
>
> + start of the data buffer to be hashed, extended, and logged.
>
> + If BIT0 of Flags is 1, it is physical address of the
>
> + start of the pre-hash data buffter to be extended, and logged.
>
> + The pre-hash data format is TPML_DIGEST_VALUES.
>
> + @param[in] HashDataLen The length, in bytes, of the buffer referenced by
> HashData.
>
> + @param[in] NewEventHdr Pointer to a TCG_PCR_EVENT_HDR data
> structure.
>
> + @param[in] NewEventData Pointer to the new event data.
>
> +
>
> + @retval EFI_SUCCESS Operation completed successfully.
>
> + @retval EFI_OUT_OF_RESOURCES No enough memory to log the new event.
>
> + @retval EFI_DEVICE_ERROR The command was unsuccessful.
>
> +
>
> +**/
>
> +EFI_STATUS
>
> +EFIAPI
>
> +HashLogExtendEvent (
>
> + IN EDKII_TCG_PPI *This,
>
> + IN UINT64 Flags,
>
> + IN UINT8 *HashData,
>
> + IN UINTN HashDataLen,
>
> + IN TCG_PCR_EVENT_HDR *NewEventHdr,
>
> + IN UINT8 *NewEventData
>
> + );
>
> +
>
> +EDKII_TCG_PPI mEdkiiTcgPpi = {
>
> + HashLogExtendEvent
>
> +};
>
> +
>
> +EFI_PEI_PPI_DESCRIPTOR mTcgPpiList = {
>
> + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
>
> + &gEdkiiTcgPpiGuid,
>
> + &mEdkiiTcgPpi
>
> +};
>
> +
>
> //
>
> // Number of firmware blobs to grow by each time we run out of room
>
> //
>
> @@ -375,9 +418,13 @@ LogHashEvent (
> and build a GUIDed HOB recording the event which will be passed to the DXE
> phase and
>
> added into the Event Log.
>
>
>
> + @param[in] This Indicates the calling context
>
> @param[in] Flags Bitmap providing additional information.
>
> - @param[in] HashData Physical address of the start of the data buffer
>
> - to be hashed, extended, and logged.
>
> + @param[in] HashData If BIT0 of Flags is 0, it is physical address of the
>
> + start of the data buffer to be hashed, extended, and logged.
>
> + If BIT0 of Flags is 1, it is physical address of the
>
> + start of the pre-hash data buffter to be extended, and logged.
>
> + The pre-hash data format is TPML_DIGEST_VALUES.
>
> @param[in] HashDataLen The length, in bytes, of the buffer referenced by
> HashData.
>
> @param[in] NewEventHdr Pointer to a TCG_PCR_EVENT_HDR data
> structure.
>
> @param[in] NewEventData Pointer to the new event data.
>
> @@ -388,7 +435,9 @@ LogHashEvent (
>
>
> **/
>
> EFI_STATUS
>
> +EFIAPI
>
> HashLogExtendEvent (
>
> + IN EDKII_TCG_PPI *This,
>
> IN UINT64 Flags,
>
> IN UINT8 *HashData,
>
> IN UINTN HashDataLen,
>
> @@ -403,16 +452,23 @@ HashLogExtendEvent (
> return EFI_DEVICE_ERROR;
>
> }
>
>
>
> - Status = HashAndExtend (
>
> - NewEventHdr->PCRIndex,
>
> - HashData,
>
> - HashDataLen,
>
> + if(Flags & EDKII_TCG_PRE_HASH) {
>
> + ZeroMem (&DigestList, sizeof(DigestList));
>
> + CopyMem(&DigestList, HashData, sizeof(DigestList));
Missing a space before '('.
>
> + Status = Tpm2PcrExtend(
Missing a space before '('.
>
> + 0,
>
> &DigestList
>
> );
>
> + } else {
>
> + Status = HashAndExtend (
>
> + NewEventHdr->PCRIndex,
>
> + HashData,
>
> + HashDataLen,
>
> + &DigestList
>
> + );
>
> + }
>
> if (!EFI_ERROR (Status)) {
>
> - if ((Flags & EFI_TCG2_EXTEND_ONLY) == 0) {
>
> - Status = LogHashEvent (&DigestList, NewEventHdr, NewEventData);
>
> - }
>
> + Status = LogHashEvent (&DigestList, NewEventHdr, NewEventData);
>
> }
>
>
>
> if (Status == EFI_DEVICE_ERROR) {
>
> @@ -452,6 +508,7 @@ MeasureCRTMVersion (
> TcgEventHdr.EventSize = (UINT32) StrSize((CHAR16*)PcdGetPtr
> (PcdFirmwareVersionString));
>
>
>
> return HashLogExtendEvent (
>
> + &mEdkiiTcgPpi,
>
> 0,
>
> (UINT8*)PcdGetPtr (PcdFirmwareVersionString),
>
> TcgEventHdr.EventSize,
>
> @@ -651,27 +708,22 @@ MeasureFvImage (
> // FV pre-hash algos comply with current TPM hash requirement
>
> // Skip hashing step in measure, only extend DigestList to PCR and log event
>
> //
>
> - Status = Tpm2PcrExtend(
>
> - 0,
>
> - &DigestList
>
> + Status = HashLogExtendEvent (
>
> + &mEdkiiTcgPpi,
>
> + EDKII_TCG_PRE_HASH,
>
> + (UINT8*) &DigestList, // HashData
>
> + (UINTN) sizeof(DigestList), // HashDataLen
>
> + &TcgEventHdr, // EventHdr
>
> + EventData // EventData
>
> );
>
> -
>
> - if (!EFI_ERROR(Status)) {
>
> - Status = LogHashEvent (&DigestList, &TcgEventHdr, EventData);
>
> - DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & logged by
> Tcg2Pei starts at: 0x%x\n", FvBase));
>
> - DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & logged by
> Tcg2Pei has the size: 0x%x\n", FvLength));
>
> - } else if (Status == EFI_DEVICE_ERROR) {
>
> - BuildGuidHob (&gTpmErrorHobGuid,0);
>
> - REPORT_STATUS_CODE (
>
> - EFI_ERROR_CODE | EFI_ERROR_MINOR,
>
> - (PcdGet32 (PcdStatusCodeSubClassTpmDevice) |
> EFI_P_EC_INTERFACE_ERROR)
>
> - );
>
> - }
Please explain the purpose of the removal of this part of code in commit message.
It looks that it's not a relevant change to this patch series.
>
> + DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & logged by
> Tcg2Pei starts at: 0x%x\n", FvBase));
>
> + DEBUG ((DEBUG_INFO, "The pre-hashed FV which is extended & logged by
> Tcg2Pei has the size: 0x%x\n", FvLength));
>
> } else {
>
> //
>
> // Hash the FV, extend digest to the TPM and log TCG event
>
> //
>
> Status = HashLogExtendEvent (
>
> + &mEdkiiTcgPpi,
>
> 0,
>
> (UINT8*) (UINTN) FvBase, // HashData
>
> (UINTN) FvLength, // HashDataLen
>
> @@ -849,6 +901,12 @@ PeimEntryMP (
> {
>
> EFI_STATUS Status;
>
>
>
> + //
>
> + // install Tcg Services
>
> + //
>
> + Status = PeiServicesInstallPpi (&mTcgPpiList);
>
> + ASSERT_EFI_ERROR (Status);
>
> +
>
> if (PcdGet8 (PcdTpm2ScrtmPolicy) == 1) {
>
> Status = MeasureCRTMVersion ();
>
> }
>
> @@ -893,7 +951,7 @@ MeasureSeparatorEventWithError (
> TcgEvent.PCRIndex = PCRIndex;
>
> TcgEvent.EventType = EV_SEPARATOR;
>
> TcgEvent.EventSize = (UINT32)sizeof (EventData);
>
> - return HashLogExtendEvent(0,(UINT8 *)&EventData, TcgEvent.EventSize,
> &TcgEvent,(UINT8 *)&EventData);
>
> + return HashLogExtendEvent(&mEdkiiTcgPpi, 0, (UINT8 *)&EventData,
> TcgEvent.EventSize, &TcgEvent,(UINT8 *)&EventData);
>
> }
>
>
>
> /**
>
> diff --git a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> index 3d361e8859..f64b29f1ae 100644
> --- a/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> +++ b/SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> @@ -8,7 +8,7 @@
> #
>
> # This module will initialize TPM device, measure reported FVs and BIOS version.
>
> #
>
> -# Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
>
> +# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
>
> # Copyright (c) 2017, Microsoft Corporation. All rights reserved. <BR>
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -72,6 +72,7 @@
> gPeiTpmInitializationDonePpiGuid ## PRODUCES
>
> gEfiEndOfPeiSignalPpiGuid ## SOMETIMES_CONSUMES
> ## NOTIFY
>
> gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid ##
> SOMETIMES_CONSUMES
>
> + gEdkiiTcgPpiGuid ## PRODUCES
>
>
>
> [Pcd]
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString ##
> SOMETIMES_CONSUMES
>
> --
> 2.26.2.windows.1
next prev parent reply other threads:[~2020-07-21 8:52 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-17 8:49 [PATCH v4 0/6] Add capability to let PEIM extend TcgEvent Qi Zhang
2020-07-17 8:49 ` [PATCH v4 1/6] SecurityPkg/TcgPpi: Add TcgPpi header file Qi Zhang
2020-07-21 8:37 ` Wang, Jian J
2020-07-17 8:49 ` [PATCH v4 2/6] SecurityPkg/dec: Add TcgPpi Qi Zhang
2020-07-21 8:37 ` Wang, Jian J
2020-07-17 8:49 ` [PATCH v4 3/6] SecurityPkg/Tcg: " Qi Zhang
2020-07-21 8:36 ` Wang, Jian J
2020-07-17 8:49 ` [PATCH v4 4/6] SecurityPkg/Tcg2: " Qi Zhang
2020-07-21 8:52 ` Wang, Jian J [this message]
2020-07-17 8:49 ` [PATCH v4 5/6] SecurityPkg/PeiTpmMeasurementLib: Add PEI instance Qi Zhang
2020-07-21 9:00 ` Wang, Jian J
[not found] ` <1623B8ED1BE72736.9882@groups.io>
2020-07-21 9:16 ` [edk2-devel] " Wang, Jian J
2020-07-21 15:05 ` Yao, Jiewen
2020-07-22 1:08 ` Wang, Jian J
2020-07-22 2:08 ` Qi Zhang
2020-07-22 2:13 ` Yao, Jiewen
2020-07-17 8:49 ` [PATCH v4 6/6] SecurityPkg/dsc: Add PeiTpmMeasurementLib Qi Zhang
2020-07-21 9:00 ` Wang, Jian J
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BYAPR11MB33032DB48916D3F45F54440FB6780@BYAPR11MB3303.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox