Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/smm_stub: Add x86_64 support ......................................................................
Patch Set 5:
(2 comments)
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4: Code-Review-1
(1 comment)
For security reasons, the page tables used in SMM must be located in SMRAM.
Please explain your threat model and why it is a security issue if the page table are not in SMRAM.
I imagine it is because th SMM page tables can then be accessed from outside SMM, which could be exploited to escalate privileges to SMM. Which would not be fun.
As page tables currently reside in ROM, it's as secure as the remaining firmware code. If someone is able to change that, the one already has full control over the system on next reboot, so caring about a firmware substate here is futile.
I'll add that to the documentation as TODO, but it's not worth touching the SMM relocation code for a PoC that only runs on emulated hardware.
Though the ROM based page tables are immutable, the SMM page tables should be specific to the code the runs in SMM. I've pulled and rephrased to coreboot needs a couple of the requirements from the Microsoft "System Guard Secure Launch and SMM protection" (https://docs.microsoft.com/en-us/windows/security/threat-protection/windows-...) to illustrate:
(1) Must not have execute and write permissions for the same page. (2) Must allow only that TSEG pages can be marked executable.
The short requirement here is make sure that SMM code cannot execute outside of SMRAM.
Another issue with the ROM based pages tables (on x86 processors) is the processor will try to write to page tables because it wants to set the A and D bits (access and dirty). Early in the XHIM development on STM/PE, I tried simulating ROM by setting the EPT pages to R/O. The result was a EPT violation because the processor wanted to write to the page table.
The method to mitigate this is to preset the A and D bits in the page table so that the processor will not attempt to write to the page tables. (http://vzimmer.blogspot.com/2013/02/32-versus-64-bit-and-measuring-uefi.html)
I checked how the page tables are generated (pgtblgen.c) and the A and D bite are not set and I would suggest the code be modified to set those bits.
Updated documentation in CB:42982 and added security warnings in the commit message, that this code must not be used in production.
Updated pgtblgen to have A and D bit set in CB:42981.
https://review.coreboot.org/c/coreboot/+/37392/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37392/4//COMMIT_MSG@10 PS4, Line 10: from
by?
Done
https://review.coreboot.org/c/coreboot/+/37392/4/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/4/src/cpu/x86/smm/smm_stub.S@... PS4, Line 216: #include <cpu/x86/64bit/exit32.inc>
For security reasons, the 64 bit page-tables that are used in SMM must be located in SMRAM.
Out of scope of this commit.