Attention is currently required from: Miriam Polzer, Andrey Pronin, Yu-Ping Wu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59097 )
Change subject: security/vboot: Add NVRAM counter for TPM 2.0 ......................................................................
Patch Set 2:
(7 comments)
Patchset:
PS2: I'm not really convinced this is necessary yet... please see my comment on your design doc.
File src/security/vboot/antirollback.h:
https://review.coreboot.org/c/coreboot/+/59097/comment/5c13a025_7e9b0683 PS2, Line 31: #define ENT_ROLLBACK_NV_INDEX 0x100e This should probably have COUNTER in the name (e.g. ENT_ROLLBACK_COUNTER_INDEX, analogous to ZTE_RMA_BYTES_COUNTER_INDEX).
File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/59097/comment/974c97f6_ee675cb8 PS2, Line 113: const static TPMA_NV rw_counter_attributes = { I would suggest adding PPREAD/PPWRITE here as well even if your design doesn't need it, just in case it comes in handy in a future change.
https://review.coreboot.org/c/coreboot/+/59097/comment/d2f1d5f8_bf7c0573 PS2, Line 116: .TPMA_NV_PLATFORMCREATE = 1, Similarly, might as well set WRITE_STCLEAR too. It shouldn't hurt, and it might come in handy if we want to change how we use this in the future.
https://review.coreboot.org/c/coreboot/+/59097/comment/1270877f_7fb30699 PS2, Line 189: * Empty policy digest. What's this? AFAIK you should only need a policy if TPMA_NV_POLICY_DELETE is set, which it isn't for your space. (That means the space may be deleted with platform authorization, which I think should be fine in your use case.) For spaces that don't need a policy, just pass NULL, 0 to setup_space().
https://review.coreboot.org/c/coreboot/+/59097/comment/ff9c113b_069d4aae PS2, Line 374: This is where you should set up this space, guarded by if (CONFIG(CHROMEOS))
File src/security/vboot/vboot_logic.c:
https://review.coreboot.org/c/coreboot/+/59097/comment/b0651228_1a7d008a PS2, Line 281: enterprise_rollback_create_counter(); Spaces should be created from factory_initialize_tpm(), not here. If you want to backport this to shipped devices you can make a change in the respective firmware branches to call it from somewhere else, but ToT should be written from the perspective of clean, new devices without hacks to migrate from an older state.