Hello Joel Kitching,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37471
to review the following change.
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
vboot: Clear secdata change flags after factory init
factory_initialize_tpm() calls secdata_xxx_create() (for both firmware and kernel space) and then immediately writes those spaces out to the TPM. The create() functions make vboot think it just changed the secdata (because it reinitialized the byte arrays in the context), so we also need to clear the VB2_CONTEXT_SECDATA_xxx_CHANGED flags again, otherwise vboot thinks it still needs to flush the spaces out to the TPM even though we already did that.
Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 2 files changed, 6 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/37471/1
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 0afd00d..e4d49f7 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -188,7 +188,7 @@ if (rv != TPM_SUCCESS) return rv;
- return safe_write(index, data, length); + return write_secdata(index, data, length); }
static uint32_t set_firmware_space(const void *firmware_blob) @@ -398,6 +398,9 @@ if (result != TPM_SUCCESS) return result;
+ ctx->flags &= ~(VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED | + VB2_CONTEXT_SECDATA_KERNEL_CHANGED); + VBDEBUG("TPM: factory initialization successful\n");
return TPM_SUCCESS; diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index ccce148..6c4f8fd 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -265,10 +265,10 @@
void vboot_save_data(struct vb2_context *ctx) { - if (ctx->flags & VB2_CONTEXT_SECDATA_CHANGED) { + if (ctx->flags & VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED) { printk(BIOS_INFO, "Saving secdata\n"); antirollback_write_space_firmware(ctx); - ctx->flags &= ~VB2_CONTEXT_SECDATA_CHANGED; + ctx->flags &= ~VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED; }
vboot_save_nvdata_only(ctx);
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37471/2/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/37471/2/src/security/vboot/secdata_... PS2, Line 191: return write_secdata(index, data, length); Could you please explain this part of the change?
https://review.coreboot.org/c/coreboot/+/37471/2/src/security/vboot/secdata_... PS2, Line 402: VB2_CONTEXT_SECDATA_KERNEL_CHANGED); Please add comment here why this is needed.
Hello Aaron Durbin, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37471
to look at the new patch set (#3).
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
vboot: Clear secdata change flags after factory init
factory_initialize_tpm() calls secdata_xxx_create() (for both firmware and kernel space) and then immediately writes those spaces out to the TPM. The create() functions make vboot think it just changed the secdata (because it reinitialized the byte arrays in the context), so we also need to clear the VB2_CONTEXT_SECDATA_xxx_CHANGED flags again, otherwise vboot thinks it still needs to flush the spaces out to the TPM even though we already did that.
Also clean up some minor related stuff (VB2_CONTEXT_SECDATA_CHANGED notation is deprecated, and secdata space intialization should use the same write-and-readback function we use for updates).
Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 2 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/37471/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37471/2/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/37471/2/src/security/vboot/secdata_... PS2, Line 191: return write_secdata(index, data, length);
Could you please explain this part of the change?
Sorry, just drive-by fixing minor things as I see them. Added to commit message.
https://review.coreboot.org/c/coreboot/+/37471/2/src/security/vboot/secdata_... PS2, Line 402: VB2_CONTEXT_SECDATA_KERNEL_CHANGED);
Please add comment here why this is needed.
Done
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/secdata_... PS3, Line 96: int attempts = 3; We got rid of retries in depthcharge because we decided that our communication with Cr50 is reliable. Is that not the case here? (Or are we not making the assumption of using Cr50 here?)
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/secdata_... PS3, Line 418: /* : * This seems the first time we've run. Initialize the TPM. : */ Or perhaps also making this shorter than three lines?
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/secdata_... PS3, Line 425: //RETURN_ON_FAILURE(factory_initialize_tpm(ctx)); If we're fixing any other random stuff in this CL, should we think about removing this line?
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/vboot_lo... File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/vboot_lo... PS3, Line 268: VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED Good catch, thanks.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
Patch Set 3:
Once we get this CL in, we can remove the old secdata* names:
https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_referen...
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
Patch Set 3: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/secdata_... PS3, Line 96: int attempts = 3;
We got rid of retries in depthcharge because we decided that our communication with Cr50 is reliable […]
Other upstream coreboot users are still using different TPMs. Those are probably reliable too, idk... but not really something I want to explore that much in this CL, I just wanted to remove this inconsistency (because everything else in this file uses this helper instead of safe_write() directly).
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/secdata_... PS3, Line 418: /* : * This seems the first time we've run. Initialize the TPM. : */
Or perhaps also making this shorter than three lines?
Ack
https://review.coreboot.org/c/coreboot/+/37471/3/src/security/vboot/secdata_... PS3, Line 425: //RETURN_ON_FAILURE(factory_initialize_tpm(ctx));
If we're fixing any other random stuff in this CL, should we think about removing this line?
Sure.
Hello Aaron Durbin, build bot (Jenkins), Joel Kitching,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37471
to look at the new patch set (#4).
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
vboot: Clear secdata change flags after factory init
factory_initialize_tpm() calls secdata_xxx_create() (for both firmware and kernel space) and then immediately writes those spaces out to the TPM. The create() functions make vboot think it just changed the secdata (because it reinitialized the byte arrays in the context), so we also need to clear the VB2_CONTEXT_SECDATA_xxx_CHANGED flags again, otherwise vboot thinks it still needs to flush the spaces out to the TPM even though we already did that.
Also clean up some minor related stuff (VB2_CONTEXT_SECDATA_CHANGED notation is deprecated, and secdata space intialization should use the same write-and-readback function we use for updates).
Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 2 files changed, 9 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/37471/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37471/2/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/37471/2/src/security/vboot/secdata_... PS2, Line 191: return write_secdata(index, data, length);
Sorry, just drive-by fixing minor things as I see them. Added to commit message.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
Patch Set 4: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37471 )
Change subject: vboot: Clear secdata change flags after factory init ......................................................................
vboot: Clear secdata change flags after factory init
factory_initialize_tpm() calls secdata_xxx_create() (for both firmware and kernel space) and then immediately writes those spaces out to the TPM. The create() functions make vboot think it just changed the secdata (because it reinitialized the byte arrays in the context), so we also need to clear the VB2_CONTEXT_SECDATA_xxx_CHANGED flags again, otherwise vboot thinks it still needs to flush the spaces out to the TPM even though we already did that.
Also clean up some minor related stuff (VB2_CONTEXT_SECDATA_CHANGED notation is deprecated, and secdata space intialization should use the same write-and-readback function we use for updates).
Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37471 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/security/vboot/secdata_tpm.c M src/security/vboot/vboot_logic.c 2 files changed, 9 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 0afd00d..ef24555 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -188,7 +188,7 @@ if (rv != TPM_SUCCESS) return rv;
- return safe_write(index, data, length); + return write_secdata(index, data, length); }
static uint32_t set_firmware_space(const void *firmware_blob) @@ -398,6 +398,11 @@ if (result != TPM_SUCCESS) return result;
+ /* _factory_initialize_tpm() writes initial secdata values to TPM + immediately, so let vboot know that it's up to date now. */ + ctx->flags &= ~(VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED | + VB2_CONTEXT_SECDATA_KERNEL_CHANGED); + VBDEBUG("TPM: factory initialization successful\n");
return TPM_SUCCESS; @@ -410,14 +415,11 @@ /* Read the firmware space. */ rv = read_space_firmware(ctx); if (rv == TPM_E_BADINDEX) { - /* - * This seems the first time we've run. Initialize the TPM. - */ + /* This seems the first time we've run. Initialize the TPM. */ VBDEBUG("TPM: Not initialized yet.\n"); RETURN_ON_FAILURE(factory_initialize_tpm(ctx)); } else if (rv != TPM_SUCCESS) { VBDEBUG("TPM: Firmware space in a bad state; giving up.\n"); - //RETURN_ON_FAILURE(factory_initialize_tpm(ctx)); return TPM_E_CORRUPTED_STATE; }
diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index ccce148..6c4f8fd 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -265,10 +265,10 @@
void vboot_save_data(struct vb2_context *ctx) { - if (ctx->flags & VB2_CONTEXT_SECDATA_CHANGED) { + if (ctx->flags & VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED) { printk(BIOS_INFO, "Saving secdata\n"); antirollback_write_space_firmware(ctx); - ctx->flags &= ~VB2_CONTEXT_SECDATA_CHANGED; + ctx->flags &= ~VB2_CONTEXT_SECDATA_FIRMWARE_CHANGED; }
vboot_save_nvdata_only(ctx);
-
Aaron Durbin (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)