Change in coreboot[master]: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE

[Julius Werner (Code Review)]

Hello Philipp Deppenwiese, Bill XIE, Werner Zeh,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................

security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE

CB:35077 pulled TPM measurement code into the bootblock, with the catch
that we'll only cache PCR extensions and not actually write them to the
TPM until it gets initialized in a later stage. The goal of this was to
keep the heavy TPM driver code out of the size-constrained bootblock.

Unfortunately, a small mistake in the tspi_tpm_is_setup() function
prevents the compiler from eliminating references to the TPM driver
code in the bootblock on platforms with CONFIG_VBOOT and
CONFIG_SEPARATE_VERSTAGE. In those cases vboot_logic_executed() is known
at compile-time to be 0, but that still makes the final expression
`return 0 || tpm_is_setup;`. We know that tpm_is_setup can never be set
to 1 in the bootblock, but the compiler doesn't.

This patch rewrites the logic slightly to achieve the same effect in a
way that the compiler can follow (because we only really need to check
tpm_is_setup in the stage that actually runs the vboot code).

Signed-off-by: Julius Werner <[e-mail]>
Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
---
M src/security/tpm/tspi/tspi.c
1 file changed, 5 insertions(+), 2 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39993/1

diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c
index 4f0cc97..4b32657 100644
--- a/src/security/tpm/tspi/tspi.c
+++ b/src/security/tpm/tspi/tspi.c
@@ -104,8 +104,11 @@
 static int tpm_is_setup;
 static inline int tspi_tpm_is_setup(void)
 {
-	if (CONFIG(VBOOT))
-		return vboot_logic_executed() || tpm_is_setup;
+	if (CONFIG(VBOOT)) {
+		if (verification_should_run())
+			return tpm_is_setup;
+		return vboot_logic_executed();
+	}
 
 	if (ENV_RAMSTAGE)
 		return tpm_is_setup;

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <[e-mail]>
Gerrit-Reviewer: Bill XIE <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-MessageType: newchange

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39993 )

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39993/1/src/security/tpm/tspi/tspi.... 
File src/security/tpm/tspi/tspi.c:

https://review.coreboot.org/c/coreboot/+/39993/1/src/security/tpm/tspi/tspi.... 
PS1, Line 108: 		if (verification_should_run())
Could you please add a comment here as well describing why it's structured the way it is?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <[e-mail]>
Gerrit-Reviewer: Bill XIE <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Aaron Durbin <[e-mail]>
Gerrit-Comment-Date: Tue, 31 Mar 2020 22:40:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Julius Werner (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Bill XIE, Werner Zeh, 

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

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

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

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................

security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE

CB:35077 pulled TPM measurement code into the bootblock, with the catch
that we'll only cache PCR extensions and not actually write them to the
TPM until it gets initialized in a later stage. The goal of this was to
keep the heavy TPM driver code out of the size-constrained bootblock.

Unfortunately, a small mistake in the tspi_tpm_is_setup() function
prevents the compiler from eliminating references to the TPM driver
code in the bootblock on platforms with CONFIG_VBOOT and
CONFIG_SEPARATE_VERSTAGE. In those cases vboot_logic_executed() is known
at compile-time to be 0, but that still makes the final expression
`return 0 || tpm_is_setup;`. We know that tpm_is_setup can never be set
to 1 in the bootblock, but the compiler doesn't.

This patch rewrites the logic slightly to achieve the same effect in a
way that the compiler can follow (because we only really need to check
tpm_is_setup in the stage that actually runs the vboot code).

Signed-off-by: Julius Werner <[e-mail]>
Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
---
M src/security/tpm/tspi/tspi.c
1 file changed, 12 insertions(+), 2 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/39993/2
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 2
Gerrit-Owner: Julius Werner <[e-mail]>
Gerrit-Reviewer: Bill XIE <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Aaron Durbin <[e-mail]>
Gerrit-MessageType: newpatchset

[Julius Werner (Code Review)]

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

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39993/1/src/security/tpm/tspi/tspi.... 
File src/security/tpm/tspi/tspi.c:

https://review.coreboot.org/c/coreboot/+/39993/1/src/security/tpm/tspi/tspi.... 
PS1, Line 108: 		if (verification_should_run())
> Could you please add a comment here as well describing why it's structured the way it is?
> Done

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39993 )

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39993/1/src/security/tpm/tspi/tspi.... 
File src/security/tpm/tspi/tspi.c:

https://review.coreboot.org/c/coreboot/+/39993/1/src/security/tpm/tspi/tspi.... 
PS1, Line 108: 		if (verification_should_run())
> Done
> Thanks, Julius.

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39993 )

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39993/2/src/security/tpm/tspi/tspi.... 
File src/security/tpm/tspi/tspi.c:

https://review.coreboot.org/c/coreboot/+/39993/2/src/security/tpm/tspi/tspi.... 
PS2, Line 114: 	if (CONFIG(VBOOT)) {
Could { return vboot_logic_executed() || verification_should_run() || tpm_is_setup; } give the same result?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 2
Gerrit-Owner: Julius Werner <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Bill XIE <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Wed, 01 Apr 2020 03:17:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39993 )

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39993/2/src/security/tpm/tspi/tspi.... 
File src/security/tpm/tspi/tspi.c:

https://review.coreboot.org/c/coreboot/+/39993/2/src/security/tpm/tspi/tspi.... 
PS2, Line 114: 	if (CONFIG(VBOOT)) {
> Could { return vboot_logic_executed() || verification_should_run() || tpm_is_setup; } give the same  […]
> Sorry, it might be { return vboot_logic_executed() || (verification_should_run() && tpm_is_setup); }

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39993 )

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39993/2/src/security/tpm/tspi/tspi.... 
File src/security/tpm/tspi/tspi.c:

https://review.coreboot.org/c/coreboot/+/39993/2/src/security/tpm/tspi/tspi.... 
PS2, Line 114: 	if (CONFIG(VBOOT)) {
> Sorry, it might be { return vboot_logic_executed() || (verification_should_run() && tpm_is_setup); }
> Your achievement is chearer, though.

[Werner Zeh (Code Review)]

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

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 2: Code-Review+2

Thanks for this solution, way better than mine.
It works perfectly in my case.


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 2
Gerrit-Owner: Julius Werner <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Bill XIE <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Wed, 01 Apr 2020 04:17:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Christian Walter (Code Review)]

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

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 2: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 2
Gerrit-Owner: Julius Werner <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Bill XIE <[e-mail]>
Gerrit-Reviewer: Christian Walter <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 01 Apr 2020 19:52:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Julius Werner (Code Review)]

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

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39993/2/src/security/tpm/tspi/tspi.... 
File src/security/tpm/tspi/tspi.c:

https://review.coreboot.org/c/coreboot/+/39993/2/src/security/tpm/tspi/tspi.... 
PS2, Line 114: 	if (CONFIG(VBOOT)) {
> Your achievement is chearer, though.
> Yes it could, but yes, I think writing it this way is a bit clearer.

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39993 )

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................

security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE

CB:35077 pulled TPM measurement code into the bootblock, with the catch
that we'll only cache PCR extensions and not actually write them to the
TPM until it gets initialized in a later stage. The goal of this was to
keep the heavy TPM driver code out of the size-constrained bootblock.

Unfortunately, a small mistake in the tspi_tpm_is_setup() function
prevents the compiler from eliminating references to the TPM driver
code in the bootblock on platforms with CONFIG_VBOOT and
CONFIG_SEPARATE_VERSTAGE. In those cases vboot_logic_executed() is known
at compile-time to be 0, but that still makes the final expression
`return 0 || tpm_is_setup;`. We know that tpm_is_setup can never be set
to 1 in the bootblock, but the compiler doesn't.

This patch rewrites the logic slightly to achieve the same effect in a
way that the compiler can follow (because we only really need to check
tpm_is_setup in the stage that actually runs the vboot code).

Signed-off-by: Julius Werner <[e-mail]>
Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39993
Reviewed-by: Aaron Durbin <[e-mail]>
Reviewed-by: Bill XIE <[e-mail]>
Reviewed-by: Werner Zeh <[e-mail]>
Reviewed-by: Christian Walter <[e-mail]>
Tested-by: build bot (Jenkins) <[e-mail]>
---
M src/security/tpm/tspi/tspi.c
1 file changed, 12 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, but someone else must approve
  Werner Zeh: Looks good to me, approved
  Bill XIE: Looks good to me, but someone else must approve
  Christian Walter: Looks good to me, but someone else must approve



diff --git a/src/security/tpm/tspi/tspi.c b/src/security/tpm/tspi/tspi.c
index 4f0cc97..b94a0fb 100644
--- a/src/security/tpm/tspi/tspi.c
+++ b/src/security/tpm/tspi/tspi.c
@@ -104,8 +104,18 @@
 static int tpm_is_setup;
 static inline int tspi_tpm_is_setup(void)
 {
-	if (CONFIG(VBOOT))
-		return vboot_logic_executed() || tpm_is_setup;
+	/*
+	 * vboot_logic_executed() only starts returning true at the end of
+	 * verstage, but the vboot logic itself already wants to extend PCRs
+	 * before that. So in the stage where verification actually runs, we
+	 * need to check tpm_is_setup. Skip that check in all other stages so
+	 * this whole function can be evaluated at compile time.
+	 */
+	if (CONFIG(VBOOT)) {
+		if (verification_should_run())
+			return tpm_is_setup;
+		return vboot_logic_executed();
+	}
 
 	if (ENV_RAMSTAGE)
 		return tpm_is_setup;

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 3
Gerrit-Owner: Julius Werner <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Bill XIE <[e-mail]>
Gerrit-Reviewer: Christian Walter <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: merged

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39993 )

Change subject: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
......................................................................


Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1984
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1983
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1982

Please note: This test is under development and might not be accurate at all!


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/39993
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870
Gerrit-Change-Number: 39993
Gerrit-PatchSet: 3
Gerrit-Owner: Julius Werner <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Bill XIE <[e-mail]>
Gerrit-Reviewer: Christian Walter <[e-mail]>
Gerrit-Reviewer: Julius Werner <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: Werner Zeh <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: 9elements QA <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:36:23 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment