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)