SecurityPkg: TPM must go to Idle state on CRB command completion.

[edk2-devel] [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Posted by Rodrigo Gonzalez del Cueto 2 years, 9 months ago

To follow the TCG CRB protocol specification, on every CRB TPM command
completion the TPM should return to Idle state, regardless of the
CRB Idle Bypass capability reported by the TPM device.

See: TCG PC Client Device Driver Design Principles for TPM 2.0,
Version 1.0, Rev 0.27

Signed-off-by: Rodrigo Gonzalez del Cueto <rodrigo.gonzalez.del.cueto@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
 SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
index f1f8091683..34e3874a5b 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
@@ -310,7 +310,7 @@ PtpCrbTpmCommand (
     // Command completed, but buffer is not enough
     //
     Status = EFI_BUFFER_TOO_SMALL;
-    goto GoReady_Exit;
+    goto GoIdle_Exit;
   }
   *SizeOut = TpmOutSize;
   //
@@ -328,16 +328,6 @@ PtpCrbTpmCommand (
     DEBUG ((EFI_D_VERBOSE, "\n"));
   );
 
-GoReady_Exit:
-  //
-  // Goto Ready State if command is completed successfully and TPM support IdleBypass
-  // If not supported. flow down to GoIdle
-  //
-  if (GetCachedIdleByPass () == 1) {
-    MmioWrite32((UINTN)&CrbReg->CrbControlRequest, PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
-    return Status;
-  }
-
   //
   // Do not wait for state transition for TIMEOUT_C
   // This function will try to wait 2 TIMEOUT_C at the beginning in next call.
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77841): https://edk2.groups.io/g/devel/message/77841
Mute This Topic: https://groups.io/mt/84258803/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Posted by Rodrigo Gonzalez del Cueto 2 years, 8 months ago

Missed adding the Bugzilla reference to the patch.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3463


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78448): https://edk2.groups.io/g/devel/message/78448
Mute This Topic: https://groups.io/mt/84258803/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Posted by Yao, Jiewen 2 years, 8 months ago

Would you please tell us how many TPM2 chip you have tested?

I think we need consider the compatibility of exiting TPM2 chips, to make sure the code still work.


Thank you
Yao Jiewen

> -----Original Message-----
> From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
> Sent: Saturday, July 17, 2021 5:18 AM
> To: devel@edk2.groups.io
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command
> completion.
> 
> To follow the TCG CRB protocol specification, on every CRB TPM command
> completion the TPM should return to Idle state, regardless of the
> CRB Idle Bypass capability reported by the TPM device.
> 
> See: TCG PC Client Device Driver Design Principles for TPM 2.0,
> Version 1.0, Rev 0.27
> 
> Signed-off-by: Rodrigo Gonzalez del Cueto
> <rodrigo.gonzalez.del.cueto@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> index f1f8091683..34e3874a5b 100644
> --- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> +++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> @@ -310,7 +310,7 @@ PtpCrbTpmCommand (
>      // Command completed, but buffer is not enough
>      //
>      Status = EFI_BUFFER_TOO_SMALL;
> -    goto GoReady_Exit;
> +    goto GoIdle_Exit;
>    }
>    *SizeOut = TpmOutSize;
>    //
> @@ -328,16 +328,6 @@ PtpCrbTpmCommand (
>      DEBUG ((EFI_D_VERBOSE, "\n"));
>    );
> 
> -GoReady_Exit:
> -  //
> -  // Goto Ready State if command is completed successfully and TPM support
> IdleBypass
> -  // If not supported. flow down to GoIdle
> -  //
> -  if (GetCachedIdleByPass () == 1) {
> -    MmioWrite32((UINTN)&CrbReg->CrbControlRequest,
> PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
> -    return Status;
> -  }
> -
>    //
>    // Do not wait for state transition for TIMEOUT_C
>    // This function will try to wait 2 TIMEOUT_C at the beginning in next call.
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78912): https://edk2.groups.io/g/devel/message/78912
Mute This Topic: https://groups.io/mt/84258803/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Posted by Rodrigo Gonzalez del Cueto 2 years, 5 months ago

Hi Jiewen,

I have tested the proposed CRB protocol fix with three different TPM configurations I have available which support the CRB interface: Intel PTT, STMicro and Nuvoton. Under these CRB configurations I didn't observe any issues arising from the proposed change aligning with the TCG CRB protocol definition.

I verified the BIOS flows were unaffected and completed without errors and that the OS was still able to interact with the TPM.

Regards,
-Rodrigo
________________________________
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Sunday, August 8, 2021 6:27 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Wang, Jian J <jian.j.wang@intel.com>
Subject: RE: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Would you please tell us how many TPM2 chip you have tested?

I think we need consider the compatibility of exiting TPM2 chips, to make sure the code still work.


Thank you
Yao Jiewen

> -----Original Message-----
> From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
> Sent: Saturday, July 17, 2021 5:18 AM
> To: devel@edk2.groups.io
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command
> completion.
>
> To follow the TCG CRB protocol specification, on every CRB TPM command
> completion the TPM should return to Idle state, regardless of the
> CRB Idle Bypass capability reported by the TPM device.
>
> See: TCG PC Client Device Driver Design Principles for TPM 2.0,
> Version 1.0, Rev 0.27
>
> Signed-off-by: Rodrigo Gonzalez del Cueto
> <rodrigo.gonzalez.del.cueto@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> index f1f8091683..34e3874a5b 100644
> --- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> +++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> @@ -310,7 +310,7 @@ PtpCrbTpmCommand (
>      // Command completed, but buffer is not enough
>      //
>      Status = EFI_BUFFER_TOO_SMALL;
> -    goto GoReady_Exit;
> +    goto GoIdle_Exit;
>    }
>    *SizeOut = TpmOutSize;
>    //
> @@ -328,16 +328,6 @@ PtpCrbTpmCommand (
>      DEBUG ((EFI_D_VERBOSE, "\n"));
>    );
>
> -GoReady_Exit:
> -  //
> -  // Goto Ready State if command is completed successfully and TPM support
> IdleBypass
> -  // If not supported. flow down to GoIdle
> -  //
> -  if (GetCachedIdleByPass () == 1) {
> -    MmioWrite32((UINTN)&CrbReg->CrbControlRequest,
> PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
> -    return Status;
> -  }
> -
>    //
>    // Do not wait for state transition for TIMEOUT_C
>    // This function will try to wait 2 TIMEOUT_C at the beginning in next call.
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82944): https://edk2.groups.io/g/devel/message/82944
Mute This Topic: https://groups.io/mt/84258803/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Posted by Yao, Jiewen 2 years, 5 months ago

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com>
Sent: Saturday, October 30, 2021 5:34 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Cc: Wang, Jian J <jian.j.wang@intel.com>
Subject: Re: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Hi Jiewen,

I have tested the proposed CRB protocol fix with three different TPM configurations I have available which support the CRB interface: Intel PTT, STMicro and Nuvoton. Under these CRB configurations I didn't observe any issues arising from the proposed change aligning with the TCG CRB protocol definition.

I verified the BIOS flows were unaffected and completed without errors and that the OS was still able to interact with the TPM.

Regards,
-Rodrigo
________________________________
From: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Sent: Sunday, August 8, 2021 6:27 PM
To: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
Subject: RE: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command completion.

Would you please tell us how many TPM2 chip you have tested?

I think we need consider the compatibility of exiting TPM2 chips, to make sure the code still work.


Thank you
Yao Jiewen

> -----Original Message-----
> From: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
> Sent: Saturday, July 17, 2021 5:18 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Gonzalez Del Cueto, Rodrigo <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>;
> Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Subject: [PATCH] SecurityPkg: TPM must go to Idle state on CRB command
> completion.
>
> To follow the TCG CRB protocol specification, on every CRB TPM command
> completion the TPM should return to Idle state, regardless of the
> CRB Idle Bypass capability reported by the TPM device.
>
> See: TCG PC Client Device Driver Design Principles for TPM 2.0,
> Version 1.0, Rev 0.27
>
> Signed-off-by: Rodrigo Gonzalez del Cueto
> <rodrigo.gonzalez.del.cueto@intel.com<mailto:rodrigo.gonzalez.del.cueto@intel.com>>
> Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> ---
>  SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> index f1f8091683..34e3874a5b 100644
> --- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> +++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
> @@ -310,7 +310,7 @@ PtpCrbTpmCommand (
>      // Command completed, but buffer is not enough
>      //
>      Status = EFI_BUFFER_TOO_SMALL;
> -    goto GoReady_Exit;
> +    goto GoIdle_Exit;
>    }
>    *SizeOut = TpmOutSize;
>    //
> @@ -328,16 +328,6 @@ PtpCrbTpmCommand (
>      DEBUG ((EFI_D_VERBOSE, "\n"));
>    );
>
> -GoReady_Exit:
> -  //
> -  // Goto Ready State if command is completed successfully and TPM support
> IdleBypass
> -  // If not supported. flow down to GoIdle
> -  //
> -  if (GetCachedIdleByPass () == 1) {
> -    MmioWrite32((UINTN)&CrbReg->CrbControlRequest,
> PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
> -    return Status;
> -  }
> -
>    //
>    // Do not wait for state transition for TIMEOUT_C
>    // This function will try to wait 2 TIMEOUT_C at the beginning in next call.
> --
> 2.31.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83338): https://edk2.groups.io/g/devel/message/83338
Mute This Topic: https://groups.io/mt/84258803/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-