[Patch 0/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix TPM issues

[Patch 0/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix TPM issues

[Michael Kinney]

Fix several issues with the Atmel I2C TPM library in the QuarkPlatformPkg.
This includes GCC build failures, a corner case for an incorrect output
buffer size, and a TPM detection failure due to increased error checking
in the Tpm12CommandLib.

Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Lee Leahy <leroy.p.leahy@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>

Michael Kinney (3):
  QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Remove PP Check
  QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix SubmitCommand() out size
  QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix GCC build issues

 .../Library/Tpm12DeviceLibAtmelI2c/TisPc.c         | 31 +++++++++-------------
 .../Tpm12DeviceLibAtmelI2c.inf                     |  3 +--
 2 files changed, 14 insertions(+), 20 deletions(-)

-- 
2.6.3.windows.1

[Patch 1/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Remove PP Check

[Michael Kinney]

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

The Tpm12CommandLib in the SecurityPkg was updated to check
the returnCode in the response packet in the following
commit:

https://github.com/tianocore/edk2/commit/950a3bc788b5b101729b26aed3ff75fd2a64a570

The Tpm12DeviceLibAtmelI2C uses the Tpm12PhysicalPresence()
function from the Tpm12CommandLib to verify that the I2C device
is responding to TPM command packets.  However, at this point
the TPM device has not been started, and the TPM properly
responds with an error in the returnCode field of the response
packet.  Before the commit above, the check worked, but the
additional error checking in the commit above now returns an
error and an Atmel I2C is returned as not present.

The fix is to remove the call to the Tpm12PhysicalPresence()
API in the Tpm12CommandLib from the Tpm12RequestUseTpm() API.
The Atmel I2C TPM device is detected if a byte can be read
from the slave address assigned to the Atmel I2C device.
When the startup command is sent to the TPM later, the
command and response packet processing will be verified, and
if the I2C device is not an Atmel TPM, it will be detected
at that point.

Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Lee Leahy <leroy.p.leahy@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c      | 12 +-----------
 .../Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf        |  3 +--
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
index 4e5aa41..08a005f 100644
--- a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
+++ b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
@@ -1,7 +1,7 @@
 /** @file
   Basic TIS (TPM Interface Specification) functions for Atmel I2C TPM.
 
-  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
   which accompanies this distribution.  The full text of the license may be found at
@@ -18,7 +18,6 @@
 #include <Library/TimerLib.h>
 #include <Library/DebugLib.h>
 #include <Library/I2cLib.h>
-#include <Library/Tpm12CommandLib.h>
 
 //
 // Atmel I2C TPM slave address
@@ -224,15 +223,6 @@ Tpm12RequestUseTpm (
     }
   } while (EFI_ERROR (Status));
 
-  //
-  // Send Physical Presence Command to Atmel I2C TPM
-  //
-  Status = Tpm12PhysicalPresence (TPM_PHYSICAL_PRESENCE_PRESENT);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "Atmel I2C TPM failed to submit physical presence command: %r\n", Status));
-    return Status;
-  }
-
   return EFI_SUCCESS;
 }
 
diff --git a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf
index 5a8734f..1f7e4be 100644
--- a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf
+++ b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf
@@ -4,7 +4,7 @@
 #  This instance provides basic TPM Interface Specification (TIS) functions
 #  or Atmel I2C TPM.
 #
-# Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
 # This program and the accompanying materials
 # are licensed and made available under the terms and conditions of the BSD License
 # which accompanies this distribution. The full text of the license may be found at
@@ -42,4 +42,3 @@
   TimerLib
   DebugLib
   I2cLib
-  Tpm12CommandLib
-- 
2.6.3.windows.1

Re: [Patch 1/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Remove PP Check

[Leahy, Leroy P]

Reviewed-by: Lee Leahy <leroy.p.leahy@intel.com>

-----Original Message-----
From: Kinney, Michael D 
Sent: Tuesday, January 10, 2017 10:17 AM
To: edk2-devel@lists.01.org
Cc: Steele, Kelly <kelly.steele@intel.com>; Leahy, Leroy P <leroy.p.leahy@intel.com>
Subject: [Patch 1/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Remove PP Check

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

The Tpm12CommandLib in the SecurityPkg was updated to check the returnCode in the response packet in the following
commit:

https://github.com/tianocore/edk2/commit/950a3bc788b5b101729b26aed3ff75fd2a64a570

The Tpm12DeviceLibAtmelI2C uses the Tpm12PhysicalPresence() function from the Tpm12CommandLib to verify that the I2C device is responding to TPM command packets.  However, at this point the TPM device has not been started, and the TPM properly responds with an error in the returnCode field of the response packet.  Before the commit above, the check worked, but the additional error checking in the commit above now returns an error and an Atmel I2C is returned as not present.

The fix is to remove the call to the Tpm12PhysicalPresence() API in the Tpm12CommandLib from the Tpm12RequestUseTpm() API.
The Atmel I2C TPM device is detected if a byte can be read from the slave address assigned to the Atmel I2C device.
When the startup command is sent to the TPM later, the command and response packet processing will be verified, and if the I2C device is not an Atmel TPM, it will be detected at that point.

Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Lee Leahy <leroy.p.leahy@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c      | 12 +-----------
 .../Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf        |  3 +--
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
index 4e5aa41..08a005f 100644
--- a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
+++ b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
@@ -1,7 +1,7 @@
 /** @file
   Basic TIS (TPM Interface Specification) functions for Atmel I2C TPM.
 
-  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2017, Intel Corporation. All rights 
+ reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
   which accompanies this distribution.  The full text of the license may be found at @@ -18,7 +18,6 @@  #include <Library/TimerLib.h>  #include <Library/DebugLib.h>  #include <Library/I2cLib.h> -#include <Library/Tpm12CommandLib.h>
 
 //
 // Atmel I2C TPM slave address
@@ -224,15 +223,6 @@ Tpm12RequestUseTpm (
     }
   } while (EFI_ERROR (Status));
 
-  //
-  // Send Physical Presence Command to Atmel I2C TPM
-  //
-  Status = Tpm12PhysicalPresence (TPM_PHYSICAL_PRESENCE_PRESENT);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((EFI_D_ERROR, "Atmel I2C TPM failed to submit physical presence command: %r\n", Status));
-    return Status;
-  }
-
   return EFI_SUCCESS;
 }
 
diff --git a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf
index 5a8734f..1f7e4be 100644
--- a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtmelI2c.inf
+++ b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/Tpm12DeviceLibAtme
+++ lI2c.inf
@@ -4,7 +4,7 @@
 #  This instance provides basic TPM Interface Specification (TIS) functions  #  or Atmel I2C TPM.
 #
-# Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2016 - 2017, Intel Corporation. All rights 
+reserved.<BR>
 # This program and the accompanying materials  # are licensed and made available under the terms and conditions of the BSD License  # which accompanies this distribution. The full text of the license may be found at @@ -42,4 +42,3 @@
   TimerLib
   DebugLib
   I2cLib
-  Tpm12CommandLib
--
2.6.3.windows.1

[Patch 2/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix SubmitCommand() out size

[Michael Kinney]

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

When the Tpm12SubmitCommand() detects a response packet that
is the same size as a TPM_RSP_COMMAND_HDR, it returns
EFI_SUCCESS without reading any additional response packet
information from the TPM.  In that case, the return parameter
OutputParameterBlockSize is not be updated, so the size of
that OutputParameterBlock returned is the value passed in which
could be larger than what is actually returned from the TPM.

Set the OutputParameterBlockSize to the size of the
TPM_RSP_COMMAND_HDR when this specific condition is detected.

Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Lee Leahy <leroy.p.leahy@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
index 08a005f..3aab530 100644
--- a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
+++ b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
@@ -355,6 +355,7 @@ Tpm12SubmitCommand (
 
   TpmOutSize = SwapBytes32 (ReadUnaligned32 (&ResponseHeader->paramSize));
   if (TpmOutSize == sizeof (TPM_RSP_COMMAND_HDR)) {
+    *OutputParameterBlockSize = TpmOutSize;
     Status = EFI_SUCCESS;
     goto Done;
   }
-- 
2.6.3.windows.1

Re: [Patch 2/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix SubmitCommand() out size

[Leahy, Leroy P]

Reviewed-by: Lee Leahy <leroy.p.leahy@intel.com>

-----Original Message-----
From: Kinney, Michael D 
Sent: Tuesday, January 10, 2017 10:17 AM
To: edk2-devel@lists.01.org
Cc: Steele, Kelly <kelly.steele@intel.com>; Leahy, Leroy P <leroy.p.leahy@intel.com>
Subject: [Patch 2/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix SubmitCommand() out size

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

When the Tpm12SubmitCommand() detects a response packet that is the same size as a TPM_RSP_COMMAND_HDR, it returns EFI_SUCCESS without reading any additional response packet information from the TPM.  In that case, the return parameter OutputParameterBlockSize is not be updated, so the size of that OutputParameterBlock returned is the value passed in which could be larger than what is actually returned from the TPM.

Set the OutputParameterBlockSize to the size of the TPM_RSP_COMMAND_HDR when this specific condition is detected.

Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Lee Leahy <leroy.p.leahy@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
index 08a005f..3aab530 100644
--- a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
+++ b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
@@ -355,6 +355,7 @@ Tpm12SubmitCommand (
 
   TpmOutSize = SwapBytes32 (ReadUnaligned32 (&ResponseHeader->paramSize));
   if (TpmOutSize == sizeof (TPM_RSP_COMMAND_HDR)) {
+    *OutputParameterBlockSize = TpmOutSize;
     Status = EFI_SUCCESS;
     goto Done;
   }
--
2.6.3.windows.1

[Patch 3/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix GCC build issues

[Michael Kinney]

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

Fix build issues with GCC49.  There are local variables that
may be used before initialized in some paths.

Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Lee Leahy <leroy.p.leahy@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 .../Library/Tpm12DeviceLibAtmelI2c/TisPc.c             | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
index 3aab530..894e1e3 100644
--- a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
+++ b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
@@ -59,6 +59,8 @@ WriteTpmBufferMultiple (
   UINTN                   Index;
   UINTN                   PartialLength;
 
+  Status = EFI_SUCCESS;
+
   I2CDeviceAddr.I2CDeviceAddress = ATMEL_I2C_TPM_SLAVE_ADDRESS;
 
   DEBUG ((EFI_D_VERBOSE, "WriteTpmBufferMultiple: Addr=%02x  Length=%02x\n", I2CDeviceAddr.I2CDeviceAddress, Length));
@@ -112,6 +114,8 @@ ReadTpmBufferMultiple (
   UINTN                   Index;
   UINTN                   PartialLength;
 
+  Status = EFI_SUCCESS;
+
   I2CDeviceAddr.I2CDeviceAddress = ATMEL_I2C_TPM_SLAVE_ADDRESS;
   WriteLength = 0;
 
@@ -263,6 +267,13 @@ Tpm12SubmitCommand (
   INT64                Delta;
 
   //
+  // Initialize local variables
+  //
+  Start   = 0;
+  End     = 0;
+  Total   = 0;
+
+  //
   // Make sure response buffer is big enough to hold a response header
   //
   if (*OutputParameterBlockSize < sizeof (TPM_RSP_COMMAND_HDR)) {
@@ -276,13 +287,6 @@ Tpm12SubmitCommand (
   Current = GetPerformanceCounter();
 
   //
-  // Initialize local variables
-  //
-  Start = 0;
-  End   = 0;
-  Total = 0;
-
-  //
   // Retrieve the performance counter properties and compute the number of
   // performance counter ticks required to reach the maximum TIS timeout of
   // TIS_TIMEOUT_A.  TIS_TIMEOUT_A is in microseconds.
-- 
2.6.3.windows.1

Re: [Patch 3/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix GCC build issues

[Leahy, Leroy P]

Reviewed-by: Lee Leahy <leroy.p.leahy@intel.com>

-----Original Message-----
From: Kinney, Michael D 
Sent: Tuesday, January 10, 2017 10:17 AM
To: edk2-devel@lists.01.org
Cc: Steele, Kelly <kelly.steele@intel.com>; Leahy, Leroy P <leroy.p.leahy@intel.com>
Subject: [Patch 3/3] QuarkPlatformPkg/Tpm12DeviceLibAtmelI2c: Fix GCC build issues

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

Fix build issues with GCC49.  There are local variables that may be used before initialized in some paths.

Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Lee Leahy <leroy.p.leahy@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
 .../Library/Tpm12DeviceLibAtmelI2c/TisPc.c             | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
index 3aab530..894e1e3 100644
--- a/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
+++ b/QuarkPlatformPkg/Library/Tpm12DeviceLibAtmelI2c/TisPc.c
@@ -59,6 +59,8 @@ WriteTpmBufferMultiple (
   UINTN                   Index;
   UINTN                   PartialLength;
 
+  Status = EFI_SUCCESS;
+
   I2CDeviceAddr.I2CDeviceAddress = ATMEL_I2C_TPM_SLAVE_ADDRESS;
 
   DEBUG ((EFI_D_VERBOSE, "WriteTpmBufferMultiple: Addr=%02x  Length=%02x\n", I2CDeviceAddr.I2CDeviceAddress, Length)); @@ -112,6 +114,8 @@ ReadTpmBufferMultiple (
   UINTN                   Index;
   UINTN                   PartialLength;
 
+  Status = EFI_SUCCESS;
+
   I2CDeviceAddr.I2CDeviceAddress = ATMEL_I2C_TPM_SLAVE_ADDRESS;
   WriteLength = 0;
 
@@ -263,6 +267,13 @@ Tpm12SubmitCommand (
   INT64                Delta;
 
   //
+  // Initialize local variables
+  //
+  Start   = 0;
+  End     = 0;
+  Total   = 0;
+
+  //
   // Make sure response buffer is big enough to hold a response header
   //
   if (*OutputParameterBlockSize < sizeof (TPM_RSP_COMMAND_HDR)) { @@ -276,13 +287,6 @@ Tpm12SubmitCommand (
   Current = GetPerformanceCounter();
 
   //
-  // Initialize local variables
-  //
-  Start = 0;
-  End   = 0;
-  Total = 0;
-
-  //
   // Retrieve the performance counter properties and compute the number of
   // performance counter ticks required to reach the maximum TIS timeout of
   // TIS_TIMEOUT_A.  TIS_TIMEOUT_A is in microseconds.
--
2.6.3.windows.1