Change in coreboot[master]: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

security/intel/txt/ramstage.c: Fix clearing secrets on CBNT

With CBNT it looks like only the the TXT_E2STS_SECRET_STS should be
looked at.

Change-Id: Iff4436501b84f5c209add845b3cd3a62782d17e6
Signed-off-by: Arthur Heymans 
---
M src/security/intel/txt/ramstage.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/47934/1

diff --git a/src/security/intel/txt/ramstage.c b/src/security/intel/txt/ramstage.c
index 80bf3f9..4e38d32 100644
--- a/src/security/intel/txt/ramstage.c
+++ b/src/security/intel/txt/ramstage.c
@@ -90,7 +90,7 @@
    	return;

/* Check for fatal ACM error and TXT reset */
-	if (get_wake_error_status()) {
+//	if (get_wake_error_status()) {
    	/*
    	 * Check if secrets bit needs to be reset. Only platforms that support
    	 * CONFIG(PLATFORM_HAS_DRAM_CLEAR) will be able to run this code.
@@ -106,7 +106,7 @@
    		intel_txt_log_acm_error(read32((void *)TXT_BIOSACM_ERRORCODE));
    		die("Waiting for platform reset...\n");
    	}
-	}
+//	}
 }

BOOT_STATE_INIT_ENTRY(BS_POST_DEVICE, BS_ON_ENTRY, check_secrets_txt, NULL);

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Patch Set 1:

This change is ready for review.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Patch Set 1:

This change is ready for review.

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Set Ready For Review

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Set Ready For Review

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47934/6/src/security/intel/txt/rams... 
File src/security/intel/txt/ramstage.c:

https://review.coreboot.org/c/coreboot/+/47934/6/src/security/intel/txt/rams... 
PS6, Line 95: 	 * Assume all memory really was cleared.
It would be good to add that "Typically the memory was cleared by FSP-M".

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Jonathan Zhang, Johnny Lin, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47934

to look at the new patch set (#7).

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

security/intel/txt/ramstage.c: Fix clearing secrets on CBNT

intel_txt_memory_has_secret() checks for ESTS.TXT_ESTS_WAKE_ERROR_STS
|| E2STS.TXT_E2STS_SECRET_STS and it looks like with CBNT the E2STS
bit can be set without the ESTS bit.

Change-Id: Iff4436501b84f5c209add845b3cd3a62782d17e6
Signed-off-by: Arthur Heymans 
---
M src/security/intel/txt/ramstage.c
1 file changed, 14 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/47934/7

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47934/6/src/security/intel/txt/rams... 
File src/security/intel/txt/ramstage.c:

https://review.coreboot.org/c/coreboot/+/47934/6/src/security/intel/txt/rams... 
PS6, Line 95: 	 * Assume all memory really was cleared.
> It would be good to add that "Typically the memory was cleared by FSP-M".

Done

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Patch Set 7: Code-Review+1

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Patch Set 8: Code-Review+2

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

Patch Set 8: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47934 )

Change subject: security/intel/txt/ramstage.c: Fix clearing secrets on CBNT
......................................................................

security/intel/txt/ramstage.c: Fix clearing secrets on CBNT

intel_txt_memory_has_secret() checks for ESTS.TXT_ESTS_WAKE_ERROR_STS
|| E2STS.TXT_E2STS_SECRET_STS and it looks like with CBNT the E2STS
bit can be set without the ESTS bit.

Change-Id: Iff4436501b84f5c209add845b3cd3a62782d17e6
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47934
Reviewed-by: Jonathan Zhang 
Reviewed-by: Christian Walter 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/security/intel/txt/ramstage.c
1 file changed, 14 insertions(+), 16 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, but someone else must approve
  Jonathan Zhang: Looks good to me, approved
  Christian Walter: Looks good to me, approved

diff --git a/src/security/intel/txt/ramstage.c b/src/security/intel/txt/ramstage.c
index c33af89..cbc3a41 100644
--- a/src/security/intel/txt/ramstage.c
+++ b/src/security/intel/txt/ramstage.c
@@ -89,23 +89,21 @@
    if (status & ACMSTS_TXT_DISABLED)
    	return;

-	/* Check for fatal ACM error and TXT reset */
-	if (get_wake_error_status()) {
-		/*
-		 * Check if secrets bit needs to be reset. Only platforms that support
-		 * CONFIG(PLATFORM_HAS_DRAM_CLEAR) will be able to run this code.
-		 * Assume all memory really was cleared.
-		 *
-		 * TXT will issue a platform reset to come up sober.
-		 */
-		if (intel_txt_memory_has_secrets()) {
-			printk(BIOS_INFO, "TEE-TXT: Wiping TEE...\n");
-			intel_txt_run_bios_acm(ACMINPUT_CLEAR_SECRETS);
+	/*
+	 * Check if secrets bit needs to be reset. Only platforms that support
+	 * CONFIG(PLATFORM_HAS_DRAM_CLEAR) will be able to run this code.
+	 * On some platforms FSP-M takes care of the DRAM clearing.
+	 * Assume all memory really was cleared.
+	 *
+	 * TXT will issue a platform reset to come up sober.
+	 */
+	if (intel_txt_memory_has_secrets()) {
+		printk(BIOS_INFO, "TEE-TXT: Wiping TEE...\n");
+		intel_txt_run_bios_acm(ACMINPUT_CLEAR_SECRETS);

-			/* Should never reach this point ... */
-			intel_txt_log_acm_error(read32((void *)TXT_BIOSACM_ERRORCODE));
-			die("Waiting for platform reset...\n");
-		}
+		/* Should never reach this point ... */
+		intel_txt_log_acm_error(read32((void *)TXT_BIOSACM_ERRORCODE));
+		die("Waiting for platform reset...\n");
    }
 }