Attention is currently required from: Subrata Banik, Jérémy Compostella, Sridhar Siricilla, Angel Pons, Lean Sheng Tan, Werner Zeh.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/71574 )
Change subject: security/intel/txt: Helper function to disable TXT ......................................................................
Patch Set 5:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/71574/comment/2318d419_9e53bd1c PS5, Line 6:
Possible long commit subject (prefer a maximum 65 characters)
This is a false warning as UTF-8 string markers are added for some reason.
``` ### checkpatch.out ### WARNING: Possible long commit subject (prefer a maximum 65 characters) #4: Subject: [PATCH] =?UTF-8?q?security/intel/txt:=20Helper=20function=20to=20?= ```
https://review.coreboot.org/c/coreboot/+/71574/comment/8194ef6f_f5b2f4e6 PS5, Line 7: security/intel/txt: Helper function to disable TXT Please make it a statement: Add helper function to disable TXT
https://review.coreboot.org/c/coreboot/+/71574/comment/023304b0_a78a3b20 PS5, Line 9: This patch Add a function to disable TXT as per …
https://review.coreboot.org/c/coreboot/+/71574/comment/a4c1e73c_6bdc18d2 PS5, Line 12: On platform with TXT disabled, the memory can be unlocked using platforms
File src/security/intel/txt/txtlib.c:
https://review.coreboot.org/c/coreboot/+/71574/comment/bd904b2b_b410796a PS5, Line 57: /* Don't disable if INTEL_TXT config is selected */ I’d say the comment is redundant, as the code is clear.
https://review.coreboot.org/c/coreboot/+/71574/comment/026e391c_cfcf65cb PS5, Line 59: return; I’d check that on the caller side. If I run `disable_intel_txt()` I’d expect it to disable TXT independent of the configuration option.
https://review.coreboot.org/c/coreboot/+/71574/comment/3d691387_acca28e8 PS5, Line 63: printk(BIOS_INFO, "CPU is not TXT capable.\n"); No idea how the function is supposed to be called, but I’d say something is wrong, if it’s called on a non-TXT capable CPU? (Maybe print the CPU model?)
https://review.coreboot.org/c/coreboot/+/71574/comment/5c6d271b_116f43c8 PS5, Line 70: security(TPM) Add a space before the (?
https://review.coreboot.org/c/coreboot/+/71574/comment/e6c88340_4e84453e PS5, Line 74: printk(BIOS_INFO, "TXT disabled successfully- Unlock Memory\n"); 1. Add a space before the -? 2. Maybe: Unlock*ed* *m*emory
-
Paul Menzel (Code Review)