Change in coreboot[master]: security/tpm: Fix compile-time elimination for SEPARATE_VERSTAGE
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 jwerner@chromium.org 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;
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?
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 jwerner@chromium.org 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
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 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 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?
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 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 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.
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
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 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 jwerner@chromium.org Change-Id: Idc25acf1e6c02d929639e83d529cc14af80e0870 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39993 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Bill XIE persmule@hardenedlinux.org Reviewed-by: Werner Zeh werner.zeh@siemens.com Reviewed-by: Christian Walter christian.walter@9elements.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- 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;
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!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Bill XIE (Code Review)
-
Christian Walter (Code Review)
-
Julius Werner (Code Review)
-
Werner Zeh (Code Review)