Attention is currently required from: Andrey Pronin, Yu-Ping Wu, Karthik Ramasubramanian. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59476 )
Change subject: src/security/vboot: Setup secure counter space in TPM NVRAM ......................................................................
Patch Set 1:
(7 comments)
File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/59476/comment/a53a2d50_78579711 PS1, Line 286: bool "Define Secure Counters in Verstage" Not sure this needs to be menuconfig-visible? I'd suggest just `select`ing it from SoC Kconfig `if CHROMEOS`.
File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/59476/comment/f9a97568_7f906664 PS1, Line 151: static const TPMA_NV secure_counter_attr = { In the interest of trying to keep these attributes function-describing and potentially reusable, I'd maybe call this rw_orderly_counter_attr.
https://review.coreboot.org/c/coreboot/+/59476/comment/d436a8db_576d70bb PS1, Line 159: .TPMA_NV_PPWRITE = 1, Similar to the discussion in https://review.coreboot.org/c/coreboot/+/59097/3..6/src/security/vboot/secda..., we have been setting NO_DA on all other counters for now, and the same arguments would probably apply to this one?
https://review.coreboot.org/c/coreboot/+/59476/comment/d5a8420e_c285d120 PS1, Line 355: return tlcl_nv_increment(index); Does the counter need to be incremented in firmware? If the userspace code that needs it just increments it on first use, we can avoid having to pull support for the increment command into firmware (that's how we have been dealing with the existing counters for now).
https://review.coreboot.org/c/coreboot/+/59476/comment/30a275a9_41eb6044 PS1, Line 368: rv = tlcl_read(index, &value, SECURE_COUNTER_SIZE); What's the point of this check? When we're going through factory init, this counter should never exist already.
https://review.coreboot.org/c/coreboot/+/59476/comment/2c57e186_81cd17a4 PS1, Line 416: RETURN_ON_FAILURE(setup_secure_counter_spaces()); The firmware space should always be the last one that is created, since this code uses that to confirm that factory initialization was fully completed. Need to move this call further up.
(Also, this is just aesthetics but I think it would be easier to follow and more consistent with other optional spaces to put the if (CONFIG(VBOOT_DEFINE_SECURE_COUNTERS)) right here, rather than hide it inside the function.)
File src/security/vboot/secure_counter.h:
https://review.coreboot.org/c/coreboot/+/59476/comment/a7e1a8de_3ba960c3 PS1, Line 14: SECURE_COUNTER4_NV_INDEX, /* 0x1012 */ These should be defined with the other TPM space indices in <antirollback.h>, so that we have them all in one place. I don't think you need this extra header.
Also, this name should be precise enough to make it's purpose clear and avoid potential future name clashes. "secure counter" could be anything. Maybe call it widevine_psp_counter or something?
-
Julius Werner (Code Review)