Change in coreboot[master]: security/tpm: Use correct hash digest lengths

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35287 )

Change subject: security/tpm: Use correct hash digest lengths
......................................................................

security/tpm: Use correct hash digest lengths

TPMU_HA is a union of all the different hash digests, and so
sizeof(TPMU_HA) evaluates to 64 (the size of the largest one). This will
lead to out-of-bounds writes when copying smaller digests, so use the
specific digest size for each algorithm.

Change-Id: Ic9101f157d5a19836b200ecd99f060de552498d2
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 14049{49,50,51,52,53,54,55,56,57,58,60,61,62}
---
M src/security/tpm/tspi/tspi.c
M src/security/tpm/tss/tcg-2.0/tss.c
2 files changed, 7 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/35287/1

diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c
index 4cf3711..e64e04f 100644
--- a/src/security/tpm/tspi/tspi.c
+++ b/src/security/tpm/tspi/tspi.c
@@ -219,12 +219,12 @@
    case VB2_HASH_SHA1:
    	tpml_digests.digests[0].hashAlg = TPM_ALG_SHA1;
    	memcpy(tpml_digests.digests[0].digest.sha1,
-		       digest, sizeof(TPMU_HA));
+		       digest, SHA1_DIGEST_SIZE);
    	break;
    case VB2_HASH_SHA256:
    	tpml_digests.digests[0].hashAlg = TPM_ALG_SHA256;
    	memcpy(tpml_digests.digests[0].digest.sha256,
-		       digest, sizeof(TPMU_HA));
+		       digest, SHA256_DIGEST_SIZE);
    	break;
    default:
    	return TPM_E_IOERROR;
diff --git a/src/security/tpm/tss/tcg-2.0/tss.c b/src/security/tpm/tss/tcg-2.0/tss.c
index fab334f..d9deba5 100644
--- a/src/security/tpm/tss/tcg-2.0/tss.c
+++ b/src/security/tpm/tss/tcg-2.0/tss.c
@@ -148,27 +148,27 @@
    	case TPM_ALG_SHA1:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sha1,
    		       tpml_digests->digests[i].digest.sha1,
-			       sizeof(TPMU_HA));
+			       SHA1_DIGEST_SIZE);
    		break;
    	case TPM_ALG_SHA256:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sha256,
    		       tpml_digests->digests[i].digest.sha256,
-			       sizeof(TPMU_HA));
+			       SHA256_DIGEST_SIZE);
    		break;
    	case TPM_ALG_SHA384:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sha384,
    		       tpml_digests->digests[i].digest.sha384,
-			       sizeof(TPMU_HA));
+			       SHA384_DIGEST_SIZE);
    		break;
    	case TPM_ALG_SHA512:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sha512,
    		       tpml_digests->digests[i].digest.sha512,
-			       sizeof(TPMU_HA));
+			       SHA512_DIGEST_SIZE);
    		break;
    	case TPM_ALG_SM3_256:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sm3_256,
    		       tpml_digests->digests[i].digest.sm3_256,
-			       sizeof(TPMU_HA));
+			       SM3_256_DIGEST_SIZE);
    		break;
    	}
    }

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35287 )

Change subject: security/tpm: Use correct hash digest lengths
......................................................................

Patch Set 1: Code-Review+2

I guess we can take this as a quick fix, but this is far from everything that was wrong with that patch. Frans, please see my comments in CB:33252. (Or maybe it would be better just to revert it for now?)

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35287 )

Change subject: security/tpm: Use correct hash digest lengths
......................................................................

Patch Set 1: Code-Review+2

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35287 )

Change subject: security/tpm: Use correct hash digest lengths
......................................................................

Patch Set 1: Code-Review+2

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35287 )

Change subject: security/tpm: Use correct hash digest lengths
......................................................................

security/tpm: Use correct hash digest lengths

TPMU_HA is a union of all the different hash digests, and so
sizeof(TPMU_HA) evaluates to 64 (the size of the largest one). This will
lead to out-of-bounds writes when copying smaller digests, so use the
specific digest size for each algorithm.

Change-Id: Ic9101f157d5a19836b200ecd99f060de552498d2
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 14049{49,50,51,52,53,54,55,56,57,58,60,61,62}
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35287
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
Reviewed-by: Kyösti Mälkki 
Reviewed-by: Philipp Deppenwiese 
---
M src/security/tpm/tspi/tspi.c
M src/security/tpm/tss/tcg-2.0/tss.c
2 files changed, 7 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Kyösti Mälkki: Looks good to me, approved
  Philipp Deppenwiese: Looks good to me, approved
  Julius Werner: Looks good to me, approved

diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c
index 4cf3711..e64e04f 100644
--- a/src/security/tpm/tspi/tspi.c
+++ b/src/security/tpm/tspi/tspi.c
@@ -219,12 +219,12 @@
    case VB2_HASH_SHA1:
    	tpml_digests.digests[0].hashAlg = TPM_ALG_SHA1;
    	memcpy(tpml_digests.digests[0].digest.sha1,
-		       digest, sizeof(TPMU_HA));
+		       digest, SHA1_DIGEST_SIZE);
    	break;
    case VB2_HASH_SHA256:
    	tpml_digests.digests[0].hashAlg = TPM_ALG_SHA256;
    	memcpy(tpml_digests.digests[0].digest.sha256,
-		       digest, sizeof(TPMU_HA));
+		       digest, SHA256_DIGEST_SIZE);
    	break;
    default:
    	return TPM_E_IOERROR;
diff --git a/src/security/tpm/tss/tcg-2.0/tss.c b/src/security/tpm/tss/tcg-2.0/tss.c
index fab334f..d9deba5 100644
--- a/src/security/tpm/tss/tcg-2.0/tss.c
+++ b/src/security/tpm/tss/tcg-2.0/tss.c
@@ -148,27 +148,27 @@
    	case TPM_ALG_SHA1:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sha1,
    		       tpml_digests->digests[i].digest.sha1,
-			       sizeof(TPMU_HA));
+			       SHA1_DIGEST_SIZE);
    		break;
    	case TPM_ALG_SHA256:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sha256,
    		       tpml_digests->digests[i].digest.sha256,
-			       sizeof(TPMU_HA));
+			       SHA256_DIGEST_SIZE);
    		break;
    	case TPM_ALG_SHA384:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sha384,
    		       tpml_digests->digests[i].digest.sha384,
-			       sizeof(TPMU_HA));
+			       SHA384_DIGEST_SIZE);
    		break;
    	case TPM_ALG_SHA512:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sha512,
    		       tpml_digests->digests[i].digest.sha512,
-			       sizeof(TPMU_HA));
+			       SHA512_DIGEST_SIZE);
    		break;
    	case TPM_ALG_SM3_256:
    		memcpy(pcr_ext_cmd.digests.digests[i].digest.sm3_256,
    		       tpml_digests->digests[i].digest.sm3_256,
-			       sizeof(TPMU_HA));
+			       SM3_256_DIGEST_SIZE);
    		break;
    	}
    }