Attention is currently required from: Andrey Pronin, Raul Rangel, Julius Werner, Yu-Ping Wu, Karthik Ramasubramanian. Andrey Pronin 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:
(4 comments)
File src/security/vboot/secdata_tpm.c:
https://review.coreboot.org/c/coreboot/+/59476/comment/4b43b225_fbee9878 PS1, Line 159: .TPMA_NV_PPWRITE = 1,
Similar to the discussion in https://review.coreboot.org/c/coreboot/+/59097/3.. […]
good point. yes, NO_DA makes sense
https://review.coreboot.org/c/coreboot/+/59476/comment/715ccfe3_83de2741 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 exi […]
actually, it may exist. if factory init was interrupted after creating this index, but before creating the f/w index, it will be there.
https://review.coreboot.org/c/coreboot/+/59476/comment/997f5f7e_f5a290bf PS1, Line 369: !=
Should we check for TPM_SUCCESS instead? I'm concerned this will let other errors through.
!= TPM_E_BADINDEX also makes sure that the index is created and written. but I agree shall be careful with error handling not to block factory init if the counter was created but not written (sudden reset between).
https://review.coreboot.org/c/coreboot/+/59476/comment/27ef8056_6fd1046a 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 confi […]
yes, the firmware space shall always be the last since its presence is the "factory init complete" check.