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