Attention is currently required from: Andrey Pronin, Aseda Aboagye, Aaron Durbin. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52919 )
Change subject: chromeos/Kconfig: Add TPM20_CREATE_FWMP ......................................................................
Patch Set 3:
(5 comments)
File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/52919/comment/739bf9ef_8b1c1449 PS1, Line 111: I'd put the attribute definition here so we have them all in one place to compare.
https://review.coreboot.org/c/coreboot/+/52919/comment/d7633e7f_01f35a81 PS1, Line 237: .TPMA_NV_OWNERWRITE = 1, Should we also add PPWRITE just in case the RW firmware ever needs to make modifications there? (Not currently needed but may be useful to keep open as an option for unexpected future requirements.)
https://review.coreboot.org/c/coreboot/+/52919/comment/1292c9a6_6cbefd73 PS1, Line 243: rv = tlcl_define_space(FWMP_NV_INDEX, VB2_SECDATA_FWMP_MAX_SIZE, Why not reuse set_space()? We should initialize the FWMP with valid data too. You can add a vb2_secdata_fwmp_create() function to vboot to do that. (Also, should probably wrap the set_space() invocation in another set_fwmp_space() just for consistency with the others.)
https://review.coreboot.org/c/coreboot/+/52919/comment/b9eda9a6_ff8d6cbd PS1, Line 244: pcr0_allowed_policy, I don't think this has any meaning when POLICY_DELETE isn't set, unless Andrey says otherwise. I think you just want (NULL, 0) here like for set_kernel_space().
File src/vendorcode/google/chromeos/Kconfig:
https://review.coreboot.org/c/coreboot/+/52919/comment/fbc28c9d_bb60bfc1 PS1, Line 95: config TPM20_CREATE_FWMP Not sure why this should be a Kconfig? Don't we just want to do this unconditionally on all future devices? (I remember discussing this with Andrey but can't find it anymore... basically, I think it would be good to just get off the custom cr50 hacks and do things this way on all devices going forward.)