Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_reinit to create and restore the context object respectively.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M 3rdparty/blobs M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 7 files changed, 64 insertions(+), 64 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/1
diff --git a/3rdparty/blobs b/3rdparty/blobs index 62aa0e0..678b4c4 160000 --- a/3rdparty/blobs +++ b/3rdparty/blobs @@ -1 +1 @@ -Subproject commit 62aa0e0c54295bbb7b1a3e5e73f960bafdb59d04 +Subproject commit 678b4c4a81069bb6e10e2e59f5374b83d727cd2b diff --git a/src/mainboard/google/drallion/chromeos.c b/src/mainboard/google/drallion/chromeos.c index 0eb311b..00571ed 100644 --- a/src/mainboard/google/drallion/chromeos.c +++ b/src/mainboard/google/drallion/chromeos.c @@ -95,8 +95,8 @@ * and the value from the TPM would be wrong anyway since the verstage * read would have cleared the value on the TPM. * - * The TPM recovery request is passed between stages through the - * vboot_get_shared_data or cbmem depending on stage. + * The TPM recovery request is passed between stages through vboot data + * or cbmem depending on stage. */ if (ENV_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c index 6643d9b..bdd414c 100644 --- a/src/mainboard/google/sarien/chromeos.c +++ b/src/mainboard/google/sarien/chromeos.c @@ -93,8 +93,8 @@ * and the value from the TPM would be wrong anyway since the verstage * read would have cleared the value on the TPM. * - * The TPM recovery request is passed between stages through the - * vboot_get_shared_data or cbmem depending on stage. + * The TPM recovery request is passed between stages through vboot data + * or cbmem depending on stage. */ if (ENV_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 4625bcd..25650b4 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -26,7 +26,8 @@
static int vboot_get_recovery_reason_shared_data(void) { - struct vb2_shared_data *sd = vboot_get_shared_data(); + struct vb2_shared_data *sd = + (struct vb2_shared_data *)vboot_get_workbuf(); assert(sd); return sd->recovery_reason; } diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 626fbc5..421c7a7 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -40,40 +40,40 @@ return wd; }
-void vboot_init_work_context(struct vb2_context *ctx) +struct vb2_context *vboot_get_context(void) { + static struct vb2_context *ctx; struct vboot_working_data *wd;
- /* First initialize the working data region. */ + /* Return if context has already been restored / initialized. */ + if (ctx) + return ctx; + wd = vboot_get_working_data(); - memset(wd, 0, VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE); + + /* Restore context from a previous stage. */ + if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) { + assert(vb2api_reinit(vboot_get_workbuf(), &ctx)); + return ctx; + }
/* * vboot prefers 16-byte alignment. This takes away 16 bytes * from the VBOOT2_WORK region, but the vboot devs said that's okay. */ + memset(wd, 0, sizeof(*wd)); wd->buffer_offset = ALIGN_UP(sizeof(*wd), 16); wd->buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE - wd->buffer_offset;
- /* Initialize the vb2_context. */ - memset(ctx, 0, sizeof(*ctx)); - ctx->workbuf = (void *)vboot_get_shared_data(); - ctx->workbuf_size = wd->buffer_size; + /* Initialize vb2_shared_data and friends. */ + assert(vb2api_init(vboot_get_workbuf(), wd->buffer_size, &ctx) + == VB2_SUCCESS); + + return ctx; }
-void vboot_finalize_work_context(struct vb2_context *ctx) -{ - /* - * Shrink buffer_size so that vboot_migrate_cbmem knows how - * much of vboot_working_data needs to be copied into CBMEM - * (if applicable), and so that downstream users know how much - * of the workbuf is currently used. - */ - vboot_get_working_data()->buffer_size = ctx->workbuf_used; -} - -struct vb2_shared_data *vboot_get_shared_data(void) +void *vboot_get_workbuf(void) { struct vboot_working_data *wd = vboot_get_working_data(); return (void *)((uintptr_t)wd + wd->buffer_offset); @@ -128,7 +128,8 @@ { const struct vboot_working_data *wd_preram = (struct vboot_working_data *)_vboot2_work; - size_t cbmem_size = wd_preram->buffer_offset + wd_preram->buffer_size; + /* TODO: We don't know the workbuf size with the current API... */ + size_t cbmem_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE; struct vboot_working_data *wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, cbmem_size); assert(wd_cbmem != NULL); diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 1458354..c64214b 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -54,9 +54,8 @@ * Source: security/vboot/common.c */ struct vboot_working_data *vboot_get_working_data(void); -void vboot_init_work_context(struct vb2_context *ctx); -void vboot_finalize_work_context(struct vb2_context *ctx); -struct vb2_shared_data *vboot_get_shared_data(void); +struct vb2_context *vboot_get_context(void); +void *vboot_get_workbuf(void);
/* Returns 0 on success. < 0 on failure. */ int vboot_get_selected_region(struct region *region); diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index da6231a..5110147 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -319,17 +319,17 @@ */ void verstage_main(void) { - struct vb2_context ctx; + struct vb2_context *ctx; struct region_device fw_main; vb2_error_t rv;
timestamp_add_now(TS_START_VBOOT);
/* Set up context and work buffer */ - vboot_init_work_context(&ctx); + ctx = vboot_get_context();
/* Initialize and read nvdata from non-volatile storage. */ - vbnv_init(ctx.nvdata); + vbnv_init(ctx->nvdata);
/* Set S3 resume flag if vboot should behave differently when selecting * which slot to boot. This is only relevant to vboot if the platform @@ -337,51 +337,51 @@ * the same slot that it booted from. */ if (CONFIG(RESUME_PATH_SAME_AS_BOOT) && vboot_platform_is_resuming()) - ctx.flags |= VB2_CONTEXT_S3_RESUME; + ctx->flags |= VB2_CONTEXT_S3_RESUME;
/* Read secdata from TPM. Initialize TPM if secdata not found. We don't * check the return value here because vb2api_fw_phase1 will catch * invalid secdata and tell us what to do (=reboot). */ timestamp_add_now(TS_START_TPMINIT); - if (vboot_setup_tpm(&ctx) == TPM_SUCCESS) - antirollback_read_space_firmware(&ctx); + if (vboot_setup_tpm(ctx) == TPM_SUCCESS) + antirollback_read_space_firmware(ctx); timestamp_add_now(TS_END_TPMINIT);
/* Enable measured boot mode */ if (CONFIG(VBOOT_MEASURED_BOOT) && - !(ctx.flags & VB2_CONTEXT_S3_RESUME)) { + !(ctx->flags & VB2_CONTEXT_S3_RESUME)) { if (vboot_init_crtm() != VB2_SUCCESS) die_with_post_code(POST_INVALID_ROM, "Initializing measured boot mode failed!"); }
if (get_recovery_mode_switch()) { - ctx.flags |= VB2_CONTEXT_FORCE_RECOVERY_MODE; + ctx->flags |= VB2_CONTEXT_FORCE_RECOVERY_MODE; if (CONFIG(VBOOT_DISABLE_DEV_ON_RECOVERY)) - ctx.flags |= VB2_CONTEXT_DISABLE_DEVELOPER_MODE; + ctx->flags |= VB2_CONTEXT_DISABLE_DEVELOPER_MODE; }
if (CONFIG(VBOOT_WIPEOUT_SUPPORTED) && get_wipeout_mode_switch()) - ctx.flags |= VB2_CONTEXT_FORCE_WIPEOUT_MODE; + ctx->flags |= VB2_CONTEXT_FORCE_WIPEOUT_MODE;
if (CONFIG(VBOOT_LID_SWITCH) && !get_lid_switch()) - ctx.flags |= VB2_CONTEXT_NOFAIL_BOOT; + ctx->flags |= VB2_CONTEXT_NOFAIL_BOOT;
/* Mainboard/SoC always initializes display. */ if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) - ctx.flags |= VB2_CONTEXT_DISPLAY_INIT; + ctx->flags |= VB2_CONTEXT_DISPLAY_INIT;
/* Do early init (set up secdata and NVRAM, load GBB) */ printk(BIOS_INFO, "Phase 1\n"); - rv = vb2api_fw_phase1(&ctx); + rv = vb2api_fw_phase1(ctx);
/* Jot down some information from vboot which may be required later on in coreboot boot flow. */ - if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT) + if (ctx->flags & VB2_CONTEXT_DISPLAY_INIT) /* Mainboard/SoC should initialize display. */ vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT; - if (ctx.flags & VB2_CONTEXT_DEVELOPER_MODE) + if (ctx->flags & VB2_CONTEXT_DEVELOPER_MODE) vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE;
if (rv) { @@ -393,58 +393,58 @@ */ if (rv == VB2_ERROR_API_PHASE1_RECOVERY) { printk(BIOS_INFO, "Recovery requested (%x)\n", rv); - save_if_needed(&ctx); - extend_pcrs(&ctx); /* ignore failures */ + save_if_needed(ctx); + extend_pcrs(ctx); /* ignore failures */ goto verstage_main_exit; }
printk(BIOS_INFO, "Reboot requested (%x)\n", rv); - save_if_needed(&ctx); + save_if_needed(ctx); vboot_reboot(); }
/* Determine which firmware slot to boot (based on NVRAM) */ printk(BIOS_INFO, "Phase 2\n"); - rv = vb2api_fw_phase2(&ctx); + rv = vb2api_fw_phase2(ctx); if (rv) { printk(BIOS_INFO, "Reboot requested (%x)\n", rv); - save_if_needed(&ctx); + save_if_needed(ctx); vboot_reboot(); }
/* Try that slot (verify its keyblock and preamble) */ printk(BIOS_INFO, "Phase 3\n"); timestamp_add_now(TS_START_VERIFY_SLOT); - rv = vb2api_fw_phase3(&ctx); + rv = vb2api_fw_phase3(ctx); timestamp_add_now(TS_END_VERIFY_SLOT); if (rv) { printk(BIOS_INFO, "Reboot requested (%x)\n", rv); - save_if_needed(&ctx); + save_if_needed(ctx); vboot_reboot(); }
printk(BIOS_INFO, "Phase 4\n"); - rv = locate_firmware(&ctx, &fw_main); + rv = locate_firmware(ctx, &fw_main); if (rv) die_with_post_code(POST_INVALID_ROM, "Failed to read FMAP to locate firmware");
- rv = hash_body(&ctx, &fw_main); - save_if_needed(&ctx); + rv = hash_body(ctx, &fw_main); + save_if_needed(ctx); if (rv) { printk(BIOS_INFO, "Reboot requested (%x)\n", rv); vboot_reboot(); }
/* Only extend PCRs once on boot. */ - if (!(ctx.flags & VB2_CONTEXT_S3_RESUME)) { + if (!(ctx->flags & VB2_CONTEXT_S3_RESUME)) { timestamp_add_now(TS_START_TPMPCR); - rv = extend_pcrs(&ctx); + rv = extend_pcrs(ctx); if (rv) { printk(BIOS_WARNING, "Failed to extend TPM PCRs (%#x)\n", rv); - vb2api_fail(&ctx, VB2_RECOVERY_RO_TPM_U_ERROR, rv); - save_if_needed(&ctx); + vb2api_fail(ctx, VB2_RECOVERY_RO_TPM_U_ERROR, rv); + save_if_needed(ctx); vboot_reboot(); } timestamp_add_now(TS_END_TPMPCR); @@ -456,8 +456,8 @@ rv = antirollback_lock_space_firmware(); if (rv) { printk(BIOS_INFO, "Failed to lock TPM (%x)\n", rv); - vb2api_fail(&ctx, VB2_RECOVERY_RO_TPM_L_ERROR, 0); - save_if_needed(&ctx); + vb2api_fail(ctx, VB2_RECOVERY_RO_TPM_L_ERROR, 0); + save_if_needed(ctx); vboot_reboot(); } timestamp_add_now(TS_END_TPMLOCK); @@ -468,14 +468,14 @@ if (rv) { printk(BIOS_INFO, "Failed to lock rec hash space(%x)\n", rv); - vb2api_fail(&ctx, VB2_RECOVERY_RO_TPM_REC_HASH_L_ERROR, + vb2api_fail(ctx, VB2_RECOVERY_RO_TPM_REC_HASH_L_ERROR, 0); - save_if_needed(&ctx); + save_if_needed(ctx); vboot_reboot(); } }
- printk(BIOS_INFO, "Slot %c is selected\n", is_slot_a(&ctx) ? 'A' : 'B'); + printk(BIOS_INFO, "Slot %c is selected\n", is_slot_a(ctx) ? 'A' : 'B'); vboot_set_selected_region(region_device_region(&fw_main));
verstage_main_exit: @@ -487,6 +487,5 @@ /* Save recovery reason in case of unexpected reboots on x86. */ vboot_save_recovery_reason_vbnv();
- vboot_finalize_work_context(&ctx); timestamp_add_now(TS_END_VBOOT); }
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36300/1/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/c/coreboot/+/36300/1/3rdparty/blobs@1 PS1, Line 1: Subproject commit 678b4c4a81069bb6e10e2e59f5374b83d727cd2b Please update your submodules and re-push.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 49: ctx Are all the platforms using vboot able to use static variables in their earlier stages?
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) { We shouldn't be dereferencing fields that may be uninitialized. The code should do the initialization once, the first time vboot is invoked from vboot_logic. That's what vboot_init_work_context was already doing. You could change the API to return the ctx pointer and then rely on the integrity of the object afterwards in the new vboot_get_context().
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 56: assert(vb2api_reinit(vboot_get_workbuf(), &ctx)); struct vboot_working_data isn't currently including the size of the vb2_context object. Where is the storage for ctx coming from? Allocated out of the work buf? Which vboot CL is providing the vb2api_reinit() ?
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 131: /* TODO: We don't know the workbuf size with the current API... */ It is the size provided by the macro below.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h@5... PS1, Line 58: void *vboot_get_workbuf(void); I think we could use some comments here to explain the differences between:
working data context workbuf
Should we rename working data to selection result for clarity?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... PS1, Line 30: (struct vb2_shared_data *)vboot_get_workbuf(); I'd prefer to do the same thing vboot does here (container_of(vboot_get_context(), vb2_shared_data, ctx)), rather than magically relying on shared data being the first thing in the workbuf.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 49: ctx
Are all the platforms using vboot able to use static variables in their earlier stages?
Should be no harm in marking this CAR_GLOBAL, just in case.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
We shouldn't be dereferencing fields that may be uninitialized. […]
I think this should be something like
if (!verification_should_run()) { assert(vboot_logic_executed()); assert(vb2api_reinit(vboot_get_workbuf(), car_get_var_ptr(&ctx)) == VB2_SUCCESS); return ctx; }
...the stuff that was formerly vboot_init_work_context()...
That way it should always get set up the first time it's used during verstage code, without having to worry about where to set it up explicitly.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 56: assert(vb2api_reinit(vboot_get_workbuf(), &ctx));
struct vboot_working_data isn't currently including the size of the vb2_context object. […]
Yes, we're moving the context inside the workbuf (so it can persist across stages). See CL:1716351 and go/vboot2-persistent-context.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 140: memcpy(wd_cbmem, wd_preram, cbmem_size); Didn't we agree on adding a vb2api_relocate() for this, Joel? (Then the 'ctx' pointer should be global, not static local, so that it can be updated here.)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h@5... PS1, Line 58: void *vboot_get_workbuf(void);
Should we rename working data to selection result for clarity?
Well, working data includes the workbuf as well. It's really more of a workbuf wrapper. Not sure if vboot_get_workbuf_wrapper() would be the best way to differentiate these? (We also have a few VBOOT_WD flags in there right now, which can be eliminated once this CL is in, but that should probably be left for a follow-up patch.)
Why is vboot_get_workbuf() even exported? Can't we keep it local to common.c?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... PS1, Line 30: (struct vb2_shared_data *)vboot_get_workbuf();
I'd prefer to do the same thing vboot does here (container_of(vboot_get_context(), vb2_shared_data, […]
edit: We could also make vb2_get_sd() available as one of the NEED_VB2_INTERNALS APIs. That's probably better than reimplementing it.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 131: /* TODO: We don't know the workbuf size with the current API... */
It is the size provided by the macro below.
I think the comment refers to the fact that we are getting rid of workbuf_used i.e. the actual amount of buffer really used by vboot and hence we might be allocating more in CBMEM than required.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 131: /* TODO: We don't know the workbuf size with the current API... */
I think the comment refers to the fact that we are getting rid of workbuf_used i.e. […]
We should probably just use KERNEL_WORKBUF_RECOMMENDED_SIZE here right away, then depthcharge can just use the workbuffer from there and doesn't have to vb2api_relocate() it again. (The KERNEL_WORKBUF_RECOMMENDED_SIZE is currently still a bit large for my tastes and can be reduced, but that can wait until later. In the meantime, stealing 80K from the OS isn't gonna kill us.)
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 131: /* TODO: We don't know the workbuf size with the current API... */
We should probably just use KERNEL_WORKBUF_RECOMMENDED_SIZE here right away, then depthcharge can ju […]
@Julius, yes, this is actually the current plan as listed in the design document. It seems extremely awkward to provide a vb2api_relocate function which can squash the workbuf to be smaller, when you don't actually know how small it can get. But I suppose if we don't have that use case at the moment, there is no need to expose workbuf_used?
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#2).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 70 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36300/1/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/c/coreboot/+/36300/1/3rdparty/blobs@1 PS1, Line 1: Subproject commit 678b4c4a81069bb6e10e2e59f5374b83d727cd2b
Please update your submodules and re-push.
Done
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... PS1, Line 30: (struct vb2_shared_data *)vboot_get_workbuf();
edit: We could also make vb2_get_sd() available as one of the NEED_VB2_INTERNALS APIs. […]
Actually, I already did for the depthcharge netboot case >< Let's use it here too.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 49: ctx
Should be no harm in marking this CAR_GLOBAL, just in case.
Sure, let's do that.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
I think this should be something like […]
I agree with Julius -- I'd prefer to encapsulate this in one function without having to set it up explicitly. On the other hand, this comes with a pretty big assumption, that we are only ever calling vboot_get_context() once in verstage. And we've already added a call to vboot_get_recovery_reason_shared_data() [in the latest PS], which gets called via vboot_save_recovery_reason_vbnv(), breaking this assumption.
The other option would be to add a field (flag? or we might even be able to get rid of flags with persistent context...) to vboot_working_data which is set after initialization is complete. Although this isn't much different than the current (less than ideal) solution.
How do other parts of coreboot typically deal with this situation?
For now, I think the safest thing is to explicitly initialize like Aaron suggested.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 56: assert(vb2api_reinit(vboot_get_workbuf(), &ctx));
Yes, we're moving the context inside the workbuf (so it can persist across stages). […]
Ack
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 140: memcpy(wd_cbmem, wd_preram, cbmem_size);
Didn't we agree on adding a vb2api_relocate() for this, Joel? (Then the 'ctx' pointer should be glob […]
Right -- I was just unsure of what to do because of the whole inability to get workbuf_used issue above. I'll change to VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE as discussed above, and use vb2api_relocate() here.
And I suppose you're right -- the ctx pointer MUST be a global.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h@5... PS1, Line 58: void *vboot_get_workbuf(void);
Should we rename working data to selection result for clarity? […]
Aaron, you're right that these can be pretty confusing. We do have a bit of explanation right above struct vboot_working_data. I'll re-work that comment.
On the other hand, as I mentioned in another comment, I wonder if vboot_working_data is even necessary at this point, now that vb2_context is always accessible? Is it possible to get the selected region from from vb2_context / vb2_shared_data?
Re. vboot_get_workbuf() -- changing to a local function.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/2/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/2/src/security/vboot/common.c... PS2, Line 146: VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE It'd be nice if we could know at this point how much extra we need for struct vboot_working_data + padding for 16-byte alignment... are we guaranteed that cbmem entries will be 16-byte aligned?
(Again, I think it'd be much simpler if the workbuf just lived in its own entry now...)
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#3).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 71 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/3
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#4).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 71 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/4
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
I agree with Julius -- I'd prefer to encapsulate this in one function without having to set it up ex […]
I take that back... it seems that vboot_get_recovery_reason_shared_data() is called even before verstage logic runs. I think it would be much safer to include everything in one function, so we don't have to worry about the order in which init() and get() are called...
But I don't think we can check !verification_should_run() -- this breaks the case of VBOOT_STARTS_IN_ROMSTAGE.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#5).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M 3rdparty/blobs M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 7 files changed, 79 insertions(+), 74 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/5
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#6).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 78 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/6
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 6:
(10 comments)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
On the other hand, this comes with a pretty big assumption, that we are only ever calling vboot_get_context() once in verstage.
No it doesn't? That's what the static (or now CAR_GLOBAL) variable is for. If you call it a second time in the same stage, it'll just give you the same context from there again.
I take that back... it seems that vboot_get_recovery_reason_shared_data() is called even before verstage logic runs. I think it would be much safer to include everything in one function, so we don't have to worry about the order in which init() and get() are called...
vboot_get_recovery_reason_shared_data() is guarded by vboot_logic_executed(), so I don't see how this would be a problem. It cannot run before verstage.
But I don't think we can check !verification_should_run() -- this breaks the case of VBOOT_STARTS_IN_ROMSTAGE.
How?
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 48: return (void *)((uintptr_t)wd + wd->buffer_offset); nit: might as well just inline this now? (you already have 'wd' fetched below)
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 56: if (car_get_var_ptr(vboot_ctx)) Careful, I think you're using that API wrong. 'vboot_ctx' is a pointer variable to the vboot context. The 'vboot_ctx' variable itself is the CAR_GLOBAL, not the context it points to. car_get_var_ptr() gives you a pointer to a CAR_GLOBAL.
So I think what you want here is...
if (*(car_get_var_ptr(&vboot_ctx))) return *(car_get_var_ptr(&vboot_ctx));
...which can be more easily written as...
if (car_get_var(vboot_ctx)) return car_get_var(vboot_ctx);
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 62: if (wd->buffer_offset > 0) { Careful, this data may be uninitialized on boot which is not guaranteed to be 0. I think you should do the thing I suggested before instead:
if (vboot_logic_executed()) { ...reinit... }
assert(verification_should_run());
(This is essentially equivalent to what I suggested earlier with the difference that it should also handle the RETURN_FROM_VERSTAGE case correctly.)
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 64: car_get_var_ptr(&vboot_ctx)) (This one looks right.)
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 73: memset(wd, 0, sizeof(*wd)); (This shows you that you really shouldn't read 'wd' before this. ;) )
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 82: return car_get_var_ptr(vboot_ctx); This again should just be car_get_var()
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 140: "VBOOT: copying vboot_working_data (%zu bytes) to CBMEM...\n", This message is no longer correct. We should move it inside vb2api_relocate().
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/misc.h@3... PS6, Line 35: vboot_init_context It's accessible via vboot_get_context() only, there is no vboot_init_context().
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h@5... PS1, Line 58: void *vboot_get_workbuf(void);
Aaron, you're right that these can be pretty confusing. […]
Getting rid of vboot_working_data completely may be possible but there are a few details to deal with (mostly that if you write it naively you'll do another FMAP lookup on flash for every CBFS file which we'd probably want to avoid). Let's decouple that from this patch.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 6:
(10 comments)
I was going to fix some stuff... but looks like the office is having yet another power outage.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
On the other hand, this comes with a pretty big assumption, that we are only ever calling vboot_ge […] But I don't think we can check !verification_should_run() -- this breaks the case of VBOOT_STARTS_IN_ROMSTAGE.
How?
Because we've already initialized vb2_context prior to romstage, and need to reinit it once we've reached verification. But our conditional rejects this case with !verification_should_run().
Same problem with your other comment, which has suggested replacing the conditional with vboot_logic_executed().
Unless I'm missing something, which is highly likely.
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 48: return (void *)((uintptr_t)wd + wd->buffer_offset);
nit: might as well just inline this now? (you already have 'wd' fetched below)
Done
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 56: if (car_get_var_ptr(vboot_ctx))
Careful, I think you're using that API wrong. […]
Done
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 62: if (wd->buffer_offset > 0) {
Careful, this data may be uninitialized on boot which is not guaranteed to be 0. […]
Done
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 64: car_get_var_ptr(&vboot_ctx))
(This one looks right. […]
Done
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 73: memset(wd, 0, sizeof(*wd));
(This shows you that you really shouldn't read 'wd' before this. […]
Ack
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 82: return car_get_var_ptr(vboot_ctx);
This again should just be car_get_var()
Done
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 140: "VBOOT: copying vboot_working_data (%zu bytes) to CBMEM...\n",
This message is no longer correct. We should move it inside vb2api_relocate().
I'm not against moving it into vb2api_relocate. But what's incorrect here? The fact that it shows the new size instead of the old one?
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/misc.h@3... PS6, Line 35: vboot_init_context
It's accessible via vboot_get_context() only, there is no vboot_init_context().
There was for a short, brief patchset. =)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h@5... PS1, Line 58: void *vboot_get_workbuf(void);
Getting rid of vboot_working_data completely may be possible but there are a few details to deal wit […]
Sure -- I'm definitely not suggesting that we do that in this CL.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#7).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 77 insertions(+), 75 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/7
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 7:
I was going to fix some stuff... but looks like the office is having yet another power outage.
I managed to clone the repo on my work Chromebook and upload this patchset without testing on a DUT.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 75: (void *)wd + wd->buffer_offset This is in here all over the place. Can you please provide a wrapper that provides the address of the buffer to use instead of open coding this everywhere?
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 77: car_get_var_ptr(&vboot_ctx) I apologize if you guys have been through this already. vb2_shared_data is a known entity to vboot users. i.e. we know sizes and fields names. This change is ensuring we don't know that and deferring to vboot_reference to get to:
1. get the address of vb2_contetxt 2. obtain vb2_shared_data from vb2_context when we know it's inside of vb2_shared data.
So this CL is trying to remove the assumption that the memory at wd->buffer_offset could be anything so we have no idea that it's actually vb2_shared_data?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 7:
(8 comments)
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/bootmode... PS7, Line 29: struct vb2_shared_data *sd = vb2_get_sd(vboot_get_context()); (...I guess this is one of the leftovers we're still not done surgically removing.)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
But I don't think we can check !verification_should_run() -- this breaks the case of VBOOT_STARTS_ […]
No, who has already initialized the context? The context gets initialized when vboot first runs, that's the whole point. Calling vboot_get_context() before that is an error. If VBOOT_STARTS_IN_ROMSTAGE, then nothing before the end of romstage should call vboot_get_context().
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 140: "VBOOT: copying vboot_working_data (%zu bytes) to CBMEM...\n",
I'm not against moving it into vb2api_relocate. […]
I mean, it says it's "copying" this many bytes, which is workbuf_used (not either the old or the new workbuf_size). Since workbuf_used is no longer publicly accessible, it has to be moved inside vboot (or dropped).
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 75: (void *)wd + wd->buffer_offset
This is in here all over the place. […]
Heh, poor Joel. I just asked him to drop that wrapper last patch set.
Okay, if you want a wrapper let's make one that takes 'wd' as an argument, so we don't have to call vboot_get_working_data() three times in here.
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 77: car_get_var_ptr(&vboot_ctx)
I apologize if you guys have been through this already. […]
Yes, it's removing any assumptions about where things are stored inside the work buffer. We want the work buffer to be fully opaque outside of vboot. Right now, vb2_shared_data still happens to be stored at the start of the work buffer, but we want to remove outside assumptions on that.
We also don't want anything outside of vboot code to access vb2_shared_data (that was really always the intent, but some compromises and hacks have crept in in the past which Joel has now painstakingly all surgically removed again). Having this model where the outside doesn't even know where it lives helps enforce that a bit more. The only vboot structure accessible by outside code should be vb2_context (which you can only get a valid pointer to with the vb2api_init()/_reinit()/_relocate() functions), and all information that needs to be passed to/from outside code should be in there. vb2_shared_data is for internal information that's intentionally hidden from outside code.
See go/vboot2-persistent-context for more details.
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 139: memcpy(wd_cbmem, wd_preram, sizeof(struct vboot_working_data)); Don't you need another
wd_cbmem->buffer_size = cbmem_size - wd_cbmem->buffer_offset;
here?
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 141: (void *)wd_preram + wd_preram->buffer_offset, nit: if we had a helper that takes in a vboot_working_data pointer to spit out the workbuf address, could use it for these two lines too
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 142: cbmem_size - wd_preram->buffer_offset, (and then this could just be wd_cbmem->buffer_size)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 49: memset(wd, 0, VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE); Is the responsibility to 0-initialize entire work buffer moved to vboot now? Or is it deemed not necessary?
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 139: memcpy(wd_cbmem, wd_preram, sizeof(struct vboot_working_data));
Don't you need another […]
Yes, that is required.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 140: "VBOOT: copying vboot_working_data (%zu bytes) to CBMEM...\n",
I mean, it says it's "copying" this many bytes, which is workbuf_used (not either the old or the new […]
+1
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#8).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 81 insertions(+), 78 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/8
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/bootmode... PS7, Line 29: struct vb2_shared_data *sd = vb2_get_sd(vboot_get_context());
(...I guess this is one of the leftovers we're still not done surgically removing. […]
Yeah... the recovery reason stuff is still something we need to untangle.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
No, who has already initialized the context? The context gets initialized when vboot first runs, tha […]
Just to double-check that we're not going to be causing any problems:
1 void vboot_save_recovery_reason_vbnv(void) 2 vboot_logic.c verstage_main() - end of verstage 3 int vboot_check_recovery_request(void) 4 mt8183/memory.c mt_mem_init() - romstage 5 chromeos/elog.c elog_add_boot_reason() - ramstage 6 int vboot_recovery_mode_enabled(void) 7 tons of memory initialization code - romstage
(2) is fine, since it occurs at the end of verstage.
(5) is fine, since ramstage will always take place after verstage regardless of vboot config.
(4) and (7) all route through vboot_check_recovery_request(), which only calls vboot_get_recovery_reason_shared_data() in the case that vboot_possibly_executed() and vboot_logic_executed().
So I think we are good. On the other hand, this seems very fragile. If VBOOT_STARTS_IN_ROMSTAGE is enabled, we're in romstage, and verification hasn't run yet, any unwitting call to vboot_get_context() would still return successfully. (Yes, it's an "error" to make this call, but there's currently no way to distinguish at runtime without splitting into two separate functions.)
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 140: "VBOOT: copying vboot_working_data (%zu bytes) to CBMEM...\n",
+1
Let's just drop it.
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 49: memset(wd, 0, VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE);
Is the responsibility to 0-initialize entire work buffer moved to vboot now? Or is it deemed not nec […]
coreboot takes care of zeroing the vboot_working_data struct.
vboot takes care of zeroing the vb2_shared_pointer struct (which includes vb2_context).
Everything else that takes place on the vboot workbuf should never assume zero-initialized, since it could be previously-used memory left over (freed) by other vboot calls.
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 75: (void *)wd + wd->buffer_offset
Heh, poor Joel. I just asked him to drop that wrapper last patch set. […]
¯_(ツ)_/¯
The only downside is that we have to either make two copies for const/non-const, change all const copies into non-const, or use a macro instead.
I opted for option #2, but if people prefer we could also do option #3.
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 77: car_get_var_ptr(&vboot_ctx)
Yes, it's removing any assumptions about where things are stored inside the work buffer. […]
Thanks for your excellent explanation of the situation, Julius.
Just to clarify, the design of vboot2 was *never* to allow external access to vb2_shared_data. My understanding is that in the early days of the vboot2 API, vb2_shared_data was used directly to get required pieces of information, rather than adding API calls. Later on, other code just followed that example, making the situation worse.
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 139: memcpy(wd_cbmem, wd_preram, sizeof(struct vboot_working_data));
Yes, that is required.
Yes -- although we currently don't make use of the buffer_size field after this point.
(We used to -- when depthcharge would need to copy the cbmem data into its own new workbuf memory. But now the important size field workbuf_size is saved in vb2_context, and read by vb2api_init/reinit/relocate.)
Also updating the argument in vb2api_relocate to use wd_cbmem->buffer_size instead.
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 141: (void *)wd_preram + wd_preram->buffer_offset,
nit: if we had a helper that takes in a vboot_working_data pointer to spit out the workbuf address, […]
Done
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 142: cbmem_size - wd_preram->buffer_offset,
(and then this could just be wd_cbmem->buffer_size)
Done
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 8:
Something broke in between PS6 and PS8.
With PS8, depthcharge doesn't find a valid kernel. So we must be doing something weird with the workbuf memory. Are we sure about all of our car_get_var* calls?
I'll keep investigating.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#9).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 81 insertions(+), 78 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/9
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 142: cbmem_size - wd_preram->buffer_offset,
Done
Well that's awkward. It turns out buffer_size is a uint16_t, which is too small to store VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE. The one place that it's actually used is in coreboot_table.c to give a size for the lb_range struct. Which probably isn't necessary to fill out, but could be nice for consistency. So maybe we should change it to uint32_t?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 9: Code-Review+2
(3 comments)
Okay, I think we can go with this and clean up the rest later!
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
Just to double-check that we're not going to be causing any problems: […]
Well, we can introduce another static vboot_started variable if you want. Or we could set vboot_executed right before running the vboot code rather than right after (I don't think anything relies on its value in the middle atm, but would need to double-check).
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 142: cbmem_size - wd_preram->buffer_offset,
Well that's awkward. […]
Argh... maybe just leave it incorrect then (with a comment), if it's not used at all anymore. We want to get rid of all of this anyway.
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c... PS9, Line 140: wd_cbmem->buffer_size = 0; nit: add comment why?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c... PS9, Line 52: struct vboot_working_data *wd; nit:
You can use a local variable vboot_ctx_ptr: struct vb2_context **vboot_ctx_ptr = car_get_var_ptr(&vboot_ctx);
and then you can avoid having to call car_get_var()/car_get_var_ptr() multiple times in this function. Just easier to read :).
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c... PS9, Line 140: wd_cbmem->buffer_size = 0;
nit: add comment why?
... and a TODO to remove buffer_size from vboot_working_data eventually? I think we can also get rid of the logic in lb_vboot_workbuf() and use add_cbmem_pointers() for CBMEM_ID_VBOOT_WORKBUF directly.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#10).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 88 insertions(+), 78 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/10
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 10:
(4 comments)
Seems like we are pretty much done with this CL.
Now we need to commit this into Chromium repo first, along with the depthcharge and vboot_reference Cq-Depends. Then I can uprev vboot_reference submodule in this CL.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
Well, we can introduce another static vboot_started variable if you want. […]
Perhaps we should just punt this for now, and deal with it along with cleaning up all the recovery_reason functions?
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c... PS9, Line 52: struct vboot_working_data *wd;
nit: […]
Sure, let's do that.
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c... PS9, Line 140: wd_cbmem->buffer_size = 0;
... […]
Added the TODO. I added your note about add_cbmem_pointers() to the bug I created here: https://bugs.chromium.org/p/chromium/issues/detail?id=1021452
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h@5... PS1, Line 58: void *vboot_get_workbuf(void);
Sure -- I'm definitely not suggesting that we do that in this CL.
Julius, could you comment on https://bugs.chromium.org/p/chromium/issues/detail?id=1021452 to make sure I'm understanding the FMAP lookup issue correctly?
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#11).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 88 insertions(+), 78 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/11
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 11:
(2 comments)
Fixed a typo.
https://review.coreboot.org/c/coreboot/+/36300/10/src/security/vboot/common.... File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/10/src/security/vboot/common.... PS10, Line 56: if (*vboot_ctx_ptr);
trailing semicolon indicates no statements, indent implies otherwise
Done
https://review.coreboot.org/c/coreboot/+/36300/10/src/security/vboot/common.... PS10, Line 56: if (*vboot_ctx_ptr);
trailing statements should be on next line
Done
Hello Aaron Durbin, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#12).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 6 files changed, 88 insertions(+), 78 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/12
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 12:
Note to self: compile and THEN push.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 12: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 55: if (wd->buffer_offset == ALIGN_UP(sizeof(*wd), 16)) {
Perhaps we should just punt this for now, and deal with it along with cleaning up all the recovery_r […]
Yes. I don't think there's an immediate problem here. We're just discussing additional safeguards we could add if we wanted to. (Note that those safeguards already don't exist today: if you call vboot_get_recovery_reason_shared_data() before vboot_init_work_context() in the current ToT you're also accessing garbage data and potentially invalid addresses. So this patch is not regressing safety, although we could of course still add more in the future.)
https://review.coreboot.org/c/coreboot/+/36300/12/src/security/vboot/common.... File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/12/src/security/vboot/common.... PS12, Line 52: struct vb2_context **vboot_ctx_ptr = car_get_var_ptr(&vboot_ctx); Honestly, I find this harder to follow than the previous version. But might be a matter of taste.
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/misc.h@5... PS1, Line 58: void *vboot_get_workbuf(void);
Julius, could you comment on https://bugs.chromium. […]
Done
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/12/src/security/vboot/common.... File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/12/src/security/vboot/common.... PS12, Line 52: struct vb2_context **vboot_ctx_ptr = car_get_var_ptr(&vboot_ctx);
Honestly, I find this harder to follow than the previous version. But might be a matter of taste.
¯_(ツ)_/¯
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 12: Code-Review+2
Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#13).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com --- M 3rdparty/vboot M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 7 files changed, 89 insertions(+), 79 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/13
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 13:
CB:36826
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36300/13//COMMIT_MSG@17 PS13, Line 17: Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Adding something like
Also-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
here should tell gerrit-rebase that the two CLs are the same, so please add that.
Hello Aaron Durbin, Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36300
to look at the new patch set (#14).
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com Also-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... --- M 3rdparty/vboot M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 7 files changed, 89 insertions(+), 79 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36300/14
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36300/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36300/13//COMMIT_MSG@17 PS13, Line 17: Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa
Adding something like […]
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 15: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 15:
(13 comments)
https://review.coreboot.org/c/coreboot/+/36300/1/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/c/coreboot/+/36300/1/3rdparty/blobs@1 PS1, Line 1: Subproject commit 678b4c4a81069bb6e10e2e59f5374b83d727cd2b
Done
Done
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/bootmode... PS1, Line 30: (struct vb2_shared_data *)vboot_get_workbuf();
Actually, I already did for the depthcharge netboot case >< Let's use it here too.
Done
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/bootmode... File src/security/vboot/bootmode.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/bootmode... PS7, Line 29: struct vb2_shared_data *sd = vb2_get_sd(vboot_get_context());
Yeah... the recovery reason stuff is still something we need to untangle.
Ack
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 49: ctx
Sure, let's do that.
Done
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 131: /* TODO: We don't know the workbuf size with the current API... */
@Julius, yes, this is actually the current plan as listed in the design document. […]
Done
https://review.coreboot.org/c/coreboot/+/36300/1/src/security/vboot/common.c... PS1, Line 140: memcpy(wd_cbmem, wd_preram, cbmem_size);
Right -- I was just unsure of what to do because of the whole inability to get workbuf_used issue ab […]
Done
https://review.coreboot.org/c/coreboot/+/36300/2/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/2/src/security/vboot/common.c... PS2, Line 146: VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE
It'd be nice if we could know at this point how much extra we need for struct vboot_working_data + p […]
Right now it's guaranteed to be 32 byte aligned (via imd_create_tiered_empty() setting up alignment variables and its callers setting up 1024 and 32 respectively)
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/common.c... PS6, Line 140: "VBOOT: copying vboot_working_data (%zu bytes) to CBMEM...\n",
Let's just drop it.
Done
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 75: (void *)wd + wd->buffer_offset
¯_(ツ)_/¯ […]
Done
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 77: car_get_var_ptr(&vboot_ctx)
Thanks for your excellent explanation of the situation, Julius. […]
Done
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 139: memcpy(wd_cbmem, wd_preram, sizeof(struct vboot_working_data));
Yes -- although we currently don't make use of the buffer_size field after this point. […]
Done
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/9/src/security/vboot/common.c... PS9, Line 52: struct vboot_working_data *wd;
Sure, let's do that.
Done
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/misc.h@3... PS6, Line 35: vboot_init_context
There was for a short, brief patchset. […]
can you make a follow-up CL to fix the comment?
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 15: Code-Review+2
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 15: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36300/2/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/2/src/security/vboot/common.c... PS2, Line 146: VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE
Right now it's guaranteed to be 32 byte aligned (via imd_create_tiered_empty() setting up alignment […]
Ack
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/36300/7/src/security/vboot/common.c... PS7, Line 49: memset(wd, 0, VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE);
coreboot takes care of zeroing the vboot_working_data struct. […]
Ack
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/c/coreboot/+/36300/6/src/security/vboot/misc.h@3... PS6, Line 35: vboot_init_context
can you make a follow-up CL to fix the comment?
See CB:36862
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36300 )
Change subject: vboot: use vboot persistent context ......................................................................
vboot: use vboot persistent context
vb2_context object is now stored on the workbuf as part of vb2_shared_data. Use vboot's new API functions vb2api_init and vb2api_relocate to create and move the workbuf.
BUG=b:124141368, chromium:994060 TEST=Build locally BRANCH=none
Change-Id: I051be1e47bf79b15a1689d49a5d4c031e9363dfa Signed-off-by: Joel Kitching kitching@google.com Also-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Reviewed-on: https://review.coreboot.org/c/coreboot/+/36300 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org Reviewed-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M 3rdparty/vboot M src/mainboard/google/drallion/chromeos.c M src/mainboard/google/sarien/chromeos.c M src/security/vboot/bootmode.c M src/security/vboot/common.c M src/security/vboot/misc.h M src/security/vboot/vboot_logic.c 7 files changed, 89 insertions(+), 79 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Stefan Reinauer: Looks good to me, approved Maulik V Vaghela: Looks good to me, approved
diff --git a/3rdparty/vboot b/3rdparty/vboot index 87276ff..ecdca93 160000 --- a/3rdparty/vboot +++ b/3rdparty/vboot @@ -1 +1 @@ -Subproject commit 87276ffed46b3c64ff62153ac8599a79b9bcb683 +Subproject commit ecdca931ae0637d1a9498f64862939bd5bb99e0b diff --git a/src/mainboard/google/drallion/chromeos.c b/src/mainboard/google/drallion/chromeos.c index 0eb311b..00571ed 100644 --- a/src/mainboard/google/drallion/chromeos.c +++ b/src/mainboard/google/drallion/chromeos.c @@ -95,8 +95,8 @@ * and the value from the TPM would be wrong anyway since the verstage * read would have cleared the value on the TPM. * - * The TPM recovery request is passed between stages through the - * vboot_get_shared_data or cbmem depending on stage. + * The TPM recovery request is passed between stages through vboot data + * or cbmem depending on stage. */ if (ENV_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && diff --git a/src/mainboard/google/sarien/chromeos.c b/src/mainboard/google/sarien/chromeos.c index 6643d9b..bdd414c 100644 --- a/src/mainboard/google/sarien/chromeos.c +++ b/src/mainboard/google/sarien/chromeos.c @@ -93,8 +93,8 @@ * and the value from the TPM would be wrong anyway since the verstage * read would have cleared the value on the TPM. * - * The TPM recovery request is passed between stages through the - * vboot_get_shared_data or cbmem depending on stage. + * The TPM recovery request is passed between stages through vboot data + * or cbmem depending on stage. */ if (ENV_VERSTAGE && tlcl_cr50_get_recovery_button(&cr50_state) == TPM_SUCCESS && diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c index 4625bcd..0ab0431 100644 --- a/src/security/vboot/bootmode.c +++ b/src/security/vboot/bootmode.c @@ -26,7 +26,7 @@
static int vboot_get_recovery_reason_shared_data(void) { - struct vb2_shared_data *sd = vboot_get_shared_data(); + struct vb2_shared_data *sd = vb2_get_sd(vboot_get_context()); assert(sd); return sd->recovery_reason; } diff --git a/src/security/vboot/common.c b/src/security/vboot/common.c index 626fbc5..043748c 100644 --- a/src/security/vboot/common.c +++ b/src/security/vboot/common.c @@ -24,6 +24,8 @@ #include <security/vboot/symbols.h> #include <security/vboot/vboot_common.h>
+static struct vb2_context *vboot_ctx CAR_GLOBAL; + struct vboot_working_data *vboot_get_working_data(void) { struct vboot_working_data *wd = NULL; @@ -40,43 +42,45 @@ return wd; }
-void vboot_init_work_context(struct vb2_context *ctx) +static inline void *vboot_get_workbuf(struct vboot_working_data *wd) { + return (void *)((uintptr_t)wd + wd->buffer_offset); +} + +struct vb2_context *vboot_get_context(void) +{ + struct vb2_context **vboot_ctx_ptr = car_get_var_ptr(&vboot_ctx); struct vboot_working_data *wd;
- /* First initialize the working data region. */ + /* Return if context has already been initialized/restored. */ + if (*vboot_ctx_ptr) + return *vboot_ctx_ptr; + wd = vboot_get_working_data(); - memset(wd, 0, VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE); + + /* Restore context from a previous stage. */ + if (vboot_logic_executed()) { + assert(vb2api_reinit(vboot_get_workbuf(wd), + vboot_ctx_ptr) == VB2_SUCCESS); + return *vboot_ctx_ptr; + } + + assert(verification_should_run());
/* * vboot prefers 16-byte alignment. This takes away 16 bytes * from the VBOOT2_WORK region, but the vboot devs said that's okay. */ + memset(wd, 0, sizeof(*wd)); wd->buffer_offset = ALIGN_UP(sizeof(*wd), 16); wd->buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE - wd->buffer_offset;
- /* Initialize the vb2_context. */ - memset(ctx, 0, sizeof(*ctx)); - ctx->workbuf = (void *)vboot_get_shared_data(); - ctx->workbuf_size = wd->buffer_size; -} + /* Initialize vb2_shared_data and friends. */ + assert(vb2api_init(vboot_get_workbuf(wd), wd->buffer_size, + vboot_ctx_ptr) == VB2_SUCCESS);
-void vboot_finalize_work_context(struct vb2_context *ctx) -{ - /* - * Shrink buffer_size so that vboot_migrate_cbmem knows how - * much of vboot_working_data needs to be copied into CBMEM - * (if applicable), and so that downstream users know how much - * of the workbuf is currently used. - */ - vboot_get_working_data()->buffer_size = ctx->workbuf_used; -} - -struct vb2_shared_data *vboot_get_shared_data(void) -{ - struct vboot_working_data *wd = vboot_get_working_data(); - return (void *)((uintptr_t)wd + wd->buffer_offset); + return *vboot_ctx_ptr; }
int vboot_get_selected_region(struct region *region) @@ -126,17 +130,25 @@ */ static void vboot_migrate_cbmem(int unused) { - const struct vboot_working_data *wd_preram = + const size_t cbmem_size = VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE; + struct vboot_working_data *wd_preram = (struct vboot_working_data *)_vboot2_work; - size_t cbmem_size = wd_preram->buffer_offset + wd_preram->buffer_size; struct vboot_working_data *wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, cbmem_size); assert(wd_cbmem != NULL); - - printk(BIOS_DEBUG, - "VBOOT: copying vboot_working_data (%zu bytes) to CBMEM...\n", - cbmem_size); - memcpy(wd_cbmem, wd_preram, cbmem_size); + memcpy(wd_cbmem, wd_preram, sizeof(struct vboot_working_data)); + /* + * TODO(chromium:1021452): buffer_size is uint16_t and not large enough + * to hold the kernel verification workbuf size. The only code which + * reads this value is in lb_vboot_workbuf() for lb_range->range_size. + * This value being zero doesn't cause any problems, since it is never + * read downstream. Fix or deprecate vboot_working_data. + */ + wd_cbmem->buffer_size = 0; + vb2api_relocate(vboot_get_workbuf(wd_cbmem), + vboot_get_workbuf(wd_preram), + cbmem_size - wd_cbmem->buffer_offset, + car_get_var_ptr(&vboot_ctx)); } ROMSTAGE_CBMEM_INIT_HOOK(vboot_migrate_cbmem) #else @@ -144,7 +156,7 @@ { struct vboot_working_data *wd_cbmem = cbmem_add(CBMEM_ID_VBOOT_WORKBUF, - VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE); + VB2_KERNEL_WORKBUF_RECOMMENDED_SIZE); assert(wd_cbmem != NULL); } ROMSTAGE_CBMEM_INIT_HOOK(vboot_setup_cbmem) diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h index 1458354..b45fc9c 100644 --- a/src/security/vboot/misc.h +++ b/src/security/vboot/misc.h @@ -29,10 +29,11 @@ };
/* - * this is placed at the start of the vboot work buffer. selected_region is used - * for the verstage to return the location of the selected slot. buffer is used - * by the vboot2 core. Keep the struct CPU architecture agnostic as it crosses - * stage boundaries. + * Stores vboot-related information. selected_region is used by verstage to + * store the location of the selected slot. buffer is used by vboot to store + * its work buffer. vb2_context is contained within this work buffer, and is + * accessible via vboot_init_context() and vboot_get_context() (see below). + * Keep the struct CPU architecture agnostic as it crosses stage boundaries. */ struct vboot_working_data { struct selected_region selected_region; @@ -54,9 +55,7 @@ * Source: security/vboot/common.c */ struct vboot_working_data *vboot_get_working_data(void); -void vboot_init_work_context(struct vb2_context *ctx); -void vboot_finalize_work_context(struct vb2_context *ctx); -struct vb2_shared_data *vboot_get_shared_data(void); +struct vb2_context *vboot_get_context(void);
/* Returns 0 on success. < 0 on failure. */ int vboot_get_selected_region(struct region *region); diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index da6231a..5110147 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -319,17 +319,17 @@ */ void verstage_main(void) { - struct vb2_context ctx; + struct vb2_context *ctx; struct region_device fw_main; vb2_error_t rv;
timestamp_add_now(TS_START_VBOOT);
/* Set up context and work buffer */ - vboot_init_work_context(&ctx); + ctx = vboot_get_context();
/* Initialize and read nvdata from non-volatile storage. */ - vbnv_init(ctx.nvdata); + vbnv_init(ctx->nvdata);
/* Set S3 resume flag if vboot should behave differently when selecting * which slot to boot. This is only relevant to vboot if the platform @@ -337,51 +337,51 @@ * the same slot that it booted from. */ if (CONFIG(RESUME_PATH_SAME_AS_BOOT) && vboot_platform_is_resuming()) - ctx.flags |= VB2_CONTEXT_S3_RESUME; + ctx->flags |= VB2_CONTEXT_S3_RESUME;
/* Read secdata from TPM. Initialize TPM if secdata not found. We don't * check the return value here because vb2api_fw_phase1 will catch * invalid secdata and tell us what to do (=reboot). */ timestamp_add_now(TS_START_TPMINIT); - if (vboot_setup_tpm(&ctx) == TPM_SUCCESS) - antirollback_read_space_firmware(&ctx); + if (vboot_setup_tpm(ctx) == TPM_SUCCESS) + antirollback_read_space_firmware(ctx); timestamp_add_now(TS_END_TPMINIT);
/* Enable measured boot mode */ if (CONFIG(VBOOT_MEASURED_BOOT) && - !(ctx.flags & VB2_CONTEXT_S3_RESUME)) { + !(ctx->flags & VB2_CONTEXT_S3_RESUME)) { if (vboot_init_crtm() != VB2_SUCCESS) die_with_post_code(POST_INVALID_ROM, "Initializing measured boot mode failed!"); }
if (get_recovery_mode_switch()) { - ctx.flags |= VB2_CONTEXT_FORCE_RECOVERY_MODE; + ctx->flags |= VB2_CONTEXT_FORCE_RECOVERY_MODE; if (CONFIG(VBOOT_DISABLE_DEV_ON_RECOVERY)) - ctx.flags |= VB2_CONTEXT_DISABLE_DEVELOPER_MODE; + ctx->flags |= VB2_CONTEXT_DISABLE_DEVELOPER_MODE; }
if (CONFIG(VBOOT_WIPEOUT_SUPPORTED) && get_wipeout_mode_switch()) - ctx.flags |= VB2_CONTEXT_FORCE_WIPEOUT_MODE; + ctx->flags |= VB2_CONTEXT_FORCE_WIPEOUT_MODE;
if (CONFIG(VBOOT_LID_SWITCH) && !get_lid_switch()) - ctx.flags |= VB2_CONTEXT_NOFAIL_BOOT; + ctx->flags |= VB2_CONTEXT_NOFAIL_BOOT;
/* Mainboard/SoC always initializes display. */ if (!CONFIG(VBOOT_MUST_REQUEST_DISPLAY)) - ctx.flags |= VB2_CONTEXT_DISPLAY_INIT; + ctx->flags |= VB2_CONTEXT_DISPLAY_INIT;
/* Do early init (set up secdata and NVRAM, load GBB) */ printk(BIOS_INFO, "Phase 1\n"); - rv = vb2api_fw_phase1(&ctx); + rv = vb2api_fw_phase1(ctx);
/* Jot down some information from vboot which may be required later on in coreboot boot flow. */ - if (ctx.flags & VB2_CONTEXT_DISPLAY_INIT) + if (ctx->flags & VB2_CONTEXT_DISPLAY_INIT) /* Mainboard/SoC should initialize display. */ vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DISPLAY_INIT; - if (ctx.flags & VB2_CONTEXT_DEVELOPER_MODE) + if (ctx->flags & VB2_CONTEXT_DEVELOPER_MODE) vboot_get_working_data()->flags |= VBOOT_WD_FLAG_DEVELOPER_MODE;
if (rv) { @@ -393,58 +393,58 @@ */ if (rv == VB2_ERROR_API_PHASE1_RECOVERY) { printk(BIOS_INFO, "Recovery requested (%x)\n", rv); - save_if_needed(&ctx); - extend_pcrs(&ctx); /* ignore failures */ + save_if_needed(ctx); + extend_pcrs(ctx); /* ignore failures */ goto verstage_main_exit; }
printk(BIOS_INFO, "Reboot requested (%x)\n", rv); - save_if_needed(&ctx); + save_if_needed(ctx); vboot_reboot(); }
/* Determine which firmware slot to boot (based on NVRAM) */ printk(BIOS_INFO, "Phase 2\n"); - rv = vb2api_fw_phase2(&ctx); + rv = vb2api_fw_phase2(ctx); if (rv) { printk(BIOS_INFO, "Reboot requested (%x)\n", rv); - save_if_needed(&ctx); + save_if_needed(ctx); vboot_reboot(); }
/* Try that slot (verify its keyblock and preamble) */ printk(BIOS_INFO, "Phase 3\n"); timestamp_add_now(TS_START_VERIFY_SLOT); - rv = vb2api_fw_phase3(&ctx); + rv = vb2api_fw_phase3(ctx); timestamp_add_now(TS_END_VERIFY_SLOT); if (rv) { printk(BIOS_INFO, "Reboot requested (%x)\n", rv); - save_if_needed(&ctx); + save_if_needed(ctx); vboot_reboot(); }
printk(BIOS_INFO, "Phase 4\n"); - rv = locate_firmware(&ctx, &fw_main); + rv = locate_firmware(ctx, &fw_main); if (rv) die_with_post_code(POST_INVALID_ROM, "Failed to read FMAP to locate firmware");
- rv = hash_body(&ctx, &fw_main); - save_if_needed(&ctx); + rv = hash_body(ctx, &fw_main); + save_if_needed(ctx); if (rv) { printk(BIOS_INFO, "Reboot requested (%x)\n", rv); vboot_reboot(); }
/* Only extend PCRs once on boot. */ - if (!(ctx.flags & VB2_CONTEXT_S3_RESUME)) { + if (!(ctx->flags & VB2_CONTEXT_S3_RESUME)) { timestamp_add_now(TS_START_TPMPCR); - rv = extend_pcrs(&ctx); + rv = extend_pcrs(ctx); if (rv) { printk(BIOS_WARNING, "Failed to extend TPM PCRs (%#x)\n", rv); - vb2api_fail(&ctx, VB2_RECOVERY_RO_TPM_U_ERROR, rv); - save_if_needed(&ctx); + vb2api_fail(ctx, VB2_RECOVERY_RO_TPM_U_ERROR, rv); + save_if_needed(ctx); vboot_reboot(); } timestamp_add_now(TS_END_TPMPCR); @@ -456,8 +456,8 @@ rv = antirollback_lock_space_firmware(); if (rv) { printk(BIOS_INFO, "Failed to lock TPM (%x)\n", rv); - vb2api_fail(&ctx, VB2_RECOVERY_RO_TPM_L_ERROR, 0); - save_if_needed(&ctx); + vb2api_fail(ctx, VB2_RECOVERY_RO_TPM_L_ERROR, 0); + save_if_needed(ctx); vboot_reboot(); } timestamp_add_now(TS_END_TPMLOCK); @@ -468,14 +468,14 @@ if (rv) { printk(BIOS_INFO, "Failed to lock rec hash space(%x)\n", rv); - vb2api_fail(&ctx, VB2_RECOVERY_RO_TPM_REC_HASH_L_ERROR, + vb2api_fail(ctx, VB2_RECOVERY_RO_TPM_REC_HASH_L_ERROR, 0); - save_if_needed(&ctx); + save_if_needed(ctx); vboot_reboot(); } }
- printk(BIOS_INFO, "Slot %c is selected\n", is_slot_a(&ctx) ? 'A' : 'B'); + printk(BIOS_INFO, "Slot %c is selected\n", is_slot_a(ctx) ? 'A' : 'B'); vboot_set_selected_region(region_device_region(&fw_main));
verstage_main_exit: @@ -487,6 +487,5 @@ /* Save recovery reason in case of unexpected reboots on x86. */ vboot_save_recovery_reason_vbnv();
- vboot_finalize_work_context(&ctx); timestamp_add_now(TS_END_VBOOT); }
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Maulik V Vaghela (Code Review)
-
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)