Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/smm/ssm_stub: Add x86_64 support ......................................................................
cpu/smm/ssm_stub: Add x86_64 support
Enable long mode in SMM handler. x86_32 isn't affected from this change.
* Enter long mode * Add 64bit entry to GDT * Use x86_64 SysV ABI calling conventions for C code entry * Change smm_module_params' cpu to size_t as 'push' is native integer
Tested on Lenovo T410 with additional x86_64 patches.
Change-Id: I26300492e4be62ddd5d80525022c758a019d63a1 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 25 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37392/1
diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index f0e55f9..3feee9b 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -90,6 +90,10 @@ /* gdt selector 0x10, flat data segment */ .word 0xffff, 0x0000 .byte 0x00, 0x93, 0xcf, 0x00 + + /* gdt selector 0x18, flat code segment (64-bit) */ + .word 0xffff, 0x0000 + .byte 0x00, 0x9b, 0xaf, 0x00 smm_relocate_gdt_end:
.align 4 @@ -172,11 +176,30 @@ /* Align stack to 16 bytes. Another 32 bytes are pushed below. */ andl $0xfffffff0, %esp
+#ifdef __x86_64__ + /* entry64.inc preserves ebx, esi, edi */ + mov %ecx, %edi +#include <cpu/x86/64bit/entry64.inc> + mov %edi, %ecx + +#endif + /* Call into the c-based SMM relocation function with the platform * parameters. Equivalent to: * struct arg = { c_handler_params, cpu_num, smm_runtime, canary }; * c_handler(&arg) */ +#ifdef __x86_64__ + push %rbx /* uintptr_t *canary */ + push $(smm_runtime) + push %rcx /* int cpu */ + push c_handler_arg /* void *arg */ + + mov %rsp, %rdi /* *arg */ + + movl c_handler, %eax + call *%rax +#else push $0x0 /* Padding */ push $0x0 /* Padding */ push $0x0 /* Padding */ @@ -187,7 +210,7 @@ push %esp /* smm_module_params *arg (allocated on stack). */ mov c_handler, %eax call *%eax - +#endif /* Retrieve fxsave location. */ mov -4(%ebp), %edi test %edi, %edi diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index cf107b1..0de08b6 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -75,7 +75,7 @@
struct smm_module_params { void *arg; - int cpu; + size_t cpu; const struct smm_runtime *runtime; /* A canary value that has been placed at the end of the stack. * If (uintptr_t)canary != *canary then a stack overflow has occurred.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37392
to look at the new patch set (#2).
Change subject: cpu/x86/smm/ssm_stub: Add x86_64 support ......................................................................
cpu/x86/smm/ssm_stub: Add x86_64 support
Enable long mode in SMM handler. x86_32 isn't affected from this change.
* Enter long mode * Add 64bit entry to GDT * Use x86_64 SysV ABI calling conventions for C code entry * Change smm_module_params' cpu to size_t as 'push' is native integer * Drop to protected mode after c handler
Tested on Lenovo T410 with additional x86_64 patches.
Change-Id: I26300492e4be62ddd5d80525022c758a019d63a1 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 49 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37392/2
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/ssm_stub: Add x86_64 support ......................................................................
Patch Set 2:
This should be abandoned? https://review.coreboot.org/c/coreboot/+/33234 does the same thing.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/ssm_stub: Add x86_64 support ......................................................................
Patch Set 2:
Patch Set 2:
This should be abandoned? https://review.coreboot.org/c/coreboot/+/33234 does the same thing.
I don't see why. 33234 has nothing to do with x86_64 support.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/ssm_stub: Add x86_64 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S@... PS2, Line 206: %eax %rax? or does such a mov invalidate the upper part of %rax?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/ssm_stub: Add x86_64 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S@... PS2, Line 206: %eax
%rax? or does such a mov invalidate the upper part of %rax?
yes, 32bit operands invalidate the upper bits. movl is used as c_handler is 32bit only.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/ssm_stub: Add x86_64 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S@... PS2, Line 206: %eax
yes, 32bit operands invalidate the upper bits. movl is used as c_handler is 32bit only.
I'm not sure I follow. Do you mean that the c_handler lies < 4G?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/ssm_stub: Add x86_64 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S@... PS2, Line 206: %eax
yes, 32bit operands invalidate the upper bits. […]
c_handler is an u32 (.long in assembly) and thus it lies < 4G
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/ssm_stub: Add x86_64 support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37392/2//COMMIT_MSG@7 PS2, Line 7: ssm smm
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37392
to look at the new patch set (#3).
Change subject: cpu/x86/smm/smm_stub: Add x86_64 support ......................................................................
cpu/x86/smm/smm_stub: Add x86_64 support
Enable long mode in SMM handler. x86_32 isn't affected from this change.
* Enter long mode * Add 64bit entry to GDT * Use x86_64 SysV ABI calling conventions for C code entry * Change smm_module_params' cpu to size_t as 'push' is native integer * Drop to protected mode after c handler
Tested on Lenovo T410 with additional x86_64 patches.
Change-Id: I26300492e4be62ddd5d80525022c758a019d63a1 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 49 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37392/3
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 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37392/2//COMMIT_MSG@7 PS2, Line 7: ssm
smm
Done
Raul Rangel 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 3: Code-Review+2
Patrick Rudolph has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/smm_stub: Add x86_64 support ......................................................................
cpu/x86/smm/smm_stub: Add x86_64 support
Enable long mode in SMM handler. x86_32 isn't affected from this change.
* Enter long mode * Add 64bit entry to GDT * Use x86_64 SysV ABI calling conventions for C code entry * Change smm_module_params' cpu to size_t as 'push' is native integer * Drop to protected mode after c handler
Tested on Lenovo T410 with additional x86_64 patches.
Change-Id: I26300492e4be62ddd5d80525022c758a019d63a1 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 50 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37392/4
Paul Menzel 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 4:
(1 comment)
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?
Eugene Myers 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 4: Code-Review-1
(1 comment)
For security reasons, the page tables used in SMM must be located in SMRAM.
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.
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 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.
Angel Pons 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 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.
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 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.
Eugene Myers 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 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.
Patrick Rudolph has uploaded a new patch set (#5) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/smm_stub: Add x86_64 support ......................................................................
cpu/x86/smm/smm_stub: Add x86_64 support
Enable long mode in SMM handler. x86_32 isn't affected by this change.
* Enter long mode * Add 64bit entry to GDT * Use x86_64 SysV ABI calling conventions for C code entry * Change smm_module_params' cpu to size_t as 'push' is native integer * Drop to protected mode after c handler
NOTE: This commit does NOT introduce a new security model. It uses the same page tables as the remaining firmware does. This can be a security risk if someone is able to manipulate the page tables stored in ROM at runtime. USE FOR TESTING ONLY!
Tested on Lenovo T410 with additional x86_64 patches.
Change-Id: I26300492e4be62ddd5d80525022c758a019d63a1 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 46 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37392/5
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.
Patrick Rudolph has uploaded a new patch set (#6) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/smm_stub: Add x86_64 support ......................................................................
cpu/x86/smm/smm_stub: Add x86_64 support
Enable long mode in SMM handler. x86_32 isn't affected by this change.
* Enter long mode * Add 64bit entry to GDT * Use x86_64 SysV ABI calling conventions for C code entry * Change smm_module_params' cpu to size_t as 'push' is native integer * Drop to protected mode after c handler
NOTE: This commit does NOT introduce a new security model. It uses the same page tables as the remaining firmware does. This can be a security risk if someone is able to manipulate the page tables stored in ROM at runtime. USE FOR TESTING ONLY!
Tested on Lenovo T410 with additional x86_64 patches.
Change-Id: I26300492e4be62ddd5d80525022c758a019d63a1 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 46 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37392/6
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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/2/src/cpu/x86/smm/smm_stub.S@... PS2, Line 206: %eax
c_handler is an u32 (. […]
Done
Eugene Myers 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 6:
Did a closer review and realized that this patch will break STM support.
If you turn on the STM, the build should fail because start32_offset is missing. The STM initialization functions use this value to create the entry point to start the SMI handler in 32-bit mode.
Secondly, the tss segment selector (0x20) has to be there because when the STM starts the SMI handler VM, the VMX startup checks the TSS selector for proper values. Without it, the VM start will fail with an invalid guest state error.
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 6:
Patch Set 6:
Did a closer review and realized that this patch will break STM support.
If you turn on the STM, the build should fail because start32_offset is missing. The STM initialization functions use this value to create the entry point to start the SMI handler in 32-bit mode.
Secondly, the tss segment selector (0x20) has to be there because when the STM starts the SMI handler VM, the VMX startup checks the TSS selector for proper values. Without it, the VM start will fail with an invalid guest state error.
Thanks for the indepth review! Yes, there are lot of missing features. As there's no support for running on real hardware yet (or platforms supporting STM), this can be fixed in a later commit. First of all a wrapper around FSP has to be written to enable support on those platforms.
Arthur Heymans 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 6: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S@... PS6, Line 205: %eax %rax? That would be needed if TSEG is above 4G?
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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S@... PS6, Line 205: %eax
%rax? That would be needed if TSEG is above 4G?
Yes, but the Documention section about x86_64 states that this doesn't happen. Once all x86_64 patches have been merged for qemu x86_64 support, those restrictions can be removed and this code (and other parts) needs to be updated as well. However what's the benefit of having TSEG above 4G?
Raul Rangel 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 6: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S@... PS6, Line 219: rdmsr Can you back up the upper bits like you did in the previous patch?
Angel Pons 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 6: Code-Review+1
Patch Set 6:
Patch Set 6:
Did a closer review and realized that this patch will break STM support.
If you turn on the STM, the build should fail because start32_offset is missing. The STM initialization functions use this value to create the entry point to start the SMI handler in 32-bit mode.
Secondly, the tss segment selector (0x20) has to be there because when the STM starts the SMI handler VM, the VMX startup checks the TSS selector for proper values. Without it, the VM start will fail with an invalid guest state error.
Thanks for the indepth review! Yes, there are lot of missing features. As there's no support for running on real hardware yet (or platforms supporting STM), this can be fixed in a later commit. First of all a wrapper around FSP has to be written to enable support on those platforms.
Huh? "a wrapper around FSP"? Did you mean "a replacement for FSP"? 😄
Hello build bot (Jenkins), Raul Rangel, Eugene Myers, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37392
to look at the new patch set (#7).
Change subject: cpu/x86/smm/smm_stub: Add x86_64 support ......................................................................
cpu/x86/smm/smm_stub: Add x86_64 support
Enable long mode in SMM handler. x86_32 isn't affected by this change.
* Enter long mode * Add 64bit entry to GDT * Use x86_64 SysV ABI calling conventions for C code entry * Change smm_module_params' cpu to size_t as 'push' is native integer * Drop to protected mode after c handler
NOTE: This commit does NOT introduce a new security model. It uses the same page tables as the remaining firmware does. This can be a security risk if someone is able to manipulate the page tables stored in ROM at runtime. USE FOR TESTING ONLY!
Tested on Lenovo T410 with additional x86_64 patches.
Change-Id: I26300492e4be62ddd5d80525022c758a019d63a1 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 49 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37392/7
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 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S@... PS6, Line 205: %eax
Yes, but the Documention section about x86_64 states that this doesn't happen. […]
Done
https://review.coreboot.org/c/coreboot/+/37392/6/src/cpu/x86/smm/smm_stub.S@... PS6, Line 219: rdmsr
Can you back up the upper bits like you did in the previous patch?
Done
Arthur Heymans 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 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/7/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/7/src/cpu/x86/smm/smm_stub.S@... PS7, Line 190: #include <cpu/x86/64bit/entry64.inc> Maybe some idea's for moving the page tables into SMM (for follow up patches): - Because the stub is used for both relocating SMM as the actual SMM handler, add the page table offset to 'module_parameters' * On the relocation stub: Just use CONFIG_ARCH_X86_64_PGTBL_LOC * On the permanent handler: Add a region in smm_load_module() (see chart above that function definition) and copy the CONFIG_ARCH_X86_64_PGTBL_LOC there and have the permanent handler relocatable module parameter to point to that. - Turn setup_longmode into a macro with the page table location at %eax to allow other than CONFIG_ARCH_X86_64_PGTBL_LOC pagetables.
If you want I can have a go at implementing that?
Eugene Myers 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 7: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/7/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/7/src/cpu/x86/smm/smm_stub.S@... PS7, Line 190: #include <cpu/x86/64bit/entry64.inc>
Maybe some idea's for moving the page tables into SMM (for follow up patches): […]
Copying the PGTBL won't work as the pointers to the next level will be incorrect.
There exists a 4G page table generator in SmmStc.c (stm_gen_4g_pagetable_x64(uint32_t pagetable_base)) that could be used (or modified to suit).
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 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37392/7/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/7/src/cpu/x86/smm/smm_stub.S@... PS7, Line 190: #include <cpu/x86/64bit/entry64.inc>
Copying the PGTBL won't work as the pointers to the next level will be incorrect. […]
using page tables other than CONFIG_ARCH_X86_64_PGTBL_LOC sounds like a good idea. CB:30119 could be used to generate fine grained tables for SMM or ramstage.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37392 )
Change subject: cpu/x86/smm/smm_stub: Add x86_64 support ......................................................................
cpu/x86/smm/smm_stub: Add x86_64 support
Enable long mode in SMM handler. x86_32 isn't affected by this change.
* Enter long mode * Add 64bit entry to GDT * Use x86_64 SysV ABI calling conventions for C code entry * Change smm_module_params' cpu to size_t as 'push' is native integer * Drop to protected mode after c handler
NOTE: This commit does NOT introduce a new security model. It uses the same page tables as the remaining firmware does. This can be a security risk if someone is able to manipulate the page tables stored in ROM at runtime. USE FOR TESTING ONLY!
Tested on Lenovo T410 with additional x86_64 patches.
Change-Id: I26300492e4be62ddd5d80525022c758a019d63a1 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37392 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Eugene Myers cedarhouse1@comcast.net --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 49 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved Eugene Myers: Looks good to me, but someone else must approve
diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index 425724d..11ea9a7 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -10,6 +10,7 @@ */
#include <cpu/x86/cr.h> +#include <cpu/x86/msr.h>
.code32 .section ".module_parameters", "aw", @progbits @@ -148,8 +149,8 @@ pushl $0x0 mov %esp, %ebp
- /* Allocate locals (fxsave) */ - subl $0x4, %esp + /* Allocate locals (fxsave, efer_backup) */ + subl $0xc, %esp
/* calculate fxsave location */ mov fxsave_area, %edi @@ -177,22 +178,65 @@ /* Align stack to 16 bytes. Another 32 bytes are pushed below. */ andl $0xfffffff0, %esp
+#ifdef __x86_64__ + mov %ecx, %edi + /* Backup IA32_EFER. Preserves ebx. */ + movl $(IA32_EFER), %ecx + rdmsr + movl %eax, -0x8(%ebp) + movl %edx, -0xc(%ebp) + + /* entry64.inc preserves ebx, esi, edi */ +#include <cpu/x86/64bit/entry64.inc> + mov %edi, %ecx + +#endif + /* Call into the c-based SMM relocation function with the platform * parameters. Equivalent to: * struct arg = { c_handler_params, cpu_num, smm_runtime, canary }; * c_handler(&arg) */ +#ifdef __x86_64__ + push %rbx /* uintptr_t *canary */ + push $(smm_runtime) + push %rcx /* size_t cpu */ + push c_handler_arg /* void *arg */ + + mov %rsp, %rdi /* *arg */ + + movl c_handler, %eax + call *%rax + + /* + * The only reason to go back to protected mode is that RSM doesn't restore + * MSR registers and MSR IA32_EFER was modified by entering long mode. + * Drop to protected mode to safely operate on the IA32_EFER MSR. + */ + + /* Disable long mode. */ + #include <cpu/x86/64bit/exit32.inc> + + /* Restore IA32_EFER as RSM doesn't restore MSRs. */ + movl $(IA32_EFER), %ecx + rdmsr + movl -0x8(%ebp), %eax + movl -0xc(%ebp), %edx + + wrmsr + +#else push $0x0 /* Padding */ push $0x0 /* Padding */ push $0x0 /* Padding */ push %ebx /* uintptr_t *canary */ push $(smm_runtime) - push %ecx /* int cpu */ + push %ecx /* size_t cpu */ push c_handler_arg /* void *arg */ push %esp /* smm_module_params *arg (allocated on stack). */ mov c_handler, %eax call *%eax - +#endif /* Retrieve fxsave location. */ mov -4(%ebp), %edi test %edi, %edi diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index db63e8b..de16a43 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -75,7 +75,7 @@
struct smm_module_params { void *arg; - int cpu; + size_t cpu; const struct smm_runtime *runtime; /* A canary value that has been placed at the end of the stack. * If (uintptr_t)canary != *canary then a stack overflow has occurred.
Bill XIE 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 8:
(1 comment)
Patchset:
PS8: It seems that the SMM stack canary introduced in https://review.coreboot.org/c/coreboot/+/27229 was broken by this commit. Now it is nearly guaranteed that if CONFIG_DEBUG_SMI is set, smm will die() with "SMM Handler caused a stack overflow" (which is quite possibly a false positive) on x86 platforms.
Attention is currently required from: Patrick Rudolph. Angel Pons 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 8:
(1 comment)
File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/37392/comment/38602d6f_25f23bf4 PS8, Line 153: subl $0xc, %esp This seems suspicious
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Bill XIE (Code Review)
-
Eugene Myers (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)
-
ron minnich (Code Review)