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.