Attention is currently required from: Jérémy Compostella, Paul Menzel, Sridhar Siricilla, Angel Pons, Lean Sheng Tan, Werner Zeh.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/71574 )
Change subject: security/intel/txt: Add helper function to disable TXT ......................................................................
Patch Set 5:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/71574/comment/4ccaf0cd_687eee9e PS5, Line 7: security/intel/txt: Helper function to disable TXT
Please make it a statement: Add helper function to disable TXT
Ack
https://review.coreboot.org/c/coreboot/+/71574/comment/1063e6bd_e589f86a PS5, Line 9: This patch
Add a function to disable TXT as per …
Done
https://review.coreboot.org/c/coreboot/+/71574/comment/935681f4_d77dbdd1 PS5, Line 12: On platform with TXT disabled, the memory can be unlocked using
platforms
Done
File src/security/intel/txt/txtlib.c:
https://review.coreboot.org/c/coreboot/+/71574/comment/878799ee_39473464 PS5, Line 57: /* Don't disable if INTEL_TXT config is selected */
I’d say the comment is redundant, as the code is clear.
Done
https://review.coreboot.org/c/coreboot/+/71574/comment/a7aee487_52548c45 PS5, Line 59: return;
I’d check that on the caller side. […]
Done
https://review.coreboot.org/c/coreboot/+/71574/comment/5deea4a5_797a5327 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
Can you please elaborate, what you mean by *something is wrong*. Typically, low end SoCs doesn't have TXT feature enabled, and this is generic function hence, we should have a check in place to know if the said SoC has TXT support enabled. Without that, it's meaningless to try disabling TXT on a SKU where this feature itself is not present.
if it’s called on a non-TXT capable CPU? (Maybe print the CPU model?)
SKU here CPU is TXT unsupported ``` [NOTE ] coreboot-coreboot-unknown.9999.31198eb Thu Dec 29 06:07:08 UTC 2022 bootblock starting (log level: 8)... [DEBUG] CPU: 12th Gen Intel(R) Core(TM) i3-1215U [DEBUG] CPU: ID 906a4, Alderlake R0 Platform, ucode: 00000423 [DEBUG] CPU: AES supported, TXT NOT supported, VT supported ```
same board with different SOC SKU
``` coreboot-coreboot-unknown.9999.408fbd4 Fri Dec 30 01:05:08 UTC 2022 bootblock starting (log level: 8)... CPU: 12th Gen Intel(R) Core(TM) i7-1265U CPU: ID 906a4, Alderlake R0 Platform, ucode: 00000423 CPU: AES supported, TXT supported, VT supported ```
Not sure what you mean by print the CPU model. Ideally I would avoid adding a print. The purpose of this API is to turn off TXT memory protection hence, any additional output msg is meaningless (this function is returning void)
https://review.coreboot.org/c/coreboot/+/71574/comment/714de0a2_69db3b42 PS5, Line 70: security(TPM)
Add a space before the (?
Done
https://review.coreboot.org/c/coreboot/+/71574/comment/9aba9bea_51e6a522 PS5, Line 74: printk(BIOS_INFO, "TXT disabled successfully- Unlock Memory\n");
- Add a space before the -? […]
Done
-
Subrata Banik (Code Review)