Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37304 )
Change subject: vboot: Don't perform EC sync in recovery mode ......................................................................
vboot: Don't perform EC sync in recovery mode
In recovery mode, EC sync is not supposed to be performed (everything should be running RO firmware). Also move nvdata_save() to the failure path only, otherwise if the FSP-S decides a system reboot is necessary, any recovery reason will be lost.
Note that this is the same logic that depthcharge uses as well.
BUG=b:145310842 BRANCH=firmware-hatch-12672.B TEST=Verify no EC sync happens in recovery mode
Change-Id: Ifb82396416d6b9af82fc75b3372f8339091469a4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/security/vboot/ec_sync.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/37304/1
diff --git a/src/security/vboot/ec_sync.c b/src/security/vboot/ec_sync.c index c2a6b25..67c5ae0 100644 --- a/src/security/vboot/ec_sync.c +++ b/src/security/vboot/ec_sync.c @@ -57,12 +57,14 @@ ctx = vboot_get_context(); ctx->flags |= VB2_CONTEXT_EC_SYNC_SUPPORTED;
- retval = vb2api_ec_sync(ctx); - vboot_save_nvdata_only(ctx); + if (!(ctx->flags & VB2_CONTEXT_RECOVERY_MODE)) { + retval = vb2api_ec_sync(ctx);
- if (retval != VB2_SUCCESS) { - printk(BIOS_ERR, "EC software sync failed (%#x), rebooting\n", retval); - vboot_reboot(); + if (retval != VB2_SUCCESS) { + printk(BIOS_ERR, "EC software sync failed (%#x), rebooting\n", retval); + vboot_save_nvdata_only(ctx); + vboot_reboot(); + } }
timestamp_add_now(TS_END_EC_SYNC);
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37304 )
Change subject: vboot: Don't perform EC sync in recovery mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... PS1, Line 60: if (!(ctx->flags & VB2_CONTEXT_RECOVERY_MODE)) { Does coreboot need to care about checking this? i.e. can this be handled in vb2api_ec_sync() so that it returns right away if VB2_CONTEXT_RECOVERY_MDOE is set?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37304 )
Change subject: vboot: Don't perform EC sync in recovery mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... PS1, Line 60: if (!(ctx->flags & VB2_CONTEXT_RECOVERY_MODE)) {
Does coreboot need to care about checking this? i.e. […]
I was just following the same logic as depthcharge. The verstage logic also directly checks this flag as well.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37304 )
Change subject: vboot: Don't perform EC sync in recovery mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... PS1, Line 60: if (!(ctx->flags & VB2_CONTEXT_RECOVERY_MODE)) {
I was just following the same logic as depthcharge. […]
I agree, I think checking in vboot is cleaner. That is equivalent to VbSelectAndLoadKernel() deciding internally whether to do software sync or not. There are other cases where coreboot/depthcharge need to check directly for recovery mode and that's fine, but in this case I don't think it's necessary.
Tim Wawrzynczak has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37304 )
Change subject: vboot: Don't perform EC sync in recovery mode ......................................................................
Abandoned
moving to vboot: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_referen...
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37304 )
Change subject: vboot: Don't perform EC sync in recovery mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... PS1, Line 61: vboot_save_nvdata_only(ctx); I don't think we need to abandon this change... we still need to move this line into the conditional below to fix the issue.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37304 )
Change subject: vboot: Don't perform EC sync in recovery mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... PS1, Line 61: vboot_save_nvdata_only(ctx);
I don't think we need to abandon this change... […]
Do we? NVDATA_CHANGED should never be set here when we're in recovery mode. I see no harm going through the check anyway?
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37304 )
Change subject: vboot: Don't perform EC sync in recovery mode ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/37304/1/src/security/vboot/ec_sync.... PS1, Line 61: vboot_save_nvdata_only(ctx);
Do we? NVDATA_CHANGED should never be set here when we're in recovery mode. […]
Yes, fair enough. Even though coreboot went and changed the nvdata backend value behind vboot's back, vboot *believes* it to be consistent with the vb2_context.nvdata value. So you are correct that nothing will be written here.