Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Angel Pons, Morgan Jang, Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54093 )
Change subject: security/intel/cbnt: Add logging ......................................................................
Patch Set 7:
(12 comments)
File src/security/intel/cbnt/Kconfig:
https://review.coreboot.org/c/coreboot/+/54093/comment/a094a74e_d4c9a93d PS6, Line 20: config INTEL_CBNT_LOGGING
Would it make sense to use the same TXT option? This way, we avoid having two separate options.
I did not want to touch the TXT logging as it is mostly wrong for CBnT and maybe even for TXT, as I can't correlate the code to datasheets I have.
Also, this option is currently never build-tested.
I'll just always compile the logging and make the calling dependent on the Kconfig option.
I'll also add CBnT buildtesting. Some changes were needed in cbtn-prov as it did not work without IFD.
File src/security/intel/cbnt/cbnt.h:
https://review.coreboot.org/c/coreboot/+/54093/comment/027a06dd_b89f391c PS6, Line 3: 0xfed30000
Hmmm, but this matches TXT_PUBLIC_SPACE
Done
File src/security/intel/cbnt/logging.c:
https://review.coreboot.org/c/coreboot/+/54093/comment/57f3ee1f_174ffbb3 PS6, Line 14: nem
nit: nem_enabled
Done
https://review.coreboot.org/c/coreboot/+/54093/comment/6245c6f8_7ce9f761 PS6, Line 16: tpm_succes
missing one s: tpm_succes*s*
Done
https://review.coreboot.org/c/coreboot/+/54093/comment/3d6f0b31_dd836212 PS6, Line 17: reserved1
On CFL, this is `facb`
Done
https://review.coreboot.org/c/coreboot/+/54093/comment/42a24156_fd524b4c PS6, Line 32:
no space after `*`.
Done
https://review.coreboot.org/c/coreboot/+/54093/comment/bc3da29b_949393b6 PS6, Line 103: const
This `const` is meaningless. […]
Done
https://review.coreboot.org/c/coreboot/+/54093/comment/003dc538_1af5c191 PS6, Line 106: *btg = "Boot Guard Error", *reserved = "Reserved";
You don't need these static variables here. String literals are already static and constant.
Thanks.
https://review.coreboot.org/c/coreboot/+/54093/comment/7e6e8cf6_adcac3ca PS6, Line 124: | ((uint64_t)sacm_info_msr.hi << 32)};
Why not add `lo` and `hi` to `union sacm_info`?
const union sacm_info acm_info = { .lo = sacm_info_msr.lo, .hi = sacm_info_msr.hi, };
Oh wait, sacm_info is a MSR. Then, simply add a `msr_t` union field:
union sacm_info_msr { struct { uint64_t nem : 1; uint64_t tpm_type : 2; uint64_t tpm_succes : 1; uint64_t reserved1 : 1; uint64_t measured_boot : 1; uint64_t verified_boot : 1; uint64_t revoked : 1; uint64_t : 24; uint64_t btg_cap : 1; uint64_t : 1; uint64_t txt_cap : 1; uint64_t : 29; }; msr_t msr; uint64_t raw; };
Then, just initialise it directly:
const union sacm_info acm_info = { .msr = rdmsr(MSR_BOOT_GUARD_SACM_INFO), };
Nice.
https://review.coreboot.org/c/coreboot/+/54093/comment/600d860d_9c00ffc8 PS6, Line 126: CBNT:\tSACM INFO:
I wouldn't print this on each and every line. Just indent the lines after the first instance. […]
Done
https://review.coreboot.org/c/coreboot/+/54093/comment/d097c452_bafb184b PS6, Line 139: read64((void *)CBNT_BOOTSTATUS)
nit: read64p(CBNT_BOOTSTATUS) […]
Done
https://review.coreboot.org/c/coreboot/+/54093/comment/8ac31c2f_36fc40a4 PS6, Line 157: if (txt_err_valid) { : if (!btsts.txt_dis_pol) {
Join these two ifs to save one indentation level? […]
Done
-
Arthur Heymans (Code Review)