Attention is currently required from: Andrey Pronin, Raul Rangel, Paul Menzel, Julius Werner, Yu-Ping Wu. Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59476 )
Change subject: src/security/vboot: Set up secure counter space in TPM NVRAM ......................................................................
Patch Set 2:
(13 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59476/comment/73491284_c530a4a1 PS1, Line 7: Setup
How about adding a presubmit hook ? ;)
Done. Split for now.
https://review.coreboot.org/c/coreboot/+/59476/comment/cdd8946b_5602c29c PS1, Line 35: 72057594037927936
ah, should we adjust the endienness when printing? I guess it sounds like we are removing the increm […]
Ack. Removed read and increment operations for now.
File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/59476/comment/cd1542d3_0fe6cde9 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 C […]
Ack. Removed the prompt so that it is an invisible symbol.
File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/59476/comment/bdb5a4f8_6f1cb2da 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 […]
Done
https://review.coreboot.org/c/coreboot/+/59476/comment/c1a44d92_443f9e50 PS1, Line 159: .TPMA_NV_PPWRITE = 1,
good point. […]
Done
https://review.coreboot.org/c/coreboot/+/59476/comment/c55580eb_5354b9b1 PS1, Line 349: SECURE_COUNTER_NAME
This name is all local to coreboot. I dont think it is passed to TPM. Hence chosen a generic name.
Done
https://review.coreboot.org/c/coreboot/+/59476/comment/e2702905_b639f599 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 increm […]
Ack. Removed increment operation.
https://review.coreboot.org/c/coreboot/+/59476/comment/2d8abeab_85998135 PS1, Line 368: rv = tlcl_read(index, &value, SECURE_COUNTER_SIZE);
Yes, increment operation can be done in the first use. […]
Done. Define space can identify if a space is already defined. So leveraging that instead of an explicit read operation.
https://review.coreboot.org/c/coreboot/+/59476/comment/5d0e55fa_997a6c7b PS1, Line 369: !=
I will handle the error accordingly based on the discussion in line 368.
Now that we are not performing read operation to check if a space is already defined, I have removed any associated error checks.
https://review.coreboot.org/c/coreboot/+/59476/comment/422f7ed8_1116c97f PS1, Line 386: * Of all NVRAM spaces defined by this function the firmware space : * must be defined last, because its existence is considered an : * indication that TPM factory initialization was successfully : * completed.
Julius/Andrey confirmed that firmware_space should be the last and hence asked to move counter space […]
Done
https://review.coreboot.org/c/coreboot/+/59476/comment/e6afe5cf_13a98064 PS1, Line 416: RETURN_ON_FAILURE(setup_secure_counter_spaces());
yes, the firmware space shall always be the last since its presence is the "factory init complete" c […]
Done
File src/security/vboot/secure_counter.h:
https://review.coreboot.org/c/coreboot/+/59476/comment/af2c93c5_8d386d7f PS1, Line 7: 8
Would doing a sizeof(uint64_t) make this more clear?
Done
https://review.coreboot.org/c/coreboot/+/59476/comment/55272970_11b0c84c PS1, Line 14: SECURE_COUNTER4_NV_INDEX, /* 0x1012 */
These should be defined with the other TPM space indices in <antirollback. […]
Done