Change in coreboot[master]: vboot: Clear secdata change flags after factory init

List overview All Threads
Download

newer

older

Change in coreboot[4.11_branch]:...

Change in coreboot[master]: vboot:...

Julius Werner (Code Review)

4 Dec 2019 4 Dec '19

9:06 p.m.

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);

Attachments:

attachment.htm (text/html — 4.5 KB)

Show replies by thread

Aaron Durbin (Code Review)

4 Dec 4 Dec

9:28 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Dec 2019 21:28:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Julius Werner (Code Review)

9:44 p.m.

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

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).

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/37471/3

Julius Werner (Code Review)

9:44 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Dec 2019 21:44:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Joel Kitching (Code Review)

5 Dec 5 Dec

9:30 a.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 05 Dec 2019 09:30:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Joel Kitching (Code Review)

9:37 a.m.

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...

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 05 Dec 2019 09:37:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Aaron Durbin (Code Review)

2:04 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 05 Dec 2019 14:04:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Julius Werner (Code Review)

10:42 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 3 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 05 Dec 2019 22:42:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Joel Kitching kitching@google.com Gerrit-MessageType: comment

Julius Werner (Code Review)

6 Dec 6 Dec

1:52 a.m.

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

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).

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/37471/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 4 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Julius Werner (Code Review)

1:54 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 4 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 06 Dec 2019 01:54:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: Julius Werner jwerner@chromium.org Gerrit-MessageType: comment

Aaron Durbin (Code Review)

4:53 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 4 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 06 Dec 2019 04:53:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Julius Werner (Code Review)

5:23 a.m.

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

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 @@

vboot_save_nvdata_only(ctx);

-- To view, visit https://review.coreboot.org/c/coreboot/+/37471 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I231fadcf7b35a1aec3b39254e7e41c3d456d4911 Gerrit-Change-Number: 37471 Gerrit-PatchSet: 5 Gerrit-Owner: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1625

days inactive

1627

days old

coreboot-gerrit@coreboot.org

11 comments

3 participants

tags (0)

participants (3)

Aaron Durbin (Code Review)
Joel Kitching (Code Review)
Julius Werner (Code Review)