Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40359 )
Change subject: vboot/secdata: remove retries, readback, and CRC check ......................................................................
vboot/secdata: remove retries, readback, and CRC check
Depthcharge trusts that our TPM interface is working reliably, and so should we. Also remove CRC check -- the value returned by antirollback_read_space_firmware() is dropped in vboot_logic.c verstage_main(), and vboot handles this check internally.
BUG=b:124141368, chromium:972956 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I5d3f3823fca8507fd58087bb0f7b78cfa49417ab Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/secdata_tpm.c 1 file changed, 6 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/40359/1
diff --git a/src/security/vboot/secdata_tpm.c b/src/security/vboot/secdata_tpm.c index 87c33a5..4ca42ba 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -60,21 +60,10 @@
static uint32_t read_space_firmware(struct vb2_context *ctx) { - int attempts = 3; - - while (attempts--) { - RETURN_ON_FAILURE(tlcl_read(FIRMWARE_NV_INDEX, - ctx->secdata_firmware, - VB2_SECDATA_FIRMWARE_SIZE)); - - if (vb2api_secdata_firmware_check(ctx) == VB2_SUCCESS) - return TPM_SUCCESS; - - VBDEBUG("TPM: %s() - bad CRC\n", __func__); - } - - VBDEBUG("TPM: %s() - too many bad CRCs, giving up\n", __func__); - return TPM_E_CORRUPTED_STATE; + RETURN_ON_FAILURE(tlcl_read(FIRMWARE_NV_INDEX, + ctx->secdata_firmware, + VB2_SECDATA_FIRMWARE_SIZE)); + return TPM_SUCCESS; }
static uint32_t read_space_rec_hash(uint8_t *data) @@ -88,33 +77,8 @@ const uint8_t *secdata, uint32_t len) { - uint8_t sd[MAX(VB2_SECDATA_KERNEL_SIZE, VB2_SECDATA_FIRMWARE_SIZE)]; - uint32_t rv; - int attempts = 3; - - if (len > sizeof(sd)) { - VBDEBUG("TPM: %s() - data is too large\n", __func__); - return TPM_E_WRITE_FAILURE; - } - - while (attempts--) { - rv = safe_write(index, secdata, len); - /* Can't write, not gonna try again */ - if (rv != TPM_SUCCESS) - return rv; - - /* Read it back to be sure it got the right values. */ - rv = tlcl_read(index, sd, len); - if (rv == TPM_SUCCESS && memcmp(secdata, sd, len) == 0) - return rv; - - VBDEBUG("TPM: %s() failed. trying again\n", __func__); - /* Try writing it again. Maybe it was garbled on the way out. */ - } - - VBDEBUG("TPM: %s() - too many failures, giving up\n", __func__); - - return TPM_E_CORRUPTED_STATE; + RETURN_ON_FAILURE(safe_write(index, secdata, len)); + return TPM_SUCCESS; }
/*
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40359 )
Change subject: vboot/secdata: remove retries, readback, and CRC check ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40359/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40359/1//COMMIT_MSG@15 PS1, Line 15: TEST=make clean && make test-abuild Will it decrease the boot time?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40359 )
Change subject: vboot/secdata: remove retries, readback, and CRC check ......................................................................
Patch Set 1: Code-Review+2
Seems reasonable to let TPM drivers themselves worry about retries if necessary.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40359 )
Change subject: vboot/secdata: remove retries, readback, and CRC check ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40359/1/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/40359/1/src/security/vboot/secdata_... PS1, Line 76: static uint32_t write_secdata(uint32_t index, Does this function still serve a purpose?
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Paul Menzel, Julius Werner, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40359
to look at the new patch set (#2).
Change subject: vboot/secdata: remove retries, readback, and CRC check ......................................................................
vboot/secdata: remove retries, readback, and CRC check
Depthcharge trusts that our TPM driver is working reliably, and so should we. Also remove CRC check -- the value returned by antirollback_read_space_firmware() is dropped in vboot_logic.c verstage_main(), and vboot handles this check internally.
BUG=b:124141368, chromium:972956 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I5d3f3823fca8507fd58087bb0f7b78cfa49417ab Signed-off-by: Joel Kitching kitching@google.com --- M src/security/vboot/secdata_tpm.c 1 file changed, 15 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/40359/2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40359 )
Change subject: vboot/secdata: remove retries, readback, and CRC check ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40359/1/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/40359/1/src/security/vboot/secdata_... PS1, Line 76: static uint32_t write_secdata(uint32_t index,
Does this function still serve a purpose?
Let's get rid of it and call safe_write() directly.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40359 )
Change subject: vboot/secdata: remove retries, readback, and CRC check ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40359/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40359/1//COMMIT_MSG@15 PS1, Line 15: TEST=make clean && make test-abuild
Will it decrease the boot time?
This is about cleaning up the code flow. Boot time improvements are secondary concerns and not measured.
https://review.coreboot.org/c/coreboot/+/40359/1/src/security/vboot/secdata_... File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/40359/1/src/security/vboot/secdata_... PS1, Line 76: static uint32_t write_secdata(uint32_t index,
Let's get rid of it and call safe_write() directly.
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40359 )
Change subject: vboot/secdata: remove retries, readback, and CRC check ......................................................................
vboot/secdata: remove retries, readback, and CRC check
Depthcharge trusts that our TPM driver is working reliably, and so should we. Also remove CRC check -- the value returned by antirollback_read_space_firmware() is dropped in vboot_logic.c verstage_main(), and vboot handles this check internally.
BUG=b:124141368, chromium:972956 TEST=make clean && make test-abuild BRANCH=none
Change-Id: I5d3f3823fca8507fd58087bb0f7b78cfa49417ab Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40359 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, 15 insertions(+), 59 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 c052989..672578a 100644 --- a/src/security/vboot/secdata_tpm.c +++ b/src/security/vboot/secdata_tpm.c @@ -60,21 +60,10 @@
static uint32_t read_space_firmware(struct vb2_context *ctx) { - int attempts = 3; - - while (attempts--) { - RETURN_ON_FAILURE(tlcl_read(FIRMWARE_NV_INDEX, - ctx->secdata_firmware, - VB2_SECDATA_FIRMWARE_SIZE)); - - if (vb2api_secdata_firmware_check(ctx) == VB2_SUCCESS) - return TPM_SUCCESS; - - VBDEBUG("TPM: %s() - bad CRC\n", __func__); - } - - VBDEBUG("TPM: %s() - too many bad CRCs, giving up\n", __func__); - return TPM_E_CORRUPTED_STATE; + RETURN_ON_FAILURE(tlcl_read(FIRMWARE_NV_INDEX, + ctx->secdata_firmware, + VB2_SECDATA_FIRMWARE_SIZE)); + return TPM_SUCCESS; }
uint32_t antirollback_read_space_kernel(struct vb2_context *ctx) @@ -100,39 +89,6 @@ return TPM_SUCCESS; }
-static uint32_t write_secdata(uint32_t index, - const uint8_t *secdata, - uint32_t len) -{ - uint8_t sd[MAX(VB2_SECDATA_KERNEL_SIZE, VB2_SECDATA_FIRMWARE_SIZE)]; - uint32_t rv; - int attempts = 3; - - if (len > sizeof(sd)) { - VBDEBUG("TPM: %s() - data is too large\n", __func__); - return TPM_E_WRITE_FAILURE; - } - - while (attempts--) { - rv = safe_write(index, secdata, len); - /* Can't write, not gonna try again */ - if (rv != TPM_SUCCESS) - return rv; - - /* Read it back to be sure it got the right values. */ - rv = tlcl_read(index, sd, len); - if (rv == TPM_SUCCESS && memcmp(secdata, sd, len) == 0) - return rv; - - VBDEBUG("TPM: %s() failed. trying again\n", __func__); - /* Try writing it again. Maybe it was garbled on the way out. */ - } - - VBDEBUG("TPM: %s() - too many failures, giving up\n", __func__); - - return TPM_E_CORRUPTED_STATE; -} - /* * 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 @@ -201,7 +157,7 @@ if (rv != TPM_SUCCESS) return rv;
- return write_secdata(index, data, length); + return safe_write(index, data, length); }
static uint32_t set_firmware_space(const void *firmware_blob) @@ -300,8 +256,8 @@ TPM_NV_PER_GLOBALLOCK | TPM_NV_PER_PPWRITE, REC_HASH_NV_SIZE)); - RETURN_ON_FAILURE(write_secdata(REC_HASH_NV_INDEX, data, - REC_HASH_NV_SIZE)); + RETURN_ON_FAILURE(safe_write(REC_HASH_NV_INDEX, data, + REC_HASH_NV_SIZE));
return TPM_SUCCESS; } @@ -347,17 +303,17 @@ RETURN_ON_FAILURE(safe_define_space(KERNEL_NV_INDEX, TPM_NV_PER_PPWRITE, VB2_SECDATA_KERNEL_SIZE_V02)); - RETURN_ON_FAILURE(write_secdata(KERNEL_NV_INDEX, - ctx->secdata_kernel, - VB2_SECDATA_KERNEL_SIZE_V02)); + RETURN_ON_FAILURE(safe_write(KERNEL_NV_INDEX, + ctx->secdata_kernel, + VB2_SECDATA_KERNEL_SIZE_V02));
/* Define and write secdata_firmware space. */ RETURN_ON_FAILURE(safe_define_space(FIRMWARE_NV_INDEX, TPM_NV_PER_GLOBALLOCK | TPM_NV_PER_PPWRITE, VB2_SECDATA_FIRMWARE_SIZE)); - RETURN_ON_FAILURE(write_secdata(FIRMWARE_NV_INDEX, - ctx->secdata_firmware, + RETURN_ON_FAILURE(safe_write(FIRMWARE_NV_INDEX, + ctx->secdata_firmware, VB2_SECDATA_FIRMWARE_SIZE));
/* Define and set rec hash space, if available. */ @@ -449,8 +405,8 @@ { if (CONFIG(CR50_IMMEDIATELY_COMMIT_FW_SECDATA)) tlcl_cr50_enable_nvcommits(); - return write_secdata(FIRMWARE_NV_INDEX, ctx->secdata_firmware, - VB2_SECDATA_FIRMWARE_SIZE); + return safe_write(FIRMWARE_NV_INDEX, ctx->secdata_firmware, + VB2_SECDATA_FIRMWARE_SIZE); }
uint32_t antirollback_write_space_kernel(struct vb2_context *ctx) @@ -498,7 +454,7 @@ if (rv != TPM_SUCCESS) return rv;
- return write_secdata(REC_HASH_NV_INDEX, data, size); + return safe_write(REC_HASH_NV_INDEX, data, size); }
vb2_error_t vb2ex_tpm_clear_owner(struct vb2_context *ctx)
-
Duncan Laurie (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)