Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37016 )
Change subject: security/intel/txt: Add Intel TXT support ......................................................................
Patch Set 7: Code-Review+1
(42 comments)
https://review.coreboot.org/c/coreboot/+/37016/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37016/7//COMMIT_MSG@24 PS7, Line 24: Tested on OCP Wedge100s and Facebook Watson AFAIK both of these are Broadwell-DE. Got any other platform to test TXT on master?
https://review.coreboot.org/c/coreboot/+/37016/7//COMMIT_MSG@31 PS7, Line 31: Signed-off-by: Philipp Deppenwiese zaolin@das-labor.org nit: Are both sign-offs needed?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... File src/security/intel/txt/common.c:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 53: >> Are these aligned to something in particular?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 65: progress); nit: fits in 96 characters
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 95: ! Can we drop this negation and swap the branches instead?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 135: 4 What's this magic number?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 143: acm_header nit: shortening this to `acm` would improve readdability, IMHO.
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 147: -1 Shouldn't these return codes be defined?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 183: 0xaa, 0x3a, 0xc0, 0x7f, 0xa7, 0x46, 0xdb, 0x18, 0x2e, 0xac, 0x69, 0x8f, 0x8d, I'd balance both lines so that they have the same number of elements:
0xaa, 0x3a, 0xc0, 0x7f, 0xa7, 0x46, 0xdb, 0x18, 0x2e, 0xac, 0x69, 0x8f, 0x8d, 0x41, 0x7f, 0x5a,
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 274: no space before ! (here and the previous line)
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 274: cold reboot Who does the cold reboot?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 286: are required TXT in general hrm, doesn't parse. Maybe "are required *for* TXT in general" ?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 287: Enables Enable
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 299: if (ecx & CPUID_SMX) { : printk(BIOS_DEBUG, "true\n"); : } else { : printk(BIOS_DEBUG, "false\n"); : failure = true; : } Since this pattern repeats a lot, maybe make a helper function?
/* Returns true if cond is not met */ static bool check_precondition(const int cond) { printk(BIOS_DEBUG, "%s\n", cond ? "true" : "false"); return !cond; }
Then use it in the code as follows:
failure |= check_precondition(ecx & CPUID_VMX);
That it needs to take an int is ugly, but prevents truncation on bit test results. If you have a better idea (or don't mind using !!(cond) when calling this), feel free to use it.
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 316: locked If it's not locked, isn't that a serious error? I'd expect that TXT is enabled after the MSR is locked. IMO, I'd bail here instead of rewriting the MSR
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 363: !(ecx & CPUID_SMX) This was tested already, at the beginning
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 364: return true; Maybe add some message?
TXT: Preconditions not met, bailing.
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 377: capabable spurious `ab` in `capable`
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 425: policy Which policy?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 445: 0x17a Isn't there any define for this MSR?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/comm... PS7, Line 461: txt_feature_flags if `getsec_parameter` failed, isn't this value garbage (undefined)?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... File src/security/intel/txt/getsec.c:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 65: 0x1f What does this 0x1f mean?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... File src/security/intel/txt/getsec_enteraccs.S:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 66: double empty line
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 118: 1 Oh, the macros use these labels? I'd give them more descriptive names so that it's easier to understand the code. Or even better, turn the variable MTRR macros into a loop?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 191: 1: Where is this used?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 192: GPE GPE or GP? Or GPF? GPE reminds me of General Purpose Event and I get confused
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/gets... PS7, Line 245: 1: I think this one is used right above?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... File src/security/intel/txt/logging.c:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 18: static void log_txt_error(const char *phase) Are these prints meant for preproduction only? I see some of these strings in `intel_txt_log_acm_error` too
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 146: (acm_header->size & 0xffffff) << 2) acm_size
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 150: printk(BIOS_INFO, " Flags: PW signed (Production Worthy)\n"); One tab too much?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 161: Cooperation Corporation?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 227: nit: since SINIT is one character longer than HEAP and MSEG, add one space so that the numbers are aligned?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/logg... PS7, Line 228: read64((void *)TXT_HEAP_SIZE)); I think these all fit in 96 characters. Also, I'd print base, then size (it's more common)
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... File src/security/intel/txt/ramstage.c:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 29: /* MSEG SIZE and MSEG BASE might contain random values. : * Assume below 4GiB and 8byte aligned. : */ Comment should start like this:
/* * MSEG_SIZE ...
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 101: sobber "sober"?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 152: Failed to prepare TXT environment Maybe update this message to distinguish it from the case above?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 165: . add:
with NOP function
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/rams... PS7, Line 293: "CBFS.\n"); Fits in 96 characters
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... File src/security/intel/txt/txt_getsec.h:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... PS7, Line 6: Double empty line
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... File src/security/intel/txt/txt_register.h:
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... PS7, Line 73: /* Only present in Document Number: 315168-010 and 315168-016 */ TXT_VER_QPIIF is also present on 322372-002 and 315168-011
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... PS7, Line 131: #define TXT_FILE_LCP_PD_ACM "txt_bios_policy.bin" hrm, is there a better place for CBFS filenames? That way, one doesn't have to keep them in sync with e.g. Makefiles. Maybe Kconfig?
https://review.coreboot.org/c/coreboot/+/37016/7/src/security/intel/txt/txt_... PS7, Line 205: __packed nit: putting `__packed` before the opening brace of the struct should result in a build failure in case `__packed` isn't defined. Currently, if `__packed` isn't defined, this creates a global struct variable named `__packed`. Yes, it would conflict with the other structs here, but better safe than sorry 😄