* [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
@ 2020-01-03 3:04 Gao, Zhichao
2020-01-03 3:04 ` [PATCH 01/13] SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata Gao, Zhichao
` (13 more replies)
0 siblings, 14 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel
Cc: Jiewen Yao, Jian J Wang, Chao Zhang, Jordan Justen, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to Tcg2PpVendorLib. It has one
more parameter PPData (type EFI_TCG2_PHYSICAL_PRESENCE) to transfer more data.
2. Use the Ex version instead of original one in Tcg2PhysicalPresenceLib
3. Add a pcd PcdPhysicalPresenceUserConfirmTimeout to control the user confirm
input key timeout.
4. Add FunctionIndex to structure type EFI_TCG2_PHYSICAL_PRESENCE to transfer
mTcgNvs->PhysicalPresence.Parameter data.
5. Add parameter FunctionIndex to Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
to initialize the PPdata.
6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to SecurityPkg/Include.
It is useful for platform code to implement their own Tcg2PhysicalPresenceLib.
7. Replace the ASSERT with error code return in TpmPhysicalPresenceLib
8. Fix one operation (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page 37).
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
Zhichao Gao (13):
SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function
SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
SecurityPkg/dec: Add a pcd for user response wait time
OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata
SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code
SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
.../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
.../DxeTcg2PhysicalPresenceLib.inf | 6 +-
.../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
.../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
.../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
.../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
.../DxeTcg2PhysicalPresenceLib.inf | 4 +-
.../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
.../DxeTcgPhysicalPresenceLib.inf | 6 +-
.../SmmTcg2PhysicalPresenceLib.c | 15 ++-
.../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
SecurityPkg/SecurityPkg.dec | 7 +-
SecurityPkg/SecurityPkg.uni | 7 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
21 files changed, 347 insertions(+), 95 deletions(-)
rename SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 01/13] SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 02/13] SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function Gao, Zhichao
` (12 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Add Tcg2PpVendorLibExecutePendingRequestEx and
Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to handle the
EFI_TCG2_PHYSICAL_PRESENCE parameters.
Add the definition first.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++++++++++++-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/SecurityPkg/Include/Library/Tcg2PpVendorLib.h b/SecurityPkg/Include/Library/Tcg2PpVendorLib.h
index 569eba6874..8f7fcf4e84 100644
--- a/SecurityPkg/Include/Library/Tcg2PpVendorLib.h
+++ b/SecurityPkg/Include/Library/Tcg2PpVendorLib.h
@@ -7,7 +7,7 @@
Caution: This function may receive untrusted input.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -16,6 +16,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define _TCG2_PP_VENDOR_LIB_H_
#include <IndustryStandard/Tpm20.h>
+#include <Guid/Tcg2PhysicalPresenceData.h>
#include <Protocol/Tcg2Protocol.h>
#include <Library/Tcg2PhysicalPresenceLib.h>
@@ -46,6 +47,33 @@ Tcg2PpVendorLibExecutePendingRequest (
OUT BOOLEAN *ResetRequired
);
+/**
+ Check and execute the requested physical presence command.
+
+ This API should be invoked in BIOS boot phase to process pending request.
+
+ Caution: This function may receive untrusted input.
+
+ If OperationRequest < 128, then ASSERT().
+
+ @param[in] PlatformAuth platform auth value. NULL means no platform auth change.
+ @param[in] PPData Ptr to EFI_TCG2_PHYSICAL_PRESENCE data.
+ @param[in, out] ManagementFlags BIOS TPM Management Flags.
+ @param[out] ResetRequired If reset is required to vendor settings in effect.
+ True, it indicates the reset is required.
+ False, it indicates the reset is not required.
+
+ @return TPM Operation Response to OS Environment.
+**/
+UINT32
+EFIAPI
+Tcg2PpVendorLibExecutePendingRequestEx (
+ IN TPM2B_AUTH *PlatformAuth, OPTIONAL
+ IN EFI_TCG2_PHYSICAL_PRESENCE *PPData,
+ IN OUT UINT32 *ManagementFlags,
+ OUT BOOLEAN *ResetRequired
+ );
+
/**
Check if there is a valid physical presence command request.
@@ -98,6 +126,30 @@ Tcg2PpVendorLibSubmitRequestToPreOSFunction (
IN UINT32 RequestParameter
);
+/**
+ The callback for TPM vendor specific physical presence which is called for
+ Submit TPM Operation Request to Pre-OS Environment and
+ Submit TPM Operation Request to Pre-OS Environment 2.
+
+ This API should be invoked in OS runtime phase to interface with ACPI method.
+
+ Caution: This function may receive untrusted input.
+
+ If OperationRequest < 128, then ASSERT().
+
+ @param[in] *PPData Ptr to EFI_TCG2_PHYSICAL_PRESENCE data.
+ @param[in] ManagementFlags BIOS TPM Management Flags.
+
+ @return Return Code for Submit TPM Operation Request to Pre-OS Environment and
+ Submit TPM Operation Request to Pre-OS Environment 2.
+**/
+UINT32
+EFIAPI
+Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx (
+ IN EFI_TCG2_PHYSICAL_PRESENCE *PPdata,
+ IN UINT32 ManagementFlags
+ );
+
/**
The callback for TPM vendor specific physical presence which is called for
Get User Confirmation Status for Operation.
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 02/13] SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
2020-01-03 3:04 ` [PATCH 01/13] SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 03/13] SecurityPkg/Tcg2PhysicalPresenceLib: Use the " Gao, Zhichao
` (11 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Add Tcg2PpVendorLibExecutePendingRequestEx and
Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to handle the
EFI_TCG2_PHYSICAL_PRESENCE parameters.
Implement in the null version.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 ++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c b/SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c
index 895d05a28d..0b57ba44e3 100644
--- a/SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c
+++ b/SecurityPkg/Library/Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c
@@ -1,7 +1,7 @@
/** @file
NULL Tcg2 PP Vendor library instance that does not support any vendor specific PPI.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -40,6 +40,37 @@ Tcg2PpVendorLibExecutePendingRequest (
return TCG_PP_OPERATION_RESPONSE_BIOS_FAILURE;
}
+/**
+ Check and execute the requested physical presence command.
+
+ This API should be invoked in BIOS boot phase to process pending request.
+
+ Caution: This function may receive untrusted input.
+
+ If OperationRequest < 128, then ASSERT().
+
+ @param[in] PlatformAuth platform auth value. NULL means no platform auth change.
+ @param[in] PPData Ptr to EFI_TCG2_PHYSICAL_PRESENCE data.
+ @param[in, out] ManagementFlags BIOS TPM Management Flags.
+ @param[out] ResetRequired If reset is required to vendor settings in effect.
+ True, it indicates the reset is required.
+ False, it indicates the reset is not required.
+
+ @return TPM Operation Response to OS Environment.
+**/
+UINT32
+EFIAPI
+Tcg2PpVendorLibExecutePendingRequestEx (
+ IN TPM2B_AUTH *PlatformAuth, OPTIONAL
+ IN EFI_TCG2_PHYSICAL_PRESENCE *PPData,
+ IN OUT UINT32 *ManagementFlags,
+ OUT BOOLEAN *ResetRequired
+ )
+{
+ ASSERT (PPData->PPRequest >= TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION);
+ return TCG_PP_OPERATION_RESPONSE_BIOS_FAILURE;
+}
+
/**
Check if there is a valid physical presence command request.
@@ -100,6 +131,34 @@ Tcg2PpVendorLibSubmitRequestToPreOSFunction (
return TCG_PP_SUBMIT_REQUEST_TO_PREOS_NOT_IMPLEMENTED;
}
+/**
+ The callback for TPM vendor specific physical presence which is called for
+ Submit TPM Operation Request to Pre-OS Environment and
+ Submit TPM Operation Request to Pre-OS Environment 2.
+
+ This API should be invoked in OS runtime phase to interface with ACPI method.
+
+ Caution: This function may receive untrusted input.
+
+ If OperationRequest < 128, then ASSERT().
+
+ @param[in] *PPData Ptr to EFI_TCG2_PHYSICAL_PRESENCE data.
+ @param[in] ManagementFlags BIOS TPM Management Flags.
+
+ @return Return Code for Submit TPM Operation Request to Pre-OS Environment and
+ Submit TPM Operation Request to Pre-OS Environment 2.
+**/
+UINT32
+EFIAPI
+Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx (
+ IN EFI_TCG2_PHYSICAL_PRESENCE *PPdata,
+ IN UINT32 ManagementFlags
+ )
+{
+ ASSERT (PPdata->PPRequest >= TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION);
+ return TCG_PP_SUBMIT_REQUEST_TO_PREOS_NOT_IMPLEMENTED;
+}
+
/**
The callback for TPM vendor specific physical presence which is called for
Get User Confirmation Status for Operation.
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 03/13] SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
2020-01-03 3:04 ` [PATCH 01/13] SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata Gao, Zhichao
2020-01-03 3:04 ` [PATCH 02/13] SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 04/13] SecurityPkg/SmmTcg2PhysicalPresenceLib: " Gao, Zhichao
` (10 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Change the call of Tcg2PpVendorLibExecutePendingRequest to
Tcg2PpVendorLibExecutePendingRequestEx.
Change the call of Tcg2PpVendorLibSubmitRequestToPreOSFunction to
Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c b/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
index 80e2e37bf4..081605f26c 100644
--- a/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
+++ b/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
@@ -7,7 +7,7 @@
Tpm2ExecutePendingTpmRequest() will receive untrusted input and do validation.
-Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -767,7 +767,7 @@ Tcg2ExecutePendingTpmRequest (
if (TcgPpData->PPRequest >= TCG2_PHYSICAL_PRESENCE_VENDOR_SPECIFIC_OPERATION) {
NewFlags = *Flags;
NewPPFlags = NewFlags.PPFlags;
- TcgPpData->PPResponse = Tcg2PpVendorLibExecutePendingRequest (PlatformAuth, TcgPpData->PPRequest, &NewPPFlags, &ResetRequired);
+ TcgPpData->PPResponse = Tcg2PpVendorLibExecutePendingRequestEx (PlatformAuth, TcgPpData, &NewPPFlags, &ResetRequired);
NewFlags.PPFlags = NewPPFlags;
} else {
if (!RequestConfirmed) {
@@ -1196,7 +1196,7 @@ Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunction (
if (EFI_ERROR (Status)) {
Flags.PPFlags = TCG2_BIOS_TPM_MANAGEMENT_FLAG_DEFAULT | TCG2_BIOS_STORAGE_MANAGEMENT_FLAG_DEFAULT;
}
- return Tcg2PpVendorLibSubmitRequestToPreOSFunction (OperationRequest, Flags.PPFlags, RequestParameter);
+ return Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx (&PpData, Flags.PPFlags);
}
return TCG_PP_SUBMIT_REQUEST_TO_PREOS_SUCCESS;
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 04/13] SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (2 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 03/13] SecurityPkg/Tcg2PhysicalPresenceLib: Use the " Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 05/13] SecurityPkg/dec: Add a pcd for user response wait time Gao, Zhichao
` (9 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Change the call of Tcg2PpVendorLibSubmitRequestToPreOSFunction to
Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
index 3827df9663..72b51ed5e9 100644
--- a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
+++ b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
@@ -10,7 +10,7 @@
Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunction() and Tcg2PhysicalPresenceLibGetUserConfirmationStatusFunction()
will receive untrusted input and do validation.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -164,7 +164,7 @@ Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx (
if (EFI_ERROR (Status)) {
Flags.PPFlags = TCG2_BIOS_TPM_MANAGEMENT_FLAG_DEFAULT | TCG2_BIOS_STORAGE_MANAGEMENT_FLAG_DEFAULT;
}
- ReturnCode = Tcg2PpVendorLibSubmitRequestToPreOSFunction (*OperationRequest, Flags.PPFlags, *RequestParameter);
+ ReturnCode = Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx (&PpData, Flags.PPFlags);
}
EXIT:
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 05/13] SecurityPkg/dec: Add a pcd for user response wait time
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (3 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 04/13] SecurityPkg/SmmTcg2PhysicalPresenceLib: " Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use " Gao, Zhichao
` (8 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Add a pcd PcdPhysicalPresenceUserConfirmTimeout to set the wait time
of the user response.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
SecurityPkg/SecurityPkg.dec | 7 ++++++-
SecurityPkg/SecurityPkg.uni | 7 ++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
index cac36caf0a..5711fde254 100644
--- a/SecurityPkg/SecurityPkg.dec
+++ b/SecurityPkg/SecurityPkg.dec
@@ -5,7 +5,7 @@
# It also provides the definitions(including PPIs/PROTOCOLs/GUIDs and library classes)
# and libraries instances, which are used for those features.
#
-# Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2009 - 2020, Intel Corporation. All rights reserved.<BR>
# (C) Copyright 2015 Hewlett Packard Enterprise Development LP <BR>
# Copyright (c) 2017, Microsoft Corporation. All rights reserved. <BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -287,6 +287,11 @@
# @Prompt Physical presence of the platform operator.
gEfiSecurityPkgTokenSpaceGuid.PcdTpmPhysicalPresence|TRUE|BOOLEAN|0x00010001
+ ## Maximum wait time in seconds for user response in physical presence.<BR><BR>
+ # If the value is set to 0 (default), it means waiting forever.
+ # @Prompt Timeout of user confirmation.
+ gEfiSecurityPkgTokenSpaceGuid.PcdPhysicalPresenceUserConfirmTimeout|0|UINT32|0x00010032
+
[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
## Indicates whether TPM physical presence is locked during platform initialization.
# Once it is locked, it can not be unlocked for TPM life time.<BR><BR>
diff --git a/SecurityPkg/SecurityPkg.uni b/SecurityPkg/SecurityPkg.uni
index 68587304d7..be6523b3fb 100644
--- a/SecurityPkg/SecurityPkg.uni
+++ b/SecurityPkg/SecurityPkg.uni
@@ -5,7 +5,7 @@
// It also provides the definitions(including PPIs/PROTOCOLs/GUIDs and library classes)
// and libraries instances, which are used for those features.
//
-// Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+// Copyright (c) 2009 - 2020, Intel Corporation. All rights reserved.<BR>
//
// SPDX-License-Identifier: BSD-2-Clause-Patent
//
@@ -84,6 +84,11 @@
"TRUE - The platform operator is physically present.<BR>\n"
"FALSE - The platform operator is not physically present.<BR>"
+#string STR_gEfiSecurityPkgTokenSpaceGuid_PcdPhysicalPresenceUserConfirmTimeout_PROMPT #language en-US "Timeout of user confirmation."
+
+#string STR_gEfiSecurityPkgTokenSpaceGuid_PcdPhysicalPresenceUserConfirmTimeout_HELP #language en-US "Maximum wait time in seconds for user response in physical presence.<BR><BR>\n"
+ "If the value is set to 0 (default), it means waiting forever."
+
#string STR_gEfiSecurityPkgTokenSpaceGuid_PcdPhysicalPresenceLifetimeLock_PROMPT #language en-US "Lock TPM physical presence asserting method."
#string STR_gEfiSecurityPkgTokenSpaceGuid_PcdPhysicalPresenceLifetimeLock_HELP #language en-US "Indicates whether TPM physical presence is locked during platform initialization. Once it is locked, it can not be unlocked for TPM life time.<BR><BR>\n"
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (4 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 05/13] SecurityPkg/dec: Add a pcd for user response wait time Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 14:21 ` [edk2-devel] " Laszlo Ersek
2020-01-03 3:04 ` [PATCH 07/13] SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp " Gao, Zhichao
` (7 subsequent siblings)
13 siblings, 1 reply; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel
Cc: Jiewen Yao, Jian J Wang, Chao Zhang, Jordan Justen, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Use the pcd PcdPhysicalPresenceUserConfirmTimeout to control the
wait time of user response.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../DxeTcg2PhysicalPresenceLib.c | 63 ++++++++++++++-----
.../DxeTcg2PhysicalPresenceLib.inf | 6 +-
2 files changed, 52 insertions(+), 17 deletions(-)
diff --git a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
index 00d76ba2c2..13f78cbfac 100644
--- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
+++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
@@ -9,7 +9,7 @@
Copyright (C) 2018, Red Hat, Inc.
Copyright (c) 2018, IBM Corporation. All rights reserved.<BR>
-Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -32,6 +32,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiLib.h>
#include <Library/UefiRuntimeServicesTableLib.h>
+#include <Library/TimerLib.h>
+#include <Library/PcdLib.h>
#include <Library/Tcg2PhysicalPresenceLib.h>
@@ -365,28 +367,57 @@ Tcg2ReadUserKey (
{
EFI_STATUS Status;
EFI_INPUT_KEY Key;
- UINT16 InputKey;
+ UINT16 ConfirmKey;
+ UINTN Interval;
+ INT64 Timeout;
- InputKey = 0;
+ //
+ // delay 100 milli-second
+ //
+ Interval = 100;
+ ConfirmKey = (CautionKey) ? SCAN_F12 : SCAN_F10;
+ Timeout = (INT64)PcdGet32 (PcdPhysicalPresenceUserConfirmTimeout);
+ if (Timeout > 0) {
+ Timeout = (INT64)MultU64x32 ((UINT64)Timeout, 1000);
+ } else {
+ //
+ // Wait forever
+ //
+ Timeout = MAX_INT64;
+ }
+
+ //
+ // Wait for user response within the time-out
+ //
do {
+ MicroSecondDelay (Interval * 1000);
+
Status = gBS->CheckEvent (gST->ConIn->WaitForKey);
if (!EFI_ERROR (Status)) {
Status = gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
- if (Key.ScanCode == SCAN_ESC) {
- InputKey = Key.ScanCode;
- }
- if ((Key.ScanCode == SCAN_F10) && !CautionKey) {
- InputKey = Key.ScanCode;
- }
- if ((Key.ScanCode == SCAN_F12) && CautionKey) {
- InputKey = Key.ScanCode;
+ if (!EFI_ERROR (Status)) {
+ if (Key.ScanCode == ConfirmKey) {
+ //
+ // User Confirmation
+ //
+ return TRUE;
+ }
+
+ if (Key.ScanCode == SCAN_ESC) {
+ //
+ // User Rejection
+ //
+ return FALSE;
+ }
+ } else if (Status == EFI_DEVICE_ERROR) {
+ //
+ // If error, assume User Rejection
+ //
+ return FALSE;
}
}
- } while (InputKey == 0);
-
- if (InputKey != SCAN_ESC) {
- return TRUE;
- }
+ Timeout -= Interval;
+ } while (Timeout > 0);
return FALSE;
}
diff --git a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
index 85ce0e2b29..701de1836c 100644
--- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
+++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
@@ -20,7 +20,7 @@
# This external input must be validated carefully to avoid security issue.
#
# Copyright (C) 2018, Red Hat, Inc.
-# Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -62,10 +62,14 @@
UefiBootServicesTableLib
UefiLib
UefiRuntimeServicesTableLib
+ TimerLib
[Protocols]
gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
+[Pcd]
+ gEfiSecurityPkgTokenSpaceGuid.PcdPhysicalPresenceUserConfirmTimeout
+
[Guids]
## SOMETIMES_CONSUMES ## HII
gEfiTcg2PhysicalPresenceGuid
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 07/13] SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (5 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use " Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 08/13] SecurityPkg/TcgPyhsicalPresenceLib: " Gao, Zhichao
` (6 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Use the pcd PcdPhysicalPresenceUserConfirmTimeout to control the
wait time of user response.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../DxeTcg2PhysicalPresenceLib.c | 61 ++++++++++++++-----
.../DxeTcg2PhysicalPresenceLib.inf | 4 +-
2 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c b/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
index 081605f26c..1ae19436c2 100644
--- a/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
+++ b/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
@@ -31,6 +31,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/Tpm2CommandLib.h>
#include <Library/Tcg2PhysicalPresenceLib.h>
#include <Library/Tcg2PpVendorLib.h>
+#include <Library/TimerLib.h>
+#include <Library/PcdLib.h>
#define CONFIRM_BUFFER_SIZE 4096
@@ -267,28 +269,57 @@ Tcg2ReadUserKey (
{
EFI_STATUS Status;
EFI_INPUT_KEY Key;
- UINT16 InputKey;
+ UINT16 ConfirmKey;
+ UINTN Interval;
+ INT64 Timeout;
- InputKey = 0;
+ //
+ // delay 100 milli-second
+ //
+ Interval = 100;
+ ConfirmKey = (CautionKey) ? SCAN_F12 : SCAN_F10;
+ Timeout = (INT64)PcdGet32 (PcdPhysicalPresenceUserConfirmTimeout);
+ if (Timeout > 0) {
+ Timeout = (INT64)MultU64x32 ((UINT64)Timeout, 1000);
+ } else {
+ //
+ // Wait forever
+ //
+ Timeout = MAX_INT64;
+ }
+
+ //
+ // Wait for user response within the time-out
+ //
do {
+ MicroSecondDelay (Interval * 1000);
+
Status = gBS->CheckEvent (gST->ConIn->WaitForKey);
if (!EFI_ERROR (Status)) {
Status = gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
- if (Key.ScanCode == SCAN_ESC) {
- InputKey = Key.ScanCode;
- }
- if ((Key.ScanCode == SCAN_F10) && !CautionKey) {
- InputKey = Key.ScanCode;
- }
- if ((Key.ScanCode == SCAN_F12) && CautionKey) {
- InputKey = Key.ScanCode;
+ if (!EFI_ERROR (Status)) {
+ if (Key.ScanCode == ConfirmKey) {
+ //
+ // User Confirmation
+ //
+ return TRUE;
+ }
+
+ if (Key.ScanCode == SCAN_ESC) {
+ //
+ // User Rejection
+ //
+ return FALSE;
+ }
+ } else if (Status == EFI_DEVICE_ERROR) {
+ //
+ // If error, assume User Rejection
+ //
+ return FALSE;
}
}
- } while (InputKey == 0);
-
- if (InputKey != SCAN_ESC) {
- return TRUE;
- }
+ Timeout -= Interval;
+ } while (Timeout > 0);
return FALSE;
}
diff --git a/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf b/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
index e1c7c20d52..afca48356e 100644
--- a/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
+++ b/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.inf
@@ -8,7 +8,7 @@
# This driver will have external input - variable.
# This external input must be validated carefully to avoid security issue.
#
-# Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -50,6 +50,7 @@
HobLib
Tpm2CommandLib
Tcg2PpVendorLib
+ TimerLib
[Protocols]
gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
@@ -57,6 +58,7 @@
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTcg2PhysicalPresenceFlags ## SOMETIMES_CONSUMES
+ gEfiSecurityPkgTokenSpaceGuid.PcdPhysicalPresenceUserConfirmTimeout
[Guids]
## SOMETIMES_CONSUMES ## HII
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 08/13] SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (6 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 07/13] SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp " Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 09/13] SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata Gao, Zhichao
` (5 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Use the pcd PcdPhysicalPresenceUserConfirmTimeout to control the
wait time of user response.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../DxeTcgPhysicalPresenceLib.c | 76 ++++++++++++-------
.../DxeTcgPhysicalPresenceLib.inf | 6 +-
2 files changed, 55 insertions(+), 27 deletions(-)
diff --git a/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c b/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
index 174172d5d7..14423991f0 100644
--- a/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
+++ b/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
@@ -8,7 +8,7 @@
ExecutePendingTpmRequest() will receive untrusted input and do validation.
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -29,6 +29,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/EventGroup.h>
#include <Guid/PhysicalPresenceData.h>
#include <Library/TcgPpVendorLib.h>
+#include <Library/TimerLib.h>
+#include <Library/PcdLib.h>
#define CONFIRM_BUFFER_SIZE 4096
@@ -444,35 +446,57 @@ ReadUserKey (
{
EFI_STATUS Status;
EFI_INPUT_KEY Key;
- UINT16 InputKey;
- UINTN Index;
+ UINT16 ConfirmKey;
+ UINTN Interval;
+ INT64 Timeout;
- InputKey = 0;
- do {
- Status = gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
- if (Status == EFI_NOT_READY) {
- gBS->WaitForEvent (1, &gST->ConIn->WaitForKey, &Index);
- continue;
- }
+ //
+ // delay 100 milli-second
+ //
+ Interval = 100;
+ ConfirmKey = (CautionKey) ? SCAN_F12 : SCAN_F10;
+ Timeout = (INT64)PcdGet32 (PcdPhysicalPresenceUserConfirmTimeout);
+ if (Timeout > 0) {
+ Timeout = (INT64)MultU64x32 ((UINT64)Timeout, 1000);
+ } else {
+ //
+ // Wait forever
+ //
+ Timeout = MAX_INT64;
+ }
- if (Status == EFI_DEVICE_ERROR) {
- return FALSE;
- }
+ //
+ // Wait for user response within the time-out
+ //
+ do {
+ MicroSecondDelay (Interval * 1000);
+
+ Status = gBS->CheckEvent (gST->ConIn->WaitForKey);
+ if (!EFI_ERROR (Status)) {
+ Status = gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
+ if (!EFI_ERROR (Status)) {
+ if (Key.ScanCode == ConfirmKey) {
+ //
+ // User Confirmation
+ //
+ return TRUE;
+ }
- if (Key.ScanCode == SCAN_ESC) {
- InputKey = Key.ScanCode;
- }
- if ((Key.ScanCode == SCAN_F10) && !CautionKey) {
- InputKey = Key.ScanCode;
- }
- if ((Key.ScanCode == SCAN_F12) && CautionKey) {
- InputKey = Key.ScanCode;
+ if (Key.ScanCode == SCAN_ESC) {
+ //
+ // User Rejection
+ //
+ return FALSE;
+ }
+ } else if (Status == EFI_DEVICE_ERROR) {
+ //
+ // If error, assume User Rejection
+ //
+ return FALSE;
+ }
}
- } while (InputKey == 0);
-
- if (InputKey != SCAN_ESC) {
- return TRUE;
- }
+ Timeout -= Interval;
+ } while (Timeout > 0);
return FALSE;
}
diff --git a/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.inf b/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.inf
index cfe14f20ca..13b7246290 100644
--- a/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.inf
+++ b/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.inf
@@ -9,7 +9,7 @@
# This driver will have external input - variable.
# This external input must be validated carefully to avoid security issue.
#
-# Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2009 - 2020, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -50,11 +50,15 @@
PrintLib
HiiLib
TcgPpVendorLib
+ TimerLib
[Protocols]
gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
gEdkiiVariableLockProtocolGuid ## SOMETIMES_CONSUMES
+[Pcd]
+ gEfiSecurityPkgTokenSpaceGuid.PcdPhysicalPresenceUserConfirmTimeout
+
[Guids]
## SOMETIMES_CONSUMES ## HII
## SOMETIMES_PRODUCES ## Variable:L"PhysicalPresence"
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 09/13] SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (7 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 08/13] SecurityPkg/TcgPyhsicalPresenceLib: " Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 10/13] SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func Gao, Zhichao
` (4 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Add PPFunction to extend the EFI_TCG2_PHYSICAL_PRESENCE. It
refers to Physical Presence Function Index.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
SecurityPkg/Include/Guid/Tcg2PhysicalPresenceData.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/SecurityPkg/Include/Guid/Tcg2PhysicalPresenceData.h b/SecurityPkg/Include/Guid/Tcg2PhysicalPresenceData.h
index 382b487649..b962e81788 100644
--- a/SecurityPkg/Include/Guid/Tcg2PhysicalPresenceData.h
+++ b/SecurityPkg/Include/Guid/Tcg2PhysicalPresenceData.h
@@ -4,7 +4,7 @@
cleared after it is processed in the next boot cycle. The TPM2 response
is saved to variable.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved. <BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved. <BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -20,6 +20,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define TCG2_PHYSICAL_PRESENCE_VARIABLE L"Tcg2PhysicalPresence"
typedef struct {
+ UINT8 PPFunction; ///< Physical Presence Function Index
UINT8 PPRequest; ///< Physical Presence request command.
UINT32 PPRequestParameter; ///< Physical Presence request Parameter.
UINT8 LastPPRequest;
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 10/13] SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (8 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 09/13] SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 11/13] SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder Gao, Zhichao
` (3 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Extend the Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
to handle the FunctionIndex.
And modify the all of this function to avoid build error.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
SecurityPkg/Include/Library/Tcg2PhysicalPresenceLib.h | 4 +++-
.../DxeTcg2PhysicalPresenceLib.c | 1 +
.../SmmTcg2PhysicalPresenceLib.c | 11 ++++++++++-
SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +++++++---
4 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/SecurityPkg/Include/Library/Tcg2PhysicalPresenceLib.h b/SecurityPkg/Include/Library/Tcg2PhysicalPresenceLib.h
index 39febcb655..8b71c32d53 100644
--- a/SecurityPkg/Include/Library/Tcg2PhysicalPresenceLib.h
+++ b/SecurityPkg/Include/Library/Tcg2PhysicalPresenceLib.h
@@ -2,7 +2,7 @@
This library is intended to be used by BDS modules.
This library will execute TPM2 request.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -127,6 +127,7 @@ Tcg2PhysicalPresenceLibReturnOperationResponseToOsFunction (
Caution: This function may receive untrusted input.
+ @param[in, out] Pointer to FunctionIndex TPM physical presence Function Index.
@param[in, out] Pointer to OperationRequest TPM physical presence operation request.
@param[in, out] Pointer to RequestParameter TPM physical presence operation request parameter.
@@ -135,6 +136,7 @@ Tcg2PhysicalPresenceLibReturnOperationResponseToOsFunction (
**/
UINT32
Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx (
+ IN OUT UINT32 *FunctionIndex,
IN OUT UINT32 *OperationRequest,
IN OUT UINT32 *RequestParameter
);
diff --git a/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c b/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
index 1ae19436c2..090ed2781e 100644
--- a/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
+++ b/SecurityPkg/Library/DxeTcg2PhysicalPresenceLib/DxeTcg2PhysicalPresenceLib.c
@@ -1199,6 +1199,7 @@ Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunction (
if ((PpData.PPRequest != OperationRequest) ||
(PpData.PPRequestParameter != RequestParameter)) {
+ PpData.PPFunction = (UINT8)TCG_ACPI_FUNCTION_SUBMIT_REQUEST_TO_BIOS_2;
PpData.PPRequest = (UINT8)OperationRequest;
PpData.PPRequestParameter = RequestParameter;
DataSize = sizeof (EFI_TCG2_PHYSICAL_PRESENCE);
diff --git a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
index 72b51ed5e9..271ee51060 100644
--- a/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
+++ b/SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib.c
@@ -89,6 +89,7 @@ Tcg2PhysicalPresenceLibReturnOperationResponseToOsFunction (
Caution: This function may receive untrusted input.
+ @param[in, out] Pointer to FunctionIndex TPM physical presence Function Index.
@param[in, out] Pointer to OperationRequest TPM physical presence operation request.
@param[in, out] Pointer to RequestParameter TPM physical presence operation request parameter.
@@ -97,6 +98,7 @@ Tcg2PhysicalPresenceLibReturnOperationResponseToOsFunction (
**/
UINT32
Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx (
+ IN OUT UINT32 *FunctionIndex,
IN OUT UINT32 *OperationRequest,
IN OUT UINT32 *RequestParameter
)
@@ -135,6 +137,7 @@ Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx (
if ((PpData.PPRequest != *OperationRequest) ||
(PpData.PPRequestParameter != *RequestParameter)) {
+ PpData.PPFunction = (UINT8)*FunctionIndex;
PpData.PPRequest = (UINT8)*OperationRequest;
PpData.PPRequestParameter = *RequestParameter;
DataSize = sizeof (EFI_TCG2_PHYSICAL_PRESENCE);
@@ -211,13 +214,19 @@ Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunction (
IN UINT32 RequestParameter
)
{
+ UINT32 TempFunctionIndex;
UINT32 TempOperationRequest;
UINT32 TempRequestParameter;
+ TempFunctionIndex = TCG_ACPI_FUNCTION_SUBMIT_REQUEST_TO_BIOS_2;
TempOperationRequest = OperationRequest;
TempRequestParameter = RequestParameter;
- return Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx(&TempOperationRequest, &TempRequestParameter);
+ return Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx (
+ &TempFunctionIndex,
+ &TempOperationRequest,
+ &TempRequestParameter
+ );
}
/**
diff --git a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c b/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
index 91aebb62b8..d9e8be1403 100644
--- a/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
+++ b/SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c
@@ -9,7 +9,7 @@
PhysicalPresenceCallback() and MemoryClearCallback() will receive untrusted input and do some check.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -80,6 +80,7 @@ PhysicalPresenceCallback (
{
UINT32 MostRecentRequest;
UINT32 Response;
+ UINT32 FunctionIndex;
UINT32 OperationRequest;
UINT32 RequestParameter;
@@ -95,12 +96,15 @@ PhysicalPresenceCallback (
} else if ((mTcgNvs->PhysicalPresence.Parameter == TCG_ACPI_FUNCTION_SUBMIT_REQUEST_TO_BIOS)
|| (mTcgNvs->PhysicalPresence.Parameter == TCG_ACPI_FUNCTION_SUBMIT_REQUEST_TO_BIOS_2)) {
+ FunctionIndex = mTcgNvs->PhysicalPresence.Parameter;
OperationRequest = mTcgNvs->PhysicalPresence.Request;
RequestParameter = mTcgNvs->PhysicalPresence.RequestParameter;
mTcgNvs->PhysicalPresence.ReturnCode = Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx (
- &OperationRequest,
- &RequestParameter
+ &FunctionIndex, ///< Arg2, Function Index (2 or 7)
+ &OperationRequest, ///< Arg3, Integer1 (Operation value, valid for both Function Index 2 and 7)
+ &RequestParameter ///< Arg3, Integer2 (Operation Parameter, valid for Function Index 7 only)
);
+ mTcgNvs->PhysicalPresence.Parameter = FunctionIndex;
mTcgNvs->PhysicalPresence.Request = OperationRequest;
mTcgNvs->PhysicalPresence.RequestParameter = RequestParameter;
} else if (mTcgNvs->PhysicalPresence.Parameter == TCG_ACPI_FUNCTION_GET_USER_CONFIRMATION_STATUS_FOR_REQUEST) {
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 11/13] SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (9 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 10/13] SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 12/13] SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code Gao, Zhichao
` (2 subsequent siblings)
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Tcg2ConfigNvData.h is useful for the tcg2 driver. So move it
to the Include folder for other platform tcg2 instances to use.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 ++--
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +--
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 ++--
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +--
SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 ++--
SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 ++--
7 files changed, 11 insertions(+), 13 deletions(-)
rename SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigNvData.h b/SecurityPkg/Include/Tcg2ConfigNvData.h
similarity index 94%
rename from SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigNvData.h
rename to SecurityPkg/Include/Tcg2ConfigNvData.h
index a91c052850..9c7850e865 100644
--- a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigNvData.h
+++ b/SecurityPkg/Include/Tcg2ConfigNvData.h
@@ -1,7 +1,7 @@
/** @file
Header file for NV data structure definition.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr b/SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr
index 91a463997c..10a447239e 100644
--- a/SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr
+++ b/SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr
@@ -1,12 +1,12 @@
/** @file
VFR file used by the TCG2 configuration component.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
-#include "Tcg2ConfigNvData.h"
+#include <Tcg2ConfigNvData.h>
formset
guid = TCG2_CONFIG_FORM_SET_GUID,
diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
index 6417ab82a1..8e12c2ebc8 100644
--- a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
+++ b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf
@@ -4,7 +4,7 @@
# By this module, user may select TPM device, clear TPM state, etc.
# NOTE: This module is only for reference only, each platform should have its own setup page.
#
-# Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -29,7 +29,6 @@
Tcg2ConfigImpl.h
Tcg2Config.vfr
Tcg2ConfigStrings.uni
- Tcg2ConfigNvData.h
Tcg2Internal.h
[Packages]
diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h
index af542d52ef..860379605a 100644
--- a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h
+++ b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h
@@ -2,7 +2,7 @@
The header file of HII Config Access protocol implementation of TCG2
configuration module.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -35,7 +35,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Guid/MdeModuleHii.h>
-#include "Tcg2ConfigNvData.h"
+#include <Tcg2ConfigNvData.h>
#include "Tcg2Internal.h"
#define TCG2_PROTOCOL_VERSION_DEFAULT 0x0001
diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index f2aa3234ad..416ad4610b 100644
--- a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -4,7 +4,7 @@
# This module initializes TPM device type based on variable and detection.
# NOTE: This module is only for reference only, each platform should have its own setup page.
#
-# Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
##
@@ -27,7 +27,6 @@
[Sources]
Tcg2ConfigPeim.c
- Tcg2ConfigNvData.h
Tcg2Internal.h
TpmDetection.c
diff --git a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
index e0d0a207e1..5b55c8b406 100644
--- a/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
+++ b/SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c
@@ -1,7 +1,7 @@
/** @file
The module entry point for Tcg2 configuration module.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -22,7 +22,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Ppi/TpmInitialized.h>
#include <Protocol/Tcg2Protocol.h>
-#include "Tcg2ConfigNvData.h"
+#include <Tcg2ConfigNvData.h>
#include "Tcg2Internal.h"
TPM_INSTANCE_ID mTpmInstanceId[] = TPM_INSTANCE_ID_LIST;
diff --git a/SecurityPkg/Tcg/Tcg2Config/TpmDetection.c b/SecurityPkg/Tcg/Tcg2Config/TpmDetection.c
index eeaadc5e2f..67f76853e2 100644
--- a/SecurityPkg/Tcg/Tcg2Config/TpmDetection.c
+++ b/SecurityPkg/Tcg/Tcg2Config/TpmDetection.c
@@ -1,7 +1,7 @@
/** @file
TPM1.2/dTPM2.0 auto detection.
-Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -19,7 +19,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/Tpm12CommandLib.h>
#include <IndustryStandard/Tpm12.h>
-#include "Tcg2ConfigNvData.h"
+#include <Tcg2ConfigNvData.h>
#include "Tcg2Internal.h"
/**
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 12/13] SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (10 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 11/13] SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:04 ` [PATCH 13/13] SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11 Gao, Zhichao
2020-01-03 3:09 ` [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Yao, Jiewen
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Replace the ASSERT with the error code return in the TpmPhysicalPresence
and GetTpmCapability.
Add missing error checking after call TpmPhysicalPresence in
TcgPhysicalPresenceLibProcessRequest.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../DxeTcgPhysicalPresenceLib.c | 23 +++++++++++++++----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c b/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
index 14423991f0..7fb2ef9735 100644
--- a/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
+++ b/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
@@ -104,9 +104,13 @@ GetTpmCapability (
sizeof (RecvBuffer),
(UINT8*)&RecvBuffer
);
- ASSERT_EFI_ERROR (Status);
- ASSERT (TpmRsp->tag == SwapBytes16 (TPM_TAG_RSP_COMMAND));
- ASSERT (TpmRsp->returnCode == 0);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if ((TpmRsp->tag != SwapBytes16 (TPM_TAG_RSP_COMMAND)) || (TpmRsp->returnCode != 0)) {
+ return EFI_DEVICE_ERROR;
+ }
TpmPermanentFlags = (TPM_PERMANENT_FLAGS *)&RecvBuffer[sizeof (TPM_RSP_COMMAND_HDR) + sizeof (UINT32)];
@@ -159,8 +163,14 @@ TpmPhysicalPresence (
sizeof (TpmRsp),
(UINT8*)&TpmRsp
);
- ASSERT_EFI_ERROR (Status);
- ASSERT (TpmRsp.tag == SwapBytes16 (TPM_TAG_RSP_COMMAND));
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (TpmRsp.tag != SwapBytes16 (TPM_TAG_RSP_COMMAND)) {
+ return EFI_DEVICE_ERROR;
+ }
+
if (TpmRsp.returnCode != 0) {
//
// If it fails, some requirements may be needed for this command.
@@ -1298,6 +1308,9 @@ TcgPhysicalPresenceLibProcessRequest (
// Set operator physical presence flags
//
TpmPhysicalPresence (TcgProtocol, TPM_PHYSICAL_PRESENCE_PRESENT);
+ if (EFI_ERROR (Status)) {
+ return;
+ }
//
// Execute pending TPM request.
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 13/13] SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (11 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 12/13] SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code Gao, Zhichao
@ 2020-01-03 3:04 ` Gao, Zhichao
2020-01-03 3:09 ` [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Yao, Jiewen
13 siblings, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 3:04 UTC (permalink / raw)
To: devel; +Cc: Jiewen Yao, Jian J Wang, Chao Zhang
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
Refer to Physical Presence Interface Spec Page 37, the firmware
may perform the PLATFORM RESET imediately after the
TPM_PhysicalSetDeactivated = TRUE but that requires tracking
the next command.
Change the operation to match the spec.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
.../DxeTcgPhysicalPresenceLib.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c b/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
index 7fb2ef9735..de339c9703 100644
--- a/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
+++ b/SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.c
@@ -346,9 +346,16 @@ ExecutePhysicalPresence (
return TpmResponse;
case PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE:
- TpmResponse = ExecutePhysicalPresence (TcgProtocol, PHYSICAL_PRESENCE_SET_OWNER_INSTALL_FALSE, PpiFlags);
- if (TpmResponse == 0) {
+ //
+ // PHYSICAL_PRESENCE_SET_OWNER_INSTALL_FALSE + PHYSICAL_PRESENCE_DEACTIVATE_DISABLE
+ // PHYSICAL_PRESENCE_DEACTIVATE_DISABLE will be executed after reboot
+ //
+ if ((PpiFlags->PPFlags & TCG_VENDOR_LIB_FLAG_RESET_TRACK) == 0) {
+ TpmResponse = ExecutePhysicalPresence (TcgProtocol, PHYSICAL_PRESENCE_SET_OWNER_INSTALL_FALSE, PpiFlags);
+ PpiFlags->PPFlags |= TCG_VENDOR_LIB_FLAG_RESET_TRACK;
+ } else {
TpmResponse = ExecutePhysicalPresence (TcgProtocol, PHYSICAL_PRESENCE_DEACTIVATE_DISABLE, PpiFlags);
+ PpiFlags->PPFlags &= ~TCG_VENDOR_LIB_FLAG_RESET_TRACK;
}
return TpmResponse;
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
` (12 preceding siblings ...)
2020-01-03 3:04 ` [PATCH 13/13] SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11 Gao, Zhichao
@ 2020-01-03 3:09 ` Yao, Jiewen
2020-01-03 5:07 ` Gao, Zhichao
13 siblings, 1 reply; 26+ messages in thread
From: Yao, Jiewen @ 2020-01-03 3:09 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Wang, Jian J, Zhang, Chao B, Justen, Jordan L, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
Hi
I am not clear on the purpose of this extension.
The Bugzilla just describes the solution.
But what is the problem you are trying to resolve?
I completely don’t understand.
Please do consider add the background information there.
Or it is hard for me to comment.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Friday, January 3, 2020 11:04 AM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Zhang, Chao B <chao.b.zhang@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
>
> 1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
> Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to Tcg2PpVendorLib. It has
> one
> more parameter PPData (type EFI_TCG2_PHYSICAL_PRESENCE) to transfer more
> data.
> 2. Use the Ex version instead of original one in Tcg2PhysicalPresenceLib
> 3. Add a pcd PcdPhysicalPresenceUserConfirmTimeout to control the user
> confirm
> input key timeout.
> 4. Add FunctionIndex to structure type EFI_TCG2_PHYSICAL_PRESENCE to
> transfer
> mTcgNvs->PhysicalPresence.Parameter data.
> 5. Add parameter FunctionIndex to
> Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
> to initialize the PPdata.
> 6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
> SecurityPkg/Include.
> It is useful for platform code to implement their own Tcg2PhysicalPresenceLib.
> 7. Replace the ASSERT with error code return in TpmPhysicalPresenceLib
> 8. Fix one operation
> (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
> TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page 37).
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> Zhichao Gao (13):
> SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
> SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function
> SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
> SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
> SecurityPkg/dec: Add a pcd for user response wait time
> OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
> SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
> SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
> SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata
> SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
> SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
> SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code
> SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
>
> .../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
> .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> .../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
> .../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
> SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
> .../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
> .../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
> .../DxeTcg2PhysicalPresenceLib.inf | 4 +-
> .../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
> .../DxeTcgPhysicalPresenceLib.inf | 6 +-
> .../SmmTcg2PhysicalPresenceLib.c | 15 ++-
> .../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
> SecurityPkg/SecurityPkg.dec | 7 +-
> SecurityPkg/SecurityPkg.uni | 7 +-
> SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
> SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
> SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
> SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
> SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
> SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
> SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
> 21 files changed, 347 insertions(+), 95 deletions(-)
> rename SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
>
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
2020-01-03 3:09 ` [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Yao, Jiewen
@ 2020-01-03 5:07 ` Gao, Zhichao
2020-01-03 5:30 ` Yao, Jiewen
[not found] ` <15E649625DE7E06B.2038@groups.io>
0 siblings, 2 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-03 5:07 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io
Cc: Wang, Jian J, Zhang, Chao B, Justen, Jordan L, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
See below.
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Friday, January 3, 2020 11:09 AM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> Interface
>
> Hi
> I am not clear on the purpose of this extension.
>
> The Bugzilla just describes the solution.
> But what is the problem you are trying to resolve?
>
> I completely don’t understand.
>
> Please do consider add the background information there.
> Or it is hard for me to comment.
>
>
> Thank you
> Yao Jiewen
>
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Friday, January 3, 2020 11:04 AM
> > To: devel@edk2.groups.io
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > <stefanb@linux.ibm.com>
> > Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> > Interface
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> >
> > 1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
> > Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to Tcg2PpVendorLib. It
> > has one more parameter PPData (type EFI_TCG2_PHYSICAL_PRESENCE) to
> > transfer more data.
> > 2. Use the Ex version instead of original one in
> > Tcg2PhysicalPresenceLib 3. Add a pcd
> > PcdPhysicalPresenceUserConfirmTimeout to control the user confirm
> > input key timeout.
> > 4. Add FunctionIndex to structure type EFI_TCG2_PHYSICAL_PRESENCE to
> > transfer
> > mTcgNvs->PhysicalPresence.Parameter data.
> > 5. Add parameter FunctionIndex to
> > Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
> > to initialize the PPdata.
Background:
Some platforms implement their own Tcg2PpVendorLib and want to operate with more parameters.
#1-#2 and #4-#5 changes aim to add extend the interface with PPdata. PPdata pointer can transfer most of the required parameter to do the operation.
All the extend changes is for the PPdata and transfer to the platform implemented interfaces. This would affect the platforms which implement their own Tcg2PpVendorLib.
But for open source platform, I didn't see any implementation of it.
#3 aims to add a pcd to let the customers to decide whether to wait for the input forever or with a timeout count.
> > 6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
> > SecurityPkg/Include.
> > It is useful for platform code to implement their own Tcg2PhysicalPresenceLib.
#6 is a movement to decrease the duplicated code at platform side if the platform code implement its own TCG library or driver.
> > 7. Replace the ASSERT with error code return in TpmPhysicalPresenceLib
#7 aims to remove the ASSERT because it is not critical. ASSERT when fail to call TpmPhysicalPresence and GetTpmCapability is not a good behavior.
> > 8. Fix one operation
> > (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
> > TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page 37).
#8 is a bug fix to follow the spec.
Thanks,
Zhichao
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> Zhichao Gao (13):
> > SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
> > SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function
> > SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
> > SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
> > SecurityPkg/dec: Add a pcd for user response wait time
> > OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
> > SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
> > SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
> > SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata
> > SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
> > SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
> > SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code
> > SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
> >
> > .../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
> > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > .../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
> > .../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
> > SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
> > .../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
> > .../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
> > .../DxeTcg2PhysicalPresenceLib.inf | 4 +-
> > .../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
> > .../DxeTcgPhysicalPresenceLib.inf | 6 +-
> > .../SmmTcg2PhysicalPresenceLib.c | 15 ++-
> > .../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
> > SecurityPkg/SecurityPkg.dec | 7 +-
> > SecurityPkg/SecurityPkg.uni | 7 +-
> > SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
> > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
> > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
> > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
> > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
> > SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
> > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
> > 21 files changed, 347 insertions(+), 95 deletions(-) rename
> > SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
> >
> > --
> > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
2020-01-03 5:07 ` Gao, Zhichao
@ 2020-01-03 5:30 ` Yao, Jiewen
2020-01-09 9:05 ` Gao, Zhichao
[not found] ` <15E649625DE7E06B.2038@groups.io>
1 sibling, 1 reply; 26+ messages in thread
From: Yao, Jiewen @ 2020-01-03 5:30 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Wang, Jian J, Zhang, Chao B, Justen, Jordan L, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
What is the platform use case?
Please give at least some examples.
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Friday, January 3, 2020 1:08 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> <stefanb@linux.ibm.com>
> Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> Interface
>
> See below.
>
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Friday, January 3, 2020 11:09 AM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> > Interface
> >
> > Hi
> > I am not clear on the purpose of this extension.
> >
> > The Bugzilla just describes the solution.
> > But what is the problem you are trying to resolve?
> >
> > I completely don’t understand.
> >
> > Please do consider add the background information there.
> > Or it is hard for me to comment.
> >
> >
> > Thank you
> > Yao Jiewen
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Friday, January 3, 2020 11:04 AM
> > > To: devel@edk2.groups.io
> > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > > Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > > <stefanb@linux.ibm.com>
> > > Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> > > Interface
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> > >
> > > 1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
> > > Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to Tcg2PpVendorLib. It
> > > has one more parameter PPData (type EFI_TCG2_PHYSICAL_PRESENCE) to
> > > transfer more data.
> > > 2. Use the Ex version instead of original one in
> > > Tcg2PhysicalPresenceLib 3. Add a pcd
> > > PcdPhysicalPresenceUserConfirmTimeout to control the user confirm
> > > input key timeout.
> > > 4. Add FunctionIndex to structure type EFI_TCG2_PHYSICAL_PRESENCE to
> > > transfer
> > > mTcgNvs->PhysicalPresence.Parameter data.
> > > 5. Add parameter FunctionIndex to
> > > Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
> > > to initialize the PPdata.
>
> Background:
> Some platforms implement their own Tcg2PpVendorLib and want to operate
> with more parameters.
>
> #1-#2 and #4-#5 changes aim to add extend the interface with PPdata. PPdata
> pointer can transfer most of the required parameter to do the operation.
> All the extend changes is for the PPdata and transfer to the platform
> implemented interfaces. This would affect the platforms which implement their
> own Tcg2PpVendorLib.
> But for open source platform, I didn't see any implementation of it.
>
> #3 aims to add a pcd to let the customers to decide whether to wait for the
> input forever or with a timeout count.
>
> > > 6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
> > > SecurityPkg/Include.
> > > It is useful for platform code to implement their own
> Tcg2PhysicalPresenceLib.
>
> #6 is a movement to decrease the duplicated code at platform side if the
> platform code implement its own TCG library or driver.
>
> > > 7. Replace the ASSERT with error code return in TpmPhysicalPresenceLib
>
> #7 aims to remove the ASSERT because it is not critical. ASSERT when fail to call
> TpmPhysicalPresence and GetTpmCapability is not a good behavior.
>
> > > 8. Fix one operation
> > > (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
> > > TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page 37).
>
> #8 is a bug fix to follow the spec.
>
> Thanks,
> Zhichao
>
> > >
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> Zhichao Gao (13):
> > > SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
> > > SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function
> > > SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
> > > SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
> > > SecurityPkg/dec: Add a pcd for user response wait time
> > > OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
> > > SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
> > > SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
> > > SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata
> > > SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
> > > SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
> > > SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code
> > > SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
> > >
> > > .../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
> > > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > > .../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
> > > .../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
> > > SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
> > > .../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
> > > .../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
> > > .../DxeTcg2PhysicalPresenceLib.inf | 4 +-
> > > .../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
> > > .../DxeTcgPhysicalPresenceLib.inf | 6 +-
> > > .../SmmTcg2PhysicalPresenceLib.c | 15 ++-
> > > .../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
> > > SecurityPkg/SecurityPkg.dec | 7 +-
> > > SecurityPkg/SecurityPkg.uni | 7 +-
> > > SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
> > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
> > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
> > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
> > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
> > > SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
> > > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
> > > 21 files changed, 347 insertions(+), 95 deletions(-) rename
> > > SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
> > >
> > > --
> > > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
[not found] ` <15E649625DE7E06B.2038@groups.io>
@ 2020-01-03 5:59 ` Yao, Jiewen
2020-01-07 8:05 ` Gao, Zhichao
0 siblings, 1 reply; 26+ messages in thread
From: Yao, Jiewen @ 2020-01-03 5:59 UTC (permalink / raw)
To: devel@edk2.groups.io, Yao, Jiewen, Gao, Zhichao
Cc: Wang, Jian J, Zhang, Chao B, Justen, Jordan L, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
Some other comment:
6. I disagree to move NvData.h to securityPkg.
This is designed to be an internal variable.
Why it need to move to security pkg to share with other driver?
I agree that duplicating code is bad solution.
And exposing internal data structure is as bad as duplicating code.
By design, the platform can have its own TCG platform lib and its own NvData.h.
Would you please help me understand why this is needed?
7. Removing assert has no relationship with this extend PPI interface.
If you want, please file another Bugzilla and handle it separately.
8. Fixing bug to follow spec is good.
But I do not see the relationship with the PPI interface extension.
Please file another Bugzilla and handle that separately.
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
> Sent: Friday, January 3, 2020 1:30 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> <stefanb@linux.ibm.com>
> Subject: Re: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> Presence Interface
>
> What is the platform use case?
> Please give at least some examples.
>
>
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Friday, January 3, 2020 1:08 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>;
> > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > <stefanb@linux.ibm.com>
> > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> > Interface
> >
> > See below.
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen
> > > Sent: Friday, January 3, 2020 11:09 AM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > > Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > > <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> > > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> > > Interface
> > >
> > > Hi
> > > I am not clear on the purpose of this extension.
> > >
> > > The Bugzilla just describes the solution.
> > > But what is the problem you are trying to resolve?
> > >
> > > I completely don’t understand.
> > >
> > > Please do consider add the background information there.
> > > Or it is hard for me to comment.
> > >
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > Sent: Friday, January 3, 2020 11:04 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > > > Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> > > > <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > > > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > > > <stefanb@linux.ibm.com>
> > > > Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> > > > Interface
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> > > >
> > > > 1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
> > > > Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to Tcg2PpVendorLib.
> It
> > > > has one more parameter PPData (type EFI_TCG2_PHYSICAL_PRESENCE) to
> > > > transfer more data.
> > > > 2. Use the Ex version instead of original one in
> > > > Tcg2PhysicalPresenceLib 3. Add a pcd
> > > > PcdPhysicalPresenceUserConfirmTimeout to control the user confirm
> > > > input key timeout.
> > > > 4. Add FunctionIndex to structure type EFI_TCG2_PHYSICAL_PRESENCE to
> > > > transfer
> > > > mTcgNvs->PhysicalPresence.Parameter data.
> > > > 5. Add parameter FunctionIndex to
> > > > Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
> > > > to initialize the PPdata.
> >
> > Background:
> > Some platforms implement their own Tcg2PpVendorLib and want to operate
> > with more parameters.
> >
> > #1-#2 and #4-#5 changes aim to add extend the interface with PPdata. PPdata
> > pointer can transfer most of the required parameter to do the operation.
> > All the extend changes is for the PPdata and transfer to the platform
> > implemented interfaces. This would affect the platforms which implement
> their
> > own Tcg2PpVendorLib.
> > But for open source platform, I didn't see any implementation of it.
> >
> > #3 aims to add a pcd to let the customers to decide whether to wait for the
> > input forever or with a timeout count.
> >
> > > > 6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
> > > > SecurityPkg/Include.
> > > > It is useful for platform code to implement their own
> > Tcg2PhysicalPresenceLib.
> >
> > #6 is a movement to decrease the duplicated code at platform side if the
> > platform code implement its own TCG library or driver.
> >
> > > > 7. Replace the ASSERT with error code return in TpmPhysicalPresenceLib
> >
> > #7 aims to remove the ASSERT because it is not critical. ASSERT when fail to
> call
> > TpmPhysicalPresence and GetTpmCapability is not a good behavior.
> >
> > > > 8. Fix one operation
> > > > (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
> > > > TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page 37).
> >
> > #8 is a bug fix to follow the spec.
> >
> > Thanks,
> > Zhichao
> >
> > > >
> > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> Zhichao Gao (13):
> > > > SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
> > > > SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function
> > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
> > > > SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
> > > > SecurityPkg/dec: Add a pcd for user response wait time
> > > > OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
> > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
> > > > SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
> > > > SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata
> > > > SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
> > > > SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
> > > > SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code
> > > > SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
> > > >
> > > > .../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
> > > > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > > > .../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
> > > > .../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
> > > > SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
> > > > .../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
> > > > .../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
> > > > .../DxeTcg2PhysicalPresenceLib.inf | 4 +-
> > > > .../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
> > > > .../DxeTcgPhysicalPresenceLib.inf | 6 +-
> > > > .../SmmTcg2PhysicalPresenceLib.c | 15 ++-
> > > > .../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
> > > > SecurityPkg/SecurityPkg.dec | 7 +-
> > > > SecurityPkg/SecurityPkg.uni | 7 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
> > > > SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
> > > > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
> > > > 21 files changed, 347 insertions(+), 95 deletions(-) rename
> > > > SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
> > > >
> > > > --
> > > > 2.21.0.windows.1
>
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
2020-01-03 3:04 ` [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use " Gao, Zhichao
@ 2020-01-03 14:21 ` Laszlo Ersek
2020-01-15 8:03 ` Gao, Zhichao
2020-01-19 7:03 ` Gao, Zhichao
0 siblings, 2 replies; 26+ messages in thread
From: Laszlo Ersek @ 2020-01-03 14:21 UTC (permalink / raw)
To: devel, zhichao.gao
Cc: Jiewen Yao, Jian J Wang, Chao Zhang, Jordan Justen,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
Hello Zhichao,
On 01/03/20 04:04, Gao, Zhichao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
>
> Use the pcd PcdPhysicalPresenceUserConfirmTimeout to control the
> wait time of user response.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
> .../DxeTcg2PhysicalPresenceLib.c | 63 ++++++++++++++-----
> .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> 2 files changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
> index 00d76ba2c2..13f78cbfac 100644
> --- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
> +++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
> @@ -9,7 +9,7 @@
>
> Copyright (C) 2018, Red Hat, Inc.
> Copyright (c) 2018, IBM Corporation. All rights reserved.<BR>
> -Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -32,6 +32,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> #include <Library/UefiRuntimeServicesTableLib.h>
> +#include <Library/TimerLib.h>
> +#include <Library/PcdLib.h>
>
> #include <Library/Tcg2PhysicalPresenceLib.h>
>
> @@ -365,28 +367,57 @@ Tcg2ReadUserKey (
> {
> EFI_STATUS Status;
> EFI_INPUT_KEY Key;
> - UINT16 InputKey;
> + UINT16 ConfirmKey;
> + UINTN Interval;
> + INT64 Timeout;
>
> - InputKey = 0;
> + //
> + // delay 100 milli-second
> + //
> + Interval = 100;
> + ConfirmKey = (CautionKey) ? SCAN_F12 : SCAN_F10;
> + Timeout = (INT64)PcdGet32 (PcdPhysicalPresenceUserConfirmTimeout);
> + if (Timeout > 0) {
> + Timeout = (INT64)MultU64x32 ((UINT64)Timeout, 1000);
> + } else {
> + //
> + // Wait forever
> + //
> + Timeout = MAX_INT64;
> + }
> +
> + //
> + // Wait for user response within the time-out
> + //
> do {
> + MicroSecondDelay (Interval * 1000);
> +
> Status = gBS->CheckEvent (gST->ConIn->WaitForKey);
> if (!EFI_ERROR (Status)) {
> Status = gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
> - if (Key.ScanCode == SCAN_ESC) {
> - InputKey = Key.ScanCode;
> - }
> - if ((Key.ScanCode == SCAN_F10) && !CautionKey) {
> - InputKey = Key.ScanCode;
> - }
> - if ((Key.ScanCode == SCAN_F12) && CautionKey) {
> - InputKey = Key.ScanCode;
> + if (!EFI_ERROR (Status)) {
> + if (Key.ScanCode == ConfirmKey) {
> + //
> + // User Confirmation
> + //
> + return TRUE;
> + }
> +
> + if (Key.ScanCode == SCAN_ESC) {
> + //
> + // User Rejection
> + //
> + return FALSE;
> + }
> + } else if (Status == EFI_DEVICE_ERROR) {
> + //
> + // If error, assume User Rejection
> + //
> + return FALSE;
> }
> }
> - } while (InputKey == 0);
> -
> - if (InputKey != SCAN_ESC) {
> - return TRUE;
> - }
> + Timeout -= Interval;
> + } while (Timeout > 0);
>
> return FALSE;
> }
(1) I don't understand why the original (pre-patch) code uses
CheckEvent() in a busy loop. WaitForEvent() looks like a better (more
resource-conservative) option.
Does the original code use CheckEvent() because WaitForEvent() is
restricted to TPL_APPLICATION?
I don't think that being restricted to TPL_APPLICATION should be a
problem. As far as I can tell, the only call path to Tcg2ReadUserKey() is:
Tcg2PhysicalPresenceLibProcessRequest()
Tcg2ExecutePendingTpmRequest()
Tcg2UserConfirm()
Tcg2ReadUserKey()
In turn, Tcg2PhysicalPresenceLibProcessRequest() is supposed to be
called from PlatformBootManagerLib, which in turn is called from BdsDxe.
Therefore I think we can safely assume TPL_APPLICATION.
I think we should add a separate patch first, for rewriting the present
logic of Tcg2ReadUserKey() with WaitForEvent() -- remove the busy loop.
(2) Once we use WaitForEvent(), it is easier and more elegant to use a
timer event, in addition to "gST->ConIn->WaitForKey", to limit the
waiting for a keypress.
For example, the BdsWaitForSingleEvent() function in
"MdeModulePkg/Universal/BdsDxe/BdsEntry.c" is used for implementing a
timed wait for a hotkey, if I understand correctly.
(3) I think that the Tcg2ReadUserKey() function should take the timeout
parameter, and the PCD should be consumed in the Tcg2UserConfirm()
function instead. Tcg2UserConfirm() should pass the value of the PCD to
Tcg2ReadUserKey().
The PCD is called "PcdPhysicalPresenceUserConfirmTimeout", therefore it
seems more closely tied to the Tcg2UserConfirm() function, in purpose.
The Tcg2ReadUserKey() function seems to be a general utility function,
so it should take a parameter, rather than fetch a particular PCD.
(4) The comment block on the Tcg2ReadUserKey() function should be updated.
(5) Please make sure that you format the patches next time with the
"--stat=1000 --stat-graph-width=20" options. The pathnames of the files
that are modified by this patch set are very long, and they are
truncated in the diffstats. That way, the diffstat is not helpful.
The "BaseTools/Scripts/SetupGit.py" script can automate at least the
"--stat-graph-width=20" option for you.
(6) When posting several patches, or large patches, it is helpful for
reviewers if you also push your topic branch to your local repo, and
expose the corresponding URL / branch name in the cover letter. Some
patches are easier to review locally (after a git-fetch from your repo).
(7) I don't know what happened, but I can see only patch#0 and patch#6
from this series in my list folder. There are multiple lib instances
being modified, and I couldn't compare the changes between those.
Thanks
Laszlo
> diff --git a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> index 85ce0e2b29..701de1836c 100644
> --- a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> +++ b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
> @@ -20,7 +20,7 @@
> # This external input must be validated carefully to avoid security issue.
> #
> # Copyright (C) 2018, Red Hat, Inc.
> -# Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
> #
> ##
> @@ -62,10 +62,14 @@
> UefiBootServicesTableLib
> UefiLib
> UefiRuntimeServicesTableLib
> + TimerLib
>
> [Protocols]
> gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
>
> +[Pcd]
> + gEfiSecurityPkgTokenSpaceGuid.PcdPhysicalPresenceUserConfirmTimeout
> +
> [Guids]
> ## SOMETIMES_CONSUMES ## HII
> gEfiTcg2PhysicalPresenceGuid
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
2020-01-03 5:59 ` [edk2-devel] " Yao, Jiewen
@ 2020-01-07 8:05 ` Gao, Zhichao
2020-01-07 8:31 ` Yao, Jiewen
0 siblings, 1 reply; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-07 8:05 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io
Cc: Wang, Jian J, Zhang, Chao B, Justen, Jordan L, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Friday, January 3, 2020 1:59 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> <stefanb@linux.ibm.com>
> Subject: RE: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> Presence Interface
>
> Some other comment:
>
> 6. I disagree to move NvData.h to securityPkg.
> This is designed to be an internal variable.
> Why it need to move to security pkg to share with other driver?
>
> I agree that duplicating code is bad solution.
> And exposing internal data structure is as bad as duplicating code.
>
> By design, the platform can have its own TCG platform lib and its own NvData.h.
> Would you please help me understand why this is needed?
Some definitions in the NvData.h are useful for Tcg2VerndorLib, such as:
#define TPM_DEVICE_NULL 0
#define TPM_DEVICE_1_2 1
#define TPM_DEVICE_2_0_DTPM 2
#define TPM_DEVICE_MIN TPM_DEVICE_1_2
#define TPM_DEVICE_MAX TPM_DEVICE_2_0_DTPM
#define TPM_DEVICE_DEFAULT TPM_DEVICE_1_2
I'd better to move the common definition to a normal tcg device related header file instead of moving this file.
>
> 7. Removing assert has no relationship with this extend PPI interface.
> If you want, please file another Bugzilla and handle it separately.
>
> 8. Fixing bug to follow spec is good.
> But I do not see the relationship with the PPI interface extension.
> Please file another Bugzilla and handle that separately.
>
For #7, #8 and other non-extend changes, I would add new Bugzilla to separate the patches.
Thanks,
Zhichao
>
> Thank you
> Yao Jiewen
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> > Jiewen
> > Sent: Friday, January 3, 2020 1:30 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > Biesheuvel <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> > Subject: Re: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2
> > Physical Presence Interface
> >
> > What is the platform use case?
> > Please give at least some examples.
> >
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Friday, January 3, 2020 1:08 PM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > > Biesheuvel
> > <ard.biesheuvel@linaro.org>;
> > > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > > <stefanb@linux.ibm.com>
> > > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > Presence Interface
> > >
> > > See below.
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen
> > > > Sent: Friday, January 3, 2020 11:09 AM
> > > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > > <chao.b.zhang@intel.com>; Justen, Jordan L
> > > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > > > Biesheuvel <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > > > <marcandre.lureau@redhat.com>; Stefan Berger
> > > > <stefanb@linux.ibm.com>
> > > > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > > Presence Interface
> > > >
> > > > Hi
> > > > I am not clear on the purpose of this extension.
> > > >
> > > > The Bugzilla just describes the solution.
> > > > But what is the problem you are trying to resolve?
> > > >
> > > > I completely don’t understand.
> > > >
> > > > Please do consider add the background information there.
> > > > Or it is hard for me to comment.
> > > >
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > > > -----Original Message-----
> > > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > > Sent: Friday, January 3, 2020 11:04 AM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > > > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > > > > Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> > > > > <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > > > > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > > > > <stefanb@linux.ibm.com>
> > > > > Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > > > Presence Interface
> > > > >
> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> > > > >
> > > > > 1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
> > > > > Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to Tcg2PpVendorLib.
> > It
> > > > > has one more parameter PPData (type EFI_TCG2_PHYSICAL_PRESENCE)
> > > > > to transfer more data.
> > > > > 2. Use the Ex version instead of original one in
> > > > > Tcg2PhysicalPresenceLib 3. Add a pcd
> > > > > PcdPhysicalPresenceUserConfirmTimeout to control the user
> > > > > confirm input key timeout.
> > > > > 4. Add FunctionIndex to structure type
> > > > > EFI_TCG2_PHYSICAL_PRESENCE to transfer
> > > > > mTcgNvs->PhysicalPresence.Parameter data.
> > > > > 5. Add parameter FunctionIndex to
> > > > > Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
> > > > > to initialize the PPdata.
> > >
> > > Background:
> > > Some platforms implement their own Tcg2PpVendorLib and want to
> > > operate with more parameters.
> > >
> > > #1-#2 and #4-#5 changes aim to add extend the interface with PPdata.
> > > PPdata pointer can transfer most of the required parameter to do the
> operation.
> > > All the extend changes is for the PPdata and transfer to the
> > > platform implemented interfaces. This would affect the platforms
> > > which implement
> > their
> > > own Tcg2PpVendorLib.
> > > But for open source platform, I didn't see any implementation of it.
> > >
> > > #3 aims to add a pcd to let the customers to decide whether to wait
> > > for the input forever or with a timeout count.
> > >
> > > > > 6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
> > > > > SecurityPkg/Include.
> > > > > It is useful for platform code to implement their own
> > > Tcg2PhysicalPresenceLib.
> > >
> > > #6 is a movement to decrease the duplicated code at platform side if
> > > the platform code implement its own TCG library or driver.
> > >
> > > > > 7. Replace the ASSERT with error code return in
> > > > > TpmPhysicalPresenceLib
> > >
> > > #7 aims to remove the ASSERT because it is not critical. ASSERT when
> > > fail to
> > call
> > > TpmPhysicalPresence and GetTpmCapability is not a good behavior.
> > >
> > > > > 8. Fix one operation
> > > > > (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
> > > > > TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page
> 37).
> > >
> > > #8 is a bug fix to follow the spec.
> > >
> > > Thanks,
> > > Zhichao
> > >
> > > > >
> > > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> Zhichao Gao (13):
> > > > > SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
> > > > > SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function
> > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
> > > > > SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
> > > > > SecurityPkg/dec: Add a pcd for user response wait time
> > > > > OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
> > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
> > > > > SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
> > > > > SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata
> > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
> > > > > SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
> > > > > SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error
> code
> > > > > SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
> > > > >
> > > > > .../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
> > > > > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > > > > .../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
> > > > > .../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
> > > > > SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
> > > > > .../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
> > > > > .../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
> > > > > .../DxeTcg2PhysicalPresenceLib.inf | 4 +-
> > > > > .../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
> > > > > .../DxeTcgPhysicalPresenceLib.inf | 6 +-
> > > > > .../SmmTcg2PhysicalPresenceLib.c | 15 ++-
> > > > > .../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
> > > > > SecurityPkg/SecurityPkg.dec | 7 +-
> > > > > SecurityPkg/SecurityPkg.uni | 7 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
> > > > > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
> > > > > 21 files changed, 347 insertions(+), 95 deletions(-) rename
> > > > > SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
> > > > >
> > > > > --
> > > > > 2.21.0.windows.1
> >
> >
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
2020-01-07 8:05 ` Gao, Zhichao
@ 2020-01-07 8:31 ` Yao, Jiewen
0 siblings, 0 replies; 26+ messages in thread
From: Yao, Jiewen @ 2020-01-07 8:31 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Wang, Jian J, Zhang, Chao B, Justen, Jordan L, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
Below:
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Tuesday, January 7, 2020 4:06 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> <stefanb@linux.ibm.com>
> Subject: RE: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> Presence Interface
>
>
>
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Friday, January 3, 2020 1:59 PM
> > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Gao,
> Zhichao
> > <zhichao.gao@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>;
> > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > <stefanb@linux.ibm.com>
> > Subject: RE: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > Presence Interface
> >
> > Some other comment:
> >
> > 6. I disagree to move NvData.h to securityPkg.
> > This is designed to be an internal variable.
> > Why it need to move to security pkg to share with other driver?
> >
> > I agree that duplicating code is bad solution.
> > And exposing internal data structure is as bad as duplicating code.
> >
> > By design, the platform can have its own TCG platform lib and its own
> NvData.h.
> > Would you please help me understand why this is needed?
>
> Some definitions in the NvData.h are useful for Tcg2VerndorLib, such as:
> #define TPM_DEVICE_NULL 0
> #define TPM_DEVICE_1_2 1
> #define TPM_DEVICE_2_0_DTPM 2
> #define TPM_DEVICE_MIN TPM_DEVICE_1_2
> #define TPM_DEVICE_MAX TPM_DEVICE_2_0_DTPM
> #define TPM_DEVICE_DEFAULT TPM_DEVICE_1_2
> I'd better to move the common definition to a normal tcg device related header
> file instead of moving this file.
[Jiewen] No.
This is driver internal information. No need to share it.
Other driver may use different order.
> > 7. Removing assert has no relationship with this extend PPI interface.
> > If you want, please file another Bugzilla and handle it separately.
> >
> > 8. Fixing bug to follow spec is good.
> > But I do not see the relationship with the PPI interface extension.
> > Please file another Bugzilla and handle that separately.
> >
>
> For #7, #8 and other non-extend changes, I would add new Bugzilla to separate
> the patches.
[Jiewen] Thank you.
>
> Thanks,
> Zhichao
>
> >
> > Thank you
> > Yao Jiewen
> >
> >
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> > > Jiewen
> > > Sent: Friday, January 3, 2020 1:30 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > > Biesheuvel <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > > <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> > > Subject: Re: [edk2-devel] [PATCH 00/13] Extend and fix the TCG/TCG2
> > > Physical Presence Interface
> > >
> > > What is the platform use case?
> > > Please give at least some examples.
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > Sent: Friday, January 3, 2020 1:08 PM
> > > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > > <chao.b.zhang@intel.com>; Justen, Jordan L
> > > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > > > Biesheuvel
> > > <ard.biesheuvel@linaro.org>;
> > > > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > > > <stefanb@linux.ibm.com>
> > > > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > > Presence Interface
> > > >
> > > > See below.
> > > >
> > > > > -----Original Message-----
> > > > > From: Yao, Jiewen
> > > > > Sent: Friday, January 3, 2020 11:09 AM
> > > > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > > > <chao.b.zhang@intel.com>; Justen, Jordan L
> > > > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > > > > Biesheuvel <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > > > > <marcandre.lureau@redhat.com>; Stefan Berger
> > > > > <stefanb@linux.ibm.com>
> > > > > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > > > Presence Interface
> > > > >
> > > > > Hi
> > > > > I am not clear on the purpose of this extension.
> > > > >
> > > > > The Bugzilla just describes the solution.
> > > > > But what is the problem you are trying to resolve?
> > > > >
> > > > > I completely don’t understand.
> > > > >
> > > > > Please do consider add the background information there.
> > > > > Or it is hard for me to comment.
> > > > >
> > > > >
> > > > > Thank you
> > > > > Yao Jiewen
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > > > Sent: Friday, January 3, 2020 11:04 AM
> > > > > > To: devel@edk2.groups.io
> > > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > > > > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > > > > > Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> > > > > > <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > > > > > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > > > > > <stefanb@linux.ibm.com>
> > > > > > Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > > > > Presence Interface
> > > > > >
> > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> > > > > >
> > > > > > 1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
> > > > > > Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to
> Tcg2PpVendorLib.
> > > It
> > > > > > has one more parameter PPData (type EFI_TCG2_PHYSICAL_PRESENCE)
> > > > > > to transfer more data.
> > > > > > 2. Use the Ex version instead of original one in
> > > > > > Tcg2PhysicalPresenceLib 3. Add a pcd
> > > > > > PcdPhysicalPresenceUserConfirmTimeout to control the user
> > > > > > confirm input key timeout.
> > > > > > 4. Add FunctionIndex to structure type
> > > > > > EFI_TCG2_PHYSICAL_PRESENCE to transfer
> > > > > > mTcgNvs->PhysicalPresence.Parameter data.
> > > > > > 5. Add parameter FunctionIndex to
> > > > > > Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
> > > > > > to initialize the PPdata.
> > > >
> > > > Background:
> > > > Some platforms implement their own Tcg2PpVendorLib and want to
> > > > operate with more parameters.
> > > >
> > > > #1-#2 and #4-#5 changes aim to add extend the interface with PPdata.
> > > > PPdata pointer can transfer most of the required parameter to do the
> > operation.
> > > > All the extend changes is for the PPdata and transfer to the
> > > > platform implemented interfaces. This would affect the platforms
> > > > which implement
> > > their
> > > > own Tcg2PpVendorLib.
> > > > But for open source platform, I didn't see any implementation of it.
> > > >
> > > > #3 aims to add a pcd to let the customers to decide whether to wait
> > > > for the input forever or with a timeout count.
> > > >
> > > > > > 6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
> > > > > > SecurityPkg/Include.
> > > > > > It is useful for platform code to implement their own
> > > > Tcg2PhysicalPresenceLib.
> > > >
> > > > #6 is a movement to decrease the duplicated code at platform side if
> > > > the platform code implement its own TCG library or driver.
> > > >
> > > > > > 7. Replace the ASSERT with error code return in
> > > > > > TpmPhysicalPresenceLib
> > > >
> > > > #7 aims to remove the ASSERT because it is not critical. ASSERT when
> > > > fail to
> > > call
> > > > TpmPhysicalPresence and GetTpmCapability is not a good behavior.
> > > >
> > > > > > 8. Fix one operation
> > > > > > (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
> > > > > > TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page
> > 37).
> > > >
> > > > #8 is a bug fix to follow the spec.
> > > >
> > > > Thanks,
> > > > Zhichao
> > > >
> > > > > >
> > > > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > > > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> Zhichao Gao (13):
> > > > > > SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
> > > > > > SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex
> function
> > > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
> > > > > > SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
> > > > > > SecurityPkg/dec: Add a pcd for user response wait time
> > > > > > OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait
> time
> > > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
> > > > > > SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
> > > > > > SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for
> PPdata
> > > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
> > > > > > SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
> > > > > > SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error
> > code
> > > > > > SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
> > > > > >
> > > > > > .../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
> > > > > > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > > > > > .../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
> > > > > > .../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
> > > > > > SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
> > > > > > .../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
> > > > > > .../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
> > > > > > .../DxeTcg2PhysicalPresenceLib.inf | 4 +-
> > > > > > .../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
> > > > > > .../DxeTcgPhysicalPresenceLib.inf | 6 +-
> > > > > > .../SmmTcg2PhysicalPresenceLib.c | 15 ++-
> > > > > > .../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
> > > > > > SecurityPkg/SecurityPkg.dec | 7 +-
> > > > > > SecurityPkg/SecurityPkg.uni | 7 +-
> > > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
> > > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
> > > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
> > > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
> > > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
> > > > > > SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
> > > > > > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
> > > > > > 21 files changed, 347 insertions(+), 95 deletions(-) rename
> > > > > > SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
> > > > > >
> > > > > > --
> > > > > > 2.21.0.windows.1
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
2020-01-03 5:30 ` Yao, Jiewen
@ 2020-01-09 9:05 ` Gao, Zhichao
2020-01-09 9:22 ` Yao, Jiewen
0 siblings, 1 reply; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-09 9:05 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io
Cc: Wang, Jian J, Zhang, Chao B, Justen, Jordan L, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
Jiewen,
The Tcg2Smm driver support to submit the TPM operation request to pre-OS environment and pre-OS environment, but it only transfer the operation request code and the request parameter and save to the "Tcg2PhysicalPresence" variable.
But at the execution part, the interface only transfer the operation request code. See blow:
UINT32
EFIAPI
Tcg2PpVendorLibExecutePendingRequest (
IN TPM2B_AUTH *PlatformAuth, OPTIONAL
IN UINT32 OperationRequest,
IN OUT UINT32 *ManagementFlags,
OUT BOOLEAN *ResetRequired
);
This can't meet some vendors' requirement. It drop the request parameter and we need it for some operation or validation.
That is why we want to extend the interface with the PPdata, it contain the whole parameters we need.
About the function Index, same reason, we need it to do some operations and validations.
Transfer the PPdata pointer can solve all the problems.
Thanks,
Zhichao
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Friday, January 3, 2020 1:30 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> Interface
>
> What is the platform use case?
> Please give at least some examples.
>
>
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Friday, January 3, 2020 1:08 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > Biesheuvel <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>; Stefan Berger
> <stefanb@linux.ibm.com>
> > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > Presence Interface
> >
> > See below.
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen
> > > Sent: Friday, January 3, 2020 11:09 AM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > > Biesheuvel <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > > <marcandre.lureau@redhat.com>; Stefan Berger
> <stefanb@linux.ibm.com>
> > > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > Presence Interface
> > >
> > > Hi
> > > I am not clear on the purpose of this extension.
> > >
> > > The Bugzilla just describes the solution.
> > > But what is the problem you are trying to resolve?
> > >
> > > I completely don’t understand.
> > >
> > > Please do consider add the background information there.
> > > Or it is hard for me to comment.
> > >
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > Sent: Friday, January 3, 2020 11:04 AM
> > > > To: devel@edk2.groups.io
> > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > > > Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> > > > <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > > > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > > > <stefanb@linux.ibm.com>
> > > > Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > > Presence Interface
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> > > >
> > > > 1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
> > > > Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to
> Tcg2PpVendorLib.
> > > > It has one more parameter PPData (type
> EFI_TCG2_PHYSICAL_PRESENCE)
> > > > to transfer more data.
> > > > 2. Use the Ex version instead of original one in
> > > > Tcg2PhysicalPresenceLib 3. Add a pcd
> > > > PcdPhysicalPresenceUserConfirmTimeout to control the user confirm
> > > > input key timeout.
> > > > 4. Add FunctionIndex to structure type EFI_TCG2_PHYSICAL_PRESENCE
> > > > to transfer
> > > > mTcgNvs->PhysicalPresence.Parameter data.
> > > > 5. Add parameter FunctionIndex to
> > > > Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
> > > > to initialize the PPdata.
> >
> > Background:
> > Some platforms implement their own Tcg2PpVendorLib and want to
> operate
> > with more parameters.
> >
> > #1-#2 and #4-#5 changes aim to add extend the interface with PPdata.
> > PPdata pointer can transfer most of the required parameter to do the
> operation.
> > All the extend changes is for the PPdata and transfer to the platform
> > implemented interfaces. This would affect the platforms which
> > implement their own Tcg2PpVendorLib.
> > But for open source platform, I didn't see any implementation of it.
> >
> > #3 aims to add a pcd to let the customers to decide whether to wait
> > for the input forever or with a timeout count.
> >
> > > > 6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
> > > > SecurityPkg/Include.
> > > > It is useful for platform code to implement their own
> > Tcg2PhysicalPresenceLib.
> >
> > #6 is a movement to decrease the duplicated code at platform side if
> > the platform code implement its own TCG library or driver.
> >
> > > > 7. Replace the ASSERT with error code return in
> > > > TpmPhysicalPresenceLib
> >
> > #7 aims to remove the ASSERT because it is not critical. ASSERT when
> > fail to call TpmPhysicalPresence and GetTpmCapability is not a good
> behavior.
> >
> > > > 8. Fix one operation
> > > > (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
> > > > TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page
> 37).
> >
> > #8 is a bug fix to follow the spec.
> >
> > Thanks,
> > Zhichao
> >
> > > >
> > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> Zhichao Gao (13):
> > > > SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
> > > > SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex
> function
> > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
> > > > SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
> > > > SecurityPkg/dec: Add a pcd for user response wait time
> > > > OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait
> time
> > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
> > > > SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
> > > > SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for
> PPdata
> > > > SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
> > > > SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
> > > > SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error
> code
> > > > SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
> > > >
> > > > .../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
> > > > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > > > .../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
> > > > .../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
> > > > SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
> > > > .../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
> > > > .../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
> > > > .../DxeTcg2PhysicalPresenceLib.inf | 4 +-
> > > > .../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
> > > > .../DxeTcgPhysicalPresenceLib.inf | 6 +-
> > > > .../SmmTcg2PhysicalPresenceLib.c | 15 ++-
> > > > .../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
> > > > SecurityPkg/SecurityPkg.dec | 7 +-
> > > > SecurityPkg/SecurityPkg.uni | 7 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
> > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
> > > > SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
> > > > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
> > > > 21 files changed, 347 insertions(+), 95 deletions(-) rename
> > > > SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
> > > >
> > > > --
> > > > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface
2020-01-09 9:05 ` Gao, Zhichao
@ 2020-01-09 9:22 ` Yao, Jiewen
0 siblings, 0 replies; 26+ messages in thread
From: Yao, Jiewen @ 2020-01-09 9:22 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Wang, Jian J, Zhang, Chao B, Justen, Jordan L, Laszlo Ersek,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
OK. But the PPData is internal implementation, it should never be used as parameter.
If what is needed is the Parameter, please use RequestParameter explicitly and align with the TCG2 PP lib.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Thursday, January 9, 2020 5:06 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> <stefanb@linux.ibm.com>
> Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> Interface
>
> Jiewen,
>
> The Tcg2Smm driver support to submit the TPM operation request to pre-OS
> environment and pre-OS environment, but it only transfer the operation request
> code and the request parameter and save to the "Tcg2PhysicalPresence"
> variable.
> But at the execution part, the interface only transfer the operation request code.
> See blow:
> UINT32
> EFIAPI
> Tcg2PpVendorLibExecutePendingRequest (
> IN TPM2B_AUTH *PlatformAuth, OPTIONAL
> IN UINT32 OperationRequest,
> IN OUT UINT32 *ManagementFlags,
> OUT BOOLEAN *ResetRequired
> );
> This can't meet some vendors' requirement. It drop the request parameter and
> we need it for some operation or validation.
> That is why we want to extend the interface with the PPdata, it contain the
> whole parameters we need.
>
> About the function Index, same reason, we need it to do some operations and
> validations.
> Transfer the PPdata pointer can solve all the problems.
>
> Thanks,
> Zhichao
>
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Friday, January 3, 2020 1:30 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > <chao.b.zhang@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> > Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence
> > Interface
> >
> > What is the platform use case?
> > Please give at least some examples.
> >
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Friday, January 3, 2020 1:08 PM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > <chao.b.zhang@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > > Biesheuvel <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > > <marcandre.lureau@redhat.com>; Stefan Berger
> > <stefanb@linux.ibm.com>
> > > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > Presence Interface
> > >
> > > See below.
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen
> > > > Sent: Friday, January 3, 2020 11:09 AM
> > > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Chao B
> > > > <chao.b.zhang@intel.com>; Justen, Jordan L
> > > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> > > > Biesheuvel <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > > > <marcandre.lureau@redhat.com>; Stefan Berger
> > <stefanb@linux.ibm.com>
> > > > Subject: RE: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > > Presence Interface
> > > >
> > > > Hi
> > > > I am not clear on the purpose of this extension.
> > > >
> > > > The Bugzilla just describes the solution.
> > > > But what is the problem you are trying to resolve?
> > > >
> > > > I completely don’t understand.
> > > >
> > > > Please do consider add the background information there.
> > > > Or it is hard for me to comment.
> > > >
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > > > -----Original Message-----
> > > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > > Sent: Friday, January 3, 2020 11:04 AM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > > > > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > > > > Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
> > > > > <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > > > > Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger
> > > > > <stefanb@linux.ibm.com>
> > > > > Subject: [PATCH 00/13] Extend and fix the TCG/TCG2 Physical
> > > > > Presence Interface
> > > > >
> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> > > > >
> > > > > 1. Add two interfaces Tcg2PpVendorLibExecutePendingRequestEx and
> > > > > Tcg2PpVendorLibSubmitRequestToPreOSFunctionEx to
> > Tcg2PpVendorLib.
> > > > > It has one more parameter PPData (type
> > EFI_TCG2_PHYSICAL_PRESENCE)
> > > > > to transfer more data.
> > > > > 2. Use the Ex version instead of original one in
> > > > > Tcg2PhysicalPresenceLib 3. Add a pcd
> > > > > PcdPhysicalPresenceUserConfirmTimeout to control the user confirm
> > > > > input key timeout.
> > > > > 4. Add FunctionIndex to structure type EFI_TCG2_PHYSICAL_PRESENCE
> > > > > to transfer
> > > > > mTcgNvs->PhysicalPresence.Parameter data.
> > > > > 5. Add parameter FunctionIndex to
> > > > > Tcg2PhysicalPresenceLibSubmitRequestToPreOSFunctionEx
> > > > > to initialize the PPdata.
> > >
> > > Background:
> > > Some platforms implement their own Tcg2PpVendorLib and want to
> > operate
> > > with more parameters.
> > >
> > > #1-#2 and #4-#5 changes aim to add extend the interface with PPdata.
> > > PPdata pointer can transfer most of the required parameter to do the
> > operation.
> > > All the extend changes is for the PPdata and transfer to the platform
> > > implemented interfaces. This would affect the platforms which
> > > implement their own Tcg2PpVendorLib.
> > > But for open source platform, I didn't see any implementation of it.
> > >
> > > #3 aims to add a pcd to let the customers to decide whether to wait
> > > for the input forever or with a timeout count.
> > >
> > > > > 6. Move Tcg2ConfigNvData.h from SecurityPkg/Tcg/Tcg2Config to
> > > > > SecurityPkg/Include.
> > > > > It is useful for platform code to implement their own
> > > Tcg2PhysicalPresenceLib.
> > >
> > > #6 is a movement to decrease the duplicated code at platform side if
> > > the platform code implement its own TCG library or driver.
> > >
> > > > > 7. Replace the ASSERT with error code return in
> > > > > TpmPhysicalPresenceLib
> > >
> > > #7 aims to remove the ASSERT because it is not critical. ASSERT when
> > > fail to call TpmPhysicalPresence and GetTpmCapability is not a good
> > behavior.
> > >
> > > > > 8. Fix one operation
> > > > > (PHYSICAL_PRESENCE_DEACTIVATE_DISABLE_OWNER_FALSE) flow of
> > > > > TcgPhysicalPresenceLib (refer to Physical Presence Interface Spec Page
> > 37).
> > >
> > > #8 is a bug fix to follow the spec.
> > >
> > > Thanks,
> > > Zhichao
> > >
> > > > >
> > > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com> Zhichao Gao (13):
> > > > > SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata
> > > > > SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex
> > function
> > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use the new Ex function
> > > > > SecurityPkg/SmmTcg2PhysicalPresenceLib: Use the new Ex function
> > > > > SecurityPkg/dec: Add a pcd for user response wait time
> > > > > OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait
> > time
> > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp wait time
> > > > > SecurityPkg/TcgPyhsicalPresenceLib: Use Pcd for user resp wait time
> > > > > SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for
> > PPdata
> > > > > SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func
> > > > > SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder
> > > > > SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error
> > code
> > > > > SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11
> > > > >
> > > > > .../DxeTcg2PhysicalPresenceLib.c | 63 +++++++---
> > > > > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > > > > .../Include/Guid/Tcg2PhysicalPresenceData.h | 3 +-
> > > > > .../Include/Library/Tcg2PhysicalPresenceLib.h | 4 +-
> > > > > SecurityPkg/Include/Library/Tcg2PpVendorLib.h | 54 ++++++++-
> > > > > .../Tcg2Config => Include}/Tcg2ConfigNvData.h | 2 +-
> > > > > .../DxeTcg2PhysicalPresenceLib.c | 68 ++++++++---
> > > > > .../DxeTcg2PhysicalPresenceLib.inf | 4 +-
> > > > > .../DxeTcgPhysicalPresenceLib.c | 110 ++++++++++++------
> > > > > .../DxeTcgPhysicalPresenceLib.inf | 6 +-
> > > > > .../SmmTcg2PhysicalPresenceLib.c | 15 ++-
> > > > > .../Tcg2PpVendorLibNull/Tcg2PpVendorLibNull.c | 61 +++++++++-
> > > > > SecurityPkg/SecurityPkg.dec | 7 +-
> > > > > SecurityPkg/SecurityPkg.uni | 7 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2Config.vfr | 4 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDxe.inf | 3 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h | 4 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 3 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigPeim.c | 4 +-
> > > > > SecurityPkg/Tcg/Tcg2Config/TpmDetection.c | 4 +-
> > > > > SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.c | 10 +-
> > > > > 21 files changed, 347 insertions(+), 95 deletions(-) rename
> > > > > SecurityPkg/{Tcg/Tcg2Config => Include}/Tcg2ConfigNvData.h (94%)
> > > > >
> > > > > --
> > > > > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
2020-01-03 14:21 ` [edk2-devel] " Laszlo Ersek
@ 2020-01-15 8:03 ` Gao, Zhichao
2020-01-19 7:03 ` Gao, Zhichao
1 sibling, 0 replies; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-15 8:03 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: Yao, Jiewen, Wang, Jian J, Zhang, Chao B, Justen, Jordan L,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, January 3, 2020 10:21 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> Subject: Re: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib:
> Use pcd for user response wait time
>
> Hello Zhichao,
>
> On 01/03/20 04:04, Gao, Zhichao wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> >
> > Use the pcd PcdPhysicalPresenceUserConfirmTimeout to control the wait
> > time of user response.
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> > .../DxeTcg2PhysicalPresenceLib.c | 63 ++++++++++++++-----
> > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > 2 files changed, 52 insertions(+), 17 deletions(-)
> >
> > diff --git
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.c
> >
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.c
> > index 00d76ba2c2..13f78cbfac 100644
> > ---
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.c
> > +++
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPrese
> > +++ nceLib.c
> > @@ -9,7 +9,7 @@
> >
> > Copyright (C) 2018, Red Hat, Inc.
> > Copyright (c) 2018, IBM Corporation. All rights reserved.<BR>
> > -Copyright (c) 2013 - 2016, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2013 - 2020, Intel Corporation. All rights
> > +reserved.<BR>
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > **/
> > @@ -32,6 +32,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #include <Library/UefiBootServicesTableLib.h>
> > #include <Library/UefiLib.h>
> > #include <Library/UefiRuntimeServicesTableLib.h>
> > +#include <Library/TimerLib.h>
> > +#include <Library/PcdLib.h>
> >
> > #include <Library/Tcg2PhysicalPresenceLib.h>
> >
> > @@ -365,28 +367,57 @@ Tcg2ReadUserKey ( {
> > EFI_STATUS Status;
> > EFI_INPUT_KEY Key;
> > - UINT16 InputKey;
> > + UINT16 ConfirmKey;
> > + UINTN Interval;
> > + INT64 Timeout;
> >
> > - InputKey = 0;
> > + //
> > + // delay 100 milli-second
> > + //
> > + Interval = 100;
> > + ConfirmKey = (CautionKey) ? SCAN_F12 : SCAN_F10;
> > + Timeout = (INT64)PcdGet32
> (PcdPhysicalPresenceUserConfirmTimeout);
> > + if (Timeout > 0) {
> > + Timeout = (INT64)MultU64x32 ((UINT64)Timeout, 1000);
> > + } else {
> > + //
> > + // Wait forever
> > + //
> > + Timeout = MAX_INT64;
> > + }
> > +
> > + //
> > + // Wait for user response within the time-out //
> > do {
> > + MicroSecondDelay (Interval * 1000);
> > +
> > Status = gBS->CheckEvent (gST->ConIn->WaitForKey);
> > if (!EFI_ERROR (Status)) {
> > Status = gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
> > - if (Key.ScanCode == SCAN_ESC) {
> > - InputKey = Key.ScanCode;
> > - }
> > - if ((Key.ScanCode == SCAN_F10) && !CautionKey) {
> > - InputKey = Key.ScanCode;
> > - }
> > - if ((Key.ScanCode == SCAN_F12) && CautionKey) {
> > - InputKey = Key.ScanCode;
> > + if (!EFI_ERROR (Status)) {
> > + if (Key.ScanCode == ConfirmKey) {
> > + //
> > + // User Confirmation
> > + //
> > + return TRUE;
> > + }
> > +
> > + if (Key.ScanCode == SCAN_ESC) {
> > + //
> > + // User Rejection
> > + //
> > + return FALSE;
> > + }
> > + } else if (Status == EFI_DEVICE_ERROR) {
> > + //
> > + // If error, assume User Rejection
> > + //
> > + return FALSE;
> > }
> > }
> > - } while (InputKey == 0);
> > -
> > - if (InputKey != SCAN_ESC) {
> > - return TRUE;
> > - }
> > + Timeout -= Interval;
> > + } while (Timeout > 0);
> >
> > return FALSE;
> > }
>
> (1) I don't understand why the original (pre-patch) code uses
> CheckEvent() in a busy loop. WaitForEvent() looks like a better (more
> resource-conservative) option.
>
> Does the original code use CheckEvent() because WaitForEvent() is restricted
> to TPL_APPLICATION?
>
> I don't think that being restricted to TPL_APPLICATION should be a problem.
> As far as I can tell, the only call path to Tcg2ReadUserKey() is:
>
> Tcg2PhysicalPresenceLibProcessRequest()
> Tcg2ExecutePendingTpmRequest()
> Tcg2UserConfirm()
> Tcg2ReadUserKey()
>
> In turn, Tcg2PhysicalPresenceLibProcessRequest() is supposed to be called
> from PlatformBootManagerLib, which in turn is called from BdsDxe.
> Therefore I think we can safely assume TPL_APPLICATION.
>
> I think we should add a separate patch first, for rewriting the present logic of
> Tcg2ReadUserKey() with WaitForEvent() -- remove the busy loop.
>
>
> (2) Once we use WaitForEvent(), it is easier and more elegant to use a timer
> event, in addition to "gST->ConIn->WaitForKey", to limit the waiting for a
> keypress.
>
> For example, the BdsWaitForSingleEvent() function in
> "MdeModulePkg/Universal/BdsDxe/BdsEntry.c" is used for implementing a
> timed wait for a hotkey, if I understand correctly.
>
>
> (3) I think that the Tcg2ReadUserKey() function should take the timeout
> parameter, and the PCD should be consumed in the Tcg2UserConfirm()
> function instead. Tcg2UserConfirm() should pass the value of the PCD to
> Tcg2ReadUserKey().
>
> The PCD is called "PcdPhysicalPresenceUserConfirmTimeout", therefore it
> seems more closely tied to the Tcg2UserConfirm() function, in purpose.
> The Tcg2ReadUserKey() function seems to be a general utility function, so it
> should take a parameter, rather than fetch a particular PCD.
>
>
> (4) The comment block on the Tcg2ReadUserKey() function should be
> updated.
Thanks for the suggestion. It may make sense to adjust the read key logic.
Let me work out the patch and do some test of it. The function is internal, it is fine to change it and won't affect any other interface.
>
>
> (5) Please make sure that you format the patches next time with the
> "--stat=1000 --stat-graph-width=20" options. The pathnames of the files that
> are modified by this patch set are very long, and they are truncated in the
> diffstats. That way, the diffstat is not helpful.
>
> The "BaseTools/Scripts/SetupGit.py" script can automate at least the "--stat-
> graph-width=20" option for you.
>
Fine. Will do that.
>
> (6) When posting several patches, or large patches, it is helpful for reviewers
> if you also push your topic branch to your local repo, and expose the
> corresponding URL / branch name in the cover letter. Some patches are
> easier to review locally (after a git-fetch from your repo).
Thanks for the suggestion. Would follow that if I work on a big patch.
>
>
> (7) I don't know what happened, but I can see only patch#0 and patch#6
> from this series in my list folder. There are multiple lib instances being
> modified, and I couldn't compare the changes between those.
I would separate the patch into simple ones. The user confirm change is independent and it would be a simple patch later.
Thanks,
Zhichao
>
> Thanks
> Laszlo
>
>
> > diff --git
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.inf
> >
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.inf
> > index 85ce0e2b29..701de1836c 100644
> > ---
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> eL
> > ib.inf
> > +++
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPrese
> > +++ nceLib.inf
> > @@ -20,7 +20,7 @@
> > # This external input must be validated carefully to avoid security issue.
> > #
> > # Copyright (C) 2018, Red Hat, Inc.
> > -# Copyright (c) 2013 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +# Copyright (c) 2013 - 2020, Intel Corporation. All rights
> > +reserved.<BR>
> > # SPDX-License-Identifier: BSD-2-Clause-Patent # ## @@ -62,10
> > +62,14 @@
> > UefiBootServicesTableLib
> > UefiLib
> > UefiRuntimeServicesTableLib
> > + TimerLib
> >
> > [Protocols]
> > gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
> >
> > +[Pcd]
> > +
> gEfiSecurityPkgTokenSpaceGuid.PcdPhysicalPresenceUserConfirmTimeout
> > +
> > [Guids]
> > ## SOMETIMES_CONSUMES ## HII
> > gEfiTcg2PhysicalPresenceGuid
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
2020-01-03 14:21 ` [edk2-devel] " Laszlo Ersek
2020-01-15 8:03 ` Gao, Zhichao
@ 2020-01-19 7:03 ` Gao, Zhichao
2020-01-20 8:06 ` Laszlo Ersek
1 sibling, 1 reply; 26+ messages in thread
From: Gao, Zhichao @ 2020-01-19 7:03 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: Yao, Jiewen, Wang, Jian J, Zhang, Chao B, Justen, Jordan L,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
Hi Laszlo,
Sorry for misunderstanding the logic in read key function. That makes the incorrect analysis.
The read key function is waiting for "specific" key press not a key press. We should drop all the other pressing-key.
That means we should create a hotkey service like BDS one. It is not simple. I think that is why the original use the busy loop.
I don't think it makes sense to port the function from BDS for such a simple function. If there is a requirement for the hot key service to be public and common, it makes sense.
So I prefer to keep the original busy loop instead of porting hot key service.
Thanks,
Zhichao
> -----Original Message-----
> From: Gao, Zhichao
> Sent: Wednesday, January 15, 2020 4:03 PM
> To: 'Laszlo Ersek' <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Marc-André Lureau
> <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.ibm.com>
> Subject: RE: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib:
> Use pcd for user response wait time
>
> Hi Laszlo,
>
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Friday, January 3, 2020 10:21 PM
> > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>;
> > Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>; Marc-André Lureau
> > <marcandre.lureau@redhat.com>; Stefan Berger
> <stefanb@linux.ibm.com>
> > Subject: Re: [edk2-devel] [PATCH 06/13]
> OvmfPkg/Tcg2PhysicalPresenceLib:
> > Use pcd for user response wait time
> >
> > Hello Zhichao,
> >
> > On 01/03/20 04:04, Gao, Zhichao wrote:
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2443
> > >
> > > Use the pcd PcdPhysicalPresenceUserConfirmTimeout to control the
> > > wait time of user response.
> > >
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > > ---
> > > .../DxeTcg2PhysicalPresenceLib.c | 63 ++++++++++++++-----
> > > .../DxeTcg2PhysicalPresenceLib.inf | 6 +-
> > > 2 files changed, 52 insertions(+), 17 deletions(-)
> > >
> > > diff --git
> > >
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> > eL
> > > ib.c
> > >
> >
> b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> > eL
> > > ib.c
> > > index 00d76ba2c2..13f78cbfac 100644
> > > ---
> > >
> >
> a/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenc
> > eL
> > > ib.c
> > > +++
> > b/OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPrese
> > > +++ nceLib.c
> > > @@ -9,7 +9,7 @@
> > >
> > > Copyright (C) 2018, Red Hat, Inc.
> > > Copyright (c) 2018, IBM Corporation. All rights reserved.<BR>
> > > -Copyright (c) 2013 - 2016, Intel Corporation. All rights
> > > reserved.<BR>
> > > +Copyright (c) 2013 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > > SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > **/
> > > @@ -32,6 +32,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > #include <Library/UefiBootServicesTableLib.h>
> > > #include <Library/UefiLib.h>
> > > #include <Library/UefiRuntimeServicesTableLib.h>
> > > +#include <Library/TimerLib.h>
> > > +#include <Library/PcdLib.h>
> > >
> > > #include <Library/Tcg2PhysicalPresenceLib.h>
> > >
> > > @@ -365,28 +367,57 @@ Tcg2ReadUserKey ( {
> > > EFI_STATUS Status;
> > > EFI_INPUT_KEY Key;
> > > - UINT16 InputKey;
> > > + UINT16 ConfirmKey;
> > > + UINTN Interval;
> > > + INT64 Timeout;
> > >
> > > - InputKey = 0;
> > > + //
> > > + // delay 100 milli-second
> > > + //
> > > + Interval = 100;
> > > + ConfirmKey = (CautionKey) ? SCAN_F12 : SCAN_F10;
> > > + Timeout = (INT64)PcdGet32
> > (PcdPhysicalPresenceUserConfirmTimeout);
> > > + if (Timeout > 0) {
> > > + Timeout = (INT64)MultU64x32 ((UINT64)Timeout, 1000);
> > > + } else {
> > > + //
> > > + // Wait forever
> > > + //
> > > + Timeout = MAX_INT64;
> > > + }
> > > +
> > > + //
> > > + // Wait for user response within the time-out //
> > > do {
> > > + MicroSecondDelay (Interval * 1000);
> > > +
> > > Status = gBS->CheckEvent (gST->ConIn->WaitForKey);
> > > if (!EFI_ERROR (Status)) {
> > > Status = gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
> > > - if (Key.ScanCode == SCAN_ESC) {
> > > - InputKey = Key.ScanCode;
> > > - }
> > > - if ((Key.ScanCode == SCAN_F10) && !CautionKey) {
> > > - InputKey = Key.ScanCode;
> > > - }
> > > - if ((Key.ScanCode == SCAN_F12) && CautionKey) {
> > > - InputKey = Key.ScanCode;
> > > + if (!EFI_ERROR (Status)) {
> > > + if (Key.ScanCode == ConfirmKey) {
> > > + //
> > > + // User Confirmation
> > > + //
> > > + return TRUE;
> > > + }
> > > +
> > > + if (Key.ScanCode == SCAN_ESC) {
> > > + //
> > > + // User Rejection
> > > + //
> > > + return FALSE;
> > > + }
> > > + } else if (Status == EFI_DEVICE_ERROR) {
> > > + //
> > > + // If error, assume User Rejection
> > > + //
> > > + return FALSE;
> > > }
> > > }
> > > - } while (InputKey == 0);
> > > -
> > > - if (InputKey != SCAN_ESC) {
> > > - return TRUE;
> > > - }
> > > + Timeout -= Interval;
> > > + } while (Timeout > 0);
> > >
> > > return FALSE;
> > > }
> >
> > (1) I don't understand why the original (pre-patch) code uses
> > CheckEvent() in a busy loop. WaitForEvent() looks like a better (more
> > resource-conservative) option.
> >
> > Does the original code use CheckEvent() because WaitForEvent() is
> > restricted to TPL_APPLICATION?
> >
> > I don't think that being restricted to TPL_APPLICATION should be a
> problem.
> > As far as I can tell, the only call path to Tcg2ReadUserKey() is:
> >
> > Tcg2PhysicalPresenceLibProcessRequest()
> > Tcg2ExecutePendingTpmRequest()
> > Tcg2UserConfirm()
> > Tcg2ReadUserKey()
> >
> > In turn, Tcg2PhysicalPresenceLibProcessRequest() is supposed to be
> > called from PlatformBootManagerLib, which in turn is called from BdsDxe.
> > Therefore I think we can safely assume TPL_APPLICATION.
> >
> > I think we should add a separate patch first, for rewriting the
> > present logic of
> > Tcg2ReadUserKey() with WaitForEvent() -- remove the busy loop.
> >
> >
> > (2) Once we use WaitForEvent(), it is easier and more elegant to use a
> > timer event, in addition to "gST->ConIn->WaitForKey", to limit the
> > waiting for a keypress.
> >
> > For example, the BdsWaitForSingleEvent() function in
> > "MdeModulePkg/Universal/BdsDxe/BdsEntry.c" is used for implementing
> a
> > timed wait for a hotkey, if I understand correctly.
> >
> >
> > (3) I think that the Tcg2ReadUserKey() function should take the
> > timeout parameter, and the PCD should be consumed in the
> > Tcg2UserConfirm() function instead. Tcg2UserConfirm() should pass the
> > value of the PCD to Tcg2ReadUserKey().
> >
> > The PCD is called "PcdPhysicalPresenceUserConfirmTimeout", therefore
> > it seems more closely tied to the Tcg2UserConfirm() function, in purpose.
> > The Tcg2ReadUserKey() function seems to be a general utility function,
> > so it should take a parameter, rather than fetch a particular PCD.
> >
> >
> > (4) The comment block on the Tcg2ReadUserKey() function should be
> > updated.
>
> Thanks for the suggestion. It may make sense to adjust the read key logic.
> Let me work out the patch and do some test of it. The function is internal, it is
> fine to change it and won't affect any other interface.
>
> >
> >
> > (5) Please make sure that you format the patches next time with the
> > "--stat=1000 --stat-graph-width=20" options. The pathnames of the
> > files that are modified by this patch set are very long, and they are
> > truncated in the diffstats. That way, the diffstat is not helpful.
> >
> > The "BaseTools/Scripts/SetupGit.py" script can automate at least the
> > "--stat- graph-width=20" option for you.
> >
>
> Fine. Will do that.
>
> >
> > (6) When posting several patches, or large patches, it is helpful for
> > reviewers if you also push your topic branch to your local repo, and
> > expose the corresponding URL / branch name in the cover letter. Some
> > patches are easier to review locally (after a git-fetch from your repo).
>
> Thanks for the suggestion. Would follow that if I work on a big patch.
>
> >
> >
> > (7) I don't know what happened, but I can see only patch#0 and patch#6
> > from this series in my list folder. There are multiple lib instances
> > being modified, and I couldn't compare the changes between those.
>
> I would separate the patch into simple ones. The user confirm change is
> independent and it would be a simple patch later.
>
> Thanks,
> Zhichao
>
> >
> > Thanks
> > Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use pcd for user response wait time
2020-01-19 7:03 ` Gao, Zhichao
@ 2020-01-20 8:06 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2020-01-20 8:06 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Yao, Jiewen, Wang, Jian J, Zhang, Chao B, Justen, Jordan L,
Ard Biesheuvel, Marc-André Lureau, Stefan Berger
On 01/19/20 08:03, Gao, Zhichao wrote:
> Hi Laszlo,
>
> Sorry for misunderstanding the logic in read key function. That makes the incorrect analysis.
> The read key function is waiting for "specific" key press not a key press. We should drop all the other pressing-key.
> That means we should create a hotkey service like BDS one. It is not simple. I think that is why the original use the busy loop.
> I don't think it makes sense to port the function from BDS for such a simple function. If there is a requirement for the hot key service to be public and common, it makes sense.
> So I prefer to keep the original busy loop instead of porting hot key service.
I don't understand why the fact that we're interested in a specific key
only, changes *how* we should wait for keys. In both approaches (wait
for event, vs. busy wait), we should consume and drop keys until we find
what we need. Yes, the wait-for-event approach needs another loop around
it, but that still doesn't make it a "busy" loop -- it only runs when
there's an event to process (keypress or timeout). This is a standard
event loop mechanism; block/sleep in the loop body until something
occurs, then process the event; lather, rinse, repeat.
Anyway, I don't feel strongly about this. If your original approach is
accepted for SecurityPkg, I can live with it in OvmfPkg too.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-01-20 8:06 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-03 3:04 [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Gao, Zhichao
2020-01-03 3:04 ` [PATCH 01/13] SecurityPkg/Tcg2PpVerndorLib: Add two Ex function to handle PPdata Gao, Zhichao
2020-01-03 3:04 ` [PATCH 02/13] SecurityPkg/Tcg2PpVendorLib: Add implementation of new Ex function Gao, Zhichao
2020-01-03 3:04 ` [PATCH 03/13] SecurityPkg/Tcg2PhysicalPresenceLib: Use the " Gao, Zhichao
2020-01-03 3:04 ` [PATCH 04/13] SecurityPkg/SmmTcg2PhysicalPresenceLib: " Gao, Zhichao
2020-01-03 3:04 ` [PATCH 05/13] SecurityPkg/dec: Add a pcd for user response wait time Gao, Zhichao
2020-01-03 3:04 ` [PATCH 06/13] OvmfPkg/Tcg2PhysicalPresenceLib: Use " Gao, Zhichao
2020-01-03 14:21 ` [edk2-devel] " Laszlo Ersek
2020-01-15 8:03 ` Gao, Zhichao
2020-01-19 7:03 ` Gao, Zhichao
2020-01-20 8:06 ` Laszlo Ersek
2020-01-03 3:04 ` [PATCH 07/13] SecurityPkg/Tcg2PhysicalPresenceLib: Use Pcd for user resp " Gao, Zhichao
2020-01-03 3:04 ` [PATCH 08/13] SecurityPkg/TcgPyhsicalPresenceLib: " Gao, Zhichao
2020-01-03 3:04 ` [PATCH 09/13] SecurityPkg/Tcg2PhysicalPresenceData.h: Add FunctionIndex for PPdata Gao, Zhichao
2020-01-03 3:04 ` [PATCH 10/13] SecurityPkg/Tcg2PhysicalPresenceLib: Extend the submit preOS func Gao, Zhichao
2020-01-03 3:04 ` [PATCH 11/13] SecurityPkg: Move the Tcg2ConfigNvData.h to Include folder Gao, Zhichao
2020-01-03 3:04 ` [PATCH 12/13] SecurityPkg/TcgPhysicalPresenceLib: Replace the ASSERT with error code Gao, Zhichao
2020-01-03 3:04 ` [PATCH 13/13] SecurityPkg/TcgPhysicalPresenceLib: Fix the operation of 11 Gao, Zhichao
2020-01-03 3:09 ` [PATCH 00/13] Extend and fix the TCG/TCG2 Physical Presence Interface Yao, Jiewen
2020-01-03 5:07 ` Gao, Zhichao
2020-01-03 5:30 ` Yao, Jiewen
2020-01-09 9:05 ` Gao, Zhichao
2020-01-09 9:22 ` Yao, Jiewen
[not found] ` <15E649625DE7E06B.2038@groups.io>
2020-01-03 5:59 ` [edk2-devel] " Yao, Jiewen
2020-01-07 8:05 ` Gao, Zhichao
2020-01-07 8:31 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox