Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33386
Change subject: vboot: use vboot2 API to set initial secdatak value ......................................................................
vboot: use vboot2 API to set initial secdatak value
Previously, the initial value for secdatak was embedded in secdata_tpm.c as a uint8_t array. Switch to using vb2api_secdatak_create instead, and write the value in ctx->secdatak.
Remove an unnecessary call to vb2api_secdata_create in _factory_initialize_tpm.
BUG=b:124141368 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I74261453df6cc55ef3f38d8fb922bcc604084c0a Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/secdata_tpm.c 1 file changed, 9 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/33386/1
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 39cd614..ff62185 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -149,18 +149,6 @@ }
/* - * This is derived from rollback_index.h of vboot_reference. see struct - * RollbackSpaceKernel for details. - */ -static const uint8_t secdata_kernel[] = { - 0x02, - 0x4C, 0x57, 0x52, 0x47, - 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, - 0xE8, -}; - -/* * This is used to initialize the TPM space for recovery hash after defining * it. Since there is no data available to calculate hash at the point where TPM * space is defined, initialize it to all 0s. @@ -241,7 +229,7 @@ static uint32_t set_kernel_space(const void *kernel_blob) { return set_space("kernel", KERNEL_NV_INDEX, kernel_blob, - sizeof(secdata_kernel), rw_space_attributes, NULL, 0); + VB2_SECDATAK_SIZE, rw_space_attributes, NULL, 0); }
static uint32_t set_rec_hash_space(const uint8_t *data) @@ -262,7 +250,7 @@ * indication that TPM factory initialization was successfully * completed. */ - RETURN_ON_FAILURE(set_kernel_space(secdata_kernel)); + RETURN_ON_FAILURE(set_kernel_space(ctx->secdatak));
if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) RETURN_ON_FAILURE(set_rec_hash_space(rec_hash_data)); @@ -366,16 +354,15 @@ VBDEBUG("TPM: Clearing owner\n"); RETURN_ON_FAILURE(tpm_clear_and_reenable());
- /* Define and initialize the kernel space */ + /* Define and write secdatak kernel space. */ RETURN_ON_FAILURE(safe_define_space(KERNEL_NV_INDEX, TPM_NV_PER_PPWRITE, - sizeof(secdata_kernel))); + VB2_SECDATAK_SIZE)); RETURN_ON_FAILURE(write_secdata(KERNEL_NV_INDEX, - secdata_kernel, - sizeof(secdata_kernel))); + ctx->secdatak, + VB2_SECDATAK_SIZE));
- /* Defines and sets vb2 secdata space */ - vb2api_secdata_create(ctx); + /* Define and write secdata firmware space. */ RETURN_ON_FAILURE(safe_define_space(FIRMWARE_NV_INDEX, TPM_NV_PER_GLOBALLOCK | TPM_NV_PER_PPWRITE, @@ -417,8 +404,9 @@ { uint32_t result;
- /* Defines and sets vb2 secdata space */ + /* Set initial values of secdata and secdatak spaces. */ vb2api_secdata_create(ctx); + vb2api_secdatak_create(ctx);
VBDEBUG("TPM: factory initialization\n");
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33386 )
Change subject: vboot: use vboot2 API to set initial secdatak value ......................................................................
Patch Set 1:
Note that this isn't going to compile until we have this commit merged downstream to coreboot:
https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_referen...
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33386 )
Change subject: vboot: use vboot2 API to set initial secdatak value ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33386/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33386/1//COMMIT_MSG@18 PS1, Line 18: TEST=make clean && make test-abuild Please manually confirm (e.g. hack up a version with a memcmp() or something) that both size and content of the old and new versions are identical.
https://review.coreboot.org/#/c/33386/1/src/security/vboot/secdata_tpm.c File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/#/c/33386/1/src/security/vboot/secdata_tpm.c@232 PS1, Line 232: VB2_SECDATAK_SIZE Uhhh... maybe I'm bad at math, but this is currently defined to 14 in vboot, and I only count 13 bytes? Do we need to fix something there before we start using it? (chromeos-tpm-recovery also uses 0xd to recreate the space.)
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33386
to look at the new patch set (#2).
Change subject: vboot: use vboot2 API to set initial secdatak value ......................................................................
vboot: use vboot2 API to set initial secdatak value
Previously, the initial value for secdatak was embedded in secdata_tpm.c as a uint8_t array. Switch to using vb2api_secdatak_create instead, and write the value in ctx->secdatak.
Remove an unnecessary call to vb2api_secdata_create in _factory_initialize_tpm.
BUG=b:124141368, chromium:972956 TEST=make clean && make test-abuild BRANCH=none
TEST=Check that size and value of initial secdatak has not changed. Apply the patch below and check for this output: _factory_initialize_tpm():266: _factory_initialize_tpm: secdatak sizes are identical? 1 _factory_initialize_tpm():269: _factory_initialize_tpm: secdatak values are identical? 1
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index ff62185107..c1818b482f 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -148,6 +148,18 @@ static uint32_t write_secdata(uint32_t index, return TPM_E_CORRUPTED_STATE; }
+/* + * This is derived from rollback_index.h of vboot_reference. see struct + * RollbackSpaceKernel for details. + */ +static const uint8_t secdata_kernel[] = { + 0x02, + 0x4C, 0x57, 0x52, 0x47, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, + 0xE8, +}; + /* * This is used to initialize the TPM space for recovery hash after defining * it. Since there is no data available to calculate hash at the point where TPM @@ -250,6 +262,11 @@ static uint32_t _factory_initialize_tpm(struct vb2_context *ctx) * indication that TPM factory initialization was successfully * completed. */ + VBDEBUG("%s: secdatak sizes are identical? %d\n", __func__, + sizeof(secdata_kernel) == sizeof(ctx->secdatak)); + VBDEBUG("%s: secdatak values are identical? %d\n", __func__, + memcmp(secdata_kernel, ctx->secdatak, + sizeof(secdata_kernel)) == 0); RETURN_ON_FAILURE(set_kernel_space(ctx->secdatak));
if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) @@ -452,7 +469,7 @@ uint32_t antirollback_read_space_firmware(struct vb2_context *ctx)
/* Read the firmware space. */ rv = read_space_firmware(ctx); - if (rv == TPM_E_BADINDEX) { + if (true) { /* * This seems the first time we've run. Initialize the TPM. */
Change-Id: I74261453df6cc55ef3f38d8fb922bcc604084c0a Signed-off-by: Joel Kitching kitching@google.com Cq-Depend: chromium:1652874, chromium:1655049 --- M src/security/vboot/secdata_tpm.c 1 file changed, 9 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/33386/2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33386 )
Change subject: vboot: use vboot2 API to set initial secdatak value ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/33386/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33386/1//COMMIT_MSG@18 PS1, Line 18: TEST=make clean && make test-abuild
Please manually confirm (e.g. […]
Done. Not sure if it's the prettiest commit message.
https://review.coreboot.org/#/c/33386/1/src/security/vboot/secdata_tpm.c File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/#/c/33386/1/src/security/vboot/secdata_tpm.c@232 PS1, Line 232: VB2_SECDATAK_SIZE
Uhhh... […]
Nah, it looks like you are quite adept at mathematics =) https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_referen...
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33386 )
Change subject: vboot: use vboot2 API to set initial secdatak value ......................................................................
Patch Set 2: Code-Review+2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33386 )
Change subject: vboot: use vboot2 API to set initial secdatak value ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33386/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33386/1//COMMIT_MSG@18 PS1, Line 18: TEST=make clean && make test-abuild
Done. Not sure if it's the prettiest commit message.
Done
https://review.coreboot.org/c/coreboot/+/33386/1/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/33386/1/src/security/vboot/secdata_... PS1, Line 232: VB2_SECDATAK_SIZE
Nah, it looks like you are quite adept at mathematics =) […]
Done
Joel Kitching has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33386 )
Change subject: vboot: use vboot2 API to set initial secdatak value ......................................................................
vboot: use vboot2 API to set initial secdatak value
Previously, the initial value for secdatak was embedded in secdata_tpm.c as a uint8_t array. Switch to using vb2api_secdatak_create instead, and write the value in ctx->secdatak.
Remove an unnecessary call to vb2api_secdata_create in _factory_initialize_tpm.
BUG=b:124141368, chromium:972956 TEST=make clean && make test-abuild BRANCH=none
TEST=Check that size and value of initial secdatak has not changed. Apply the patch below and check for this output: _factory_initialize_tpm():266: _factory_initialize_tpm: secdatak sizes are identical? 1 _factory_initialize_tpm():269: _factory_initialize_tpm: secdatak values are identical? 1
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index ff62185107..c1818b482f 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -148,6 +148,18 @@ static uint32_t write_secdata(uint32_t index, return TPM_E_CORRUPTED_STATE; }
+/* + * This is derived from rollback_index.h of vboot_reference. see struct + * RollbackSpaceKernel for details. + */ +static const uint8_t secdata_kernel[] = { + 0x02, + 0x4C, 0x57, 0x52, 0x47, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, + 0xE8, +}; + /* * This is used to initialize the TPM space for recovery hash after defining * it. Since there is no data available to calculate hash at the point where TPM @@ -250,6 +262,11 @@ static uint32_t _factory_initialize_tpm(struct vb2_context *ctx) * indication that TPM factory initialization was successfully * completed. */ + VBDEBUG("%s: secdatak sizes are identical? %d\n", __func__, + sizeof(secdata_kernel) == sizeof(ctx->secdatak)); + VBDEBUG("%s: secdatak values are identical? %d\n", __func__, + memcmp(secdata_kernel, ctx->secdatak, + sizeof(secdata_kernel)) == 0); RETURN_ON_FAILURE(set_kernel_space(ctx->secdatak));
if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) @@ -452,7 +469,7 @@ uint32_t antirollback_read_space_firmware(struct vb2_context *ctx)
/* Read the firmware space. */ rv = read_space_firmware(ctx); - if (rv == TPM_E_BADINDEX) { + if (true) { /* * This seems the first time we've run. Initialize the TPM. */
Change-Id: I74261453df6cc55ef3f38d8fb922bcc604084c0a Signed-off-by: Joel Kitching kitching@google.com Cq-Depend: chromium:1652874, chromium:1655049 Reviewed-on: https://review.coreboot.org/c/coreboot/+/33386 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/security/vboot/secdata_tpm.c 1 file changed, 9 insertions(+), 21 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 09c7e72..2b98883 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -125,18 +125,6 @@ }
/* - * This is derived from rollback_index.h of vboot_reference. see struct - * RollbackSpaceKernel for details. - */ -static const uint8_t secdata_kernel[] = { - 0x02, - 0x4C, 0x57, 0x52, 0x47, - 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, - 0xE8, -}; - -/* * This is used to initialize the TPM space for recovery hash after defining * it. Since there is no data available to calculate hash at the point where TPM * space is defined, initialize it to all 0s. @@ -217,7 +205,7 @@ static uint32_t set_kernel_space(const void *kernel_blob) { return set_space("kernel", KERNEL_NV_INDEX, kernel_blob, - sizeof(secdata_kernel), rw_space_attributes, NULL, 0); + VB2_SECDATAK_SIZE, rw_space_attributes, NULL, 0); }
static uint32_t set_rec_hash_space(const uint8_t *data) @@ -238,7 +226,7 @@ * indication that TPM factory initialization was successfully * completed. */ - RETURN_ON_FAILURE(set_kernel_space(secdata_kernel)); + RETURN_ON_FAILURE(set_kernel_space(ctx->secdatak));
if (CONFIG(VBOOT_HAS_REC_HASH_SPACE)) RETURN_ON_FAILURE(set_rec_hash_space(rec_hash_data)); @@ -342,16 +330,15 @@ VBDEBUG("TPM: Clearing owner\n"); RETURN_ON_FAILURE(tpm_clear_and_reenable());
- /* Define and initialize the kernel space */ + /* Define and write secdatak kernel space. */ RETURN_ON_FAILURE(safe_define_space(KERNEL_NV_INDEX, TPM_NV_PER_PPWRITE, - sizeof(secdata_kernel))); + VB2_SECDATAK_SIZE)); RETURN_ON_FAILURE(write_secdata(KERNEL_NV_INDEX, - secdata_kernel, - sizeof(secdata_kernel))); + ctx->secdatak, + VB2_SECDATAK_SIZE));
- /* Defines and sets vb2 secdata space */ - vb2api_secdata_create(ctx); + /* Define and write secdata firmware space. */ RETURN_ON_FAILURE(safe_define_space(FIRMWARE_NV_INDEX, TPM_NV_PER_GLOBALLOCK | TPM_NV_PER_PPWRITE, @@ -393,8 +380,9 @@ { uint32_t result;
- /* Defines and sets vb2 secdata space */ + /* Set initial values of secdata and secdatak spaces. */ vb2api_secdata_create(ctx); + vb2api_secdatak_create(ctx);
VBDEBUG("TPM: factory initialization\n");
-
Joel Kitching (Code Review) -
Julius Werner (Code Review)