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.