Change in coreboot[master]: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub This adds a check for LAPIC base twice. One very early check when the CPU is still executing in real mode checks if the LAPIC base is inside the region [smmbase,smmbase + SMM_DEFAULT_SIZE). This cannot use anything but a hardcoded size since even accessing the relocatable parameters is impossible in the state of the CPU. The actual SMI handler is located above smmbase + SMM_DEFAULT_SIZE and before jumping to it the LAPIC base is checked against the whole SMM region. Given that we have a working stack at this point, this is done in C code. UNTESTED. Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/sinkhole.c M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 4 files changed, 87 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37289/1 diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index 11a4e67..b94f11e 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -45,6 +45,7 @@ postcar-y += tseg_region.c smmstub-y += smm_stub.S +smmstub-y += sinkhole.c smm-y += smm_module_handler.c diff --git a/src/cpu/x86/smm/sinkhole.c b/src/cpu/x86/smm/sinkhole.c new file mode 100644 index 0000000..773549c --- /dev/null +++ b/src/cpu/x86/smm/sinkhole.c @@ -0,0 +1,34 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <cpu/x86/lapic_def.h> +#include <cpu/x86/msr.h> +#include <cpu/x86/smm.h> + +const extern struct smm_runtime smm_runtime; + +/* Don't try to recover because, if somebody tried to do shenanigans like + these, we have to expect more. */ +void mitigate_sinkhole(void) +{ + msr_t lapic_base_msr = rdmsr(LAPIC_BASE_MSR); + uintptr_t lapic_base = lapic_base_msr.lo & LAPIC_BASE_MSR_ADDR_MASK; + + const uintptr_t smm_end = smm_runtime.smbase + smm_runtime.smm_size; + + if (lapic_base > smm_runtime.smbase && lapic_base < smm_end) { + printk(BIOS_EMERG, "Wrong LAPIC base detected! dying...\n"); + asm("ud2"); + } +} diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index 304ea4b..1d39ae1 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -22,6 +22,7 @@ */ #include <cpu/x86/cr.h> +#include <cpu/x86/lapic_def.h> .code32 .section ".module_parameters", "aw", @progbits @@ -39,6 +40,7 @@ fxsave_area_size: .long 0 /* struct smm_runtime begins here. */ +.global smm_runtime smm_runtime: smbase: .long 0 @@ -63,10 +65,51 @@ (CR0_CD | CR0_NW | CR0_PG | CR0_AM | CR0_WP | \ CR0_NE | CR0_TS | CR0_EM | CR0_MP) +#define SMM_DEFAULT_SIZE 0x10000 + .text .code16 .global _start _start: +#if CONFIG(SMM_LAPIC_REMAP_MITIGATION) + /* Check if the LAPIC register block overlaps with SMM. + * This block needs to work without data accesses because they + * may be routed into the LAPIC register block. + * Code accesses, on the other hand, are never routed to LAPIC, + * which is what makes this work in the first place. + * This is a mitigation against the sinkhole vulnerability + * possible on pre sandy bridge Intel hardware. + */ + mov $LAPIC_BASE_MSR, %ecx + rdmsr + and $(~0xfff), %eax + sub $_start, %eax + /* This might cover a bit more than needed but in the case that + this is a stub to the relocated, permanent handler it should + cover some parts of the permanent handler too, so no harm is + done. In the case that this is a stub to a relocation handler + coreboot did something horribly wrong if LAPIC is here. + This has to be 'hardcoded' as the CPU is running in real mode + at this point so it cannot access any relocatable variable. */ + cmp $SMM_DEFAULT_SIZE, %eax + ja untampered_lapic +1: + /* emit "Crash" on serial */ + mov $(CONFIG_TTYS0_BASE), %dx + mov $'C', %al + out %al, (%dx) + mov $'r', %al + out %al, (%dx) + mov $'a', %al + out %al, (%dx) + mov $'s', %al + out %al, (%dx) + mov $'h', %al + out %al, (%dx) + /* now crash for real */ + ud2 +untampered_lapic: +#endif movl $(smm_relocate_gdt), %ebx lgdtl (%ebx) @@ -174,6 +217,12 @@ /* Align stack to 16 bytes. Another 32 bytes are pushed below. */ andl $0xfffffff0, %esp +#if CONFIG(SMM_LAPIC_REMAP_MITIGATION) + /* Now that we have a stack, check for against lapic base in + C code before jumping to the permanent handler */ + call mitigate_sinkhole +#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 }; diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index 2e3c639..884d5cb 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -89,6 +89,9 @@ /* SMM Runtime helpers. */ +/* SMM Runtime sinkhole mitigation check */ +void mitigate_sinkhole(void); + /* Entry point for SMM modules. */ asmlinkage void smm_handler_start(void *params); -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-MessageType: newchange
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/sinkhole.c File src/cpu/x86/smm/sinkhole.c: https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/sinkhole.c@... PS1, Line 31: printk(BIOS_EMERG, "Wrong LAPIC base detected! dying...\n"); hmm do we really want to link console code into the stub? -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 27 Nov 2019 17:45:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 1: (2 comments) Neat strategy to use the early stack setup to check the later stuff. The stack is within the safe area, right? https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/sinkhole.c File src/cpu/x86/smm/sinkhole.c: https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/sinkhole.c@... PS1, Line 31: printk(BIOS_EMERG, "Wrong LAPIC base detected! dying...\n");
hmm do we really want to link console code into the stub? We don't need to be too friendly about this as there's really no reason for anybody to do this legitimately: Do weird stuff, get hurt. I'm fine with that.
Because of that I'd go with the crash code we already have. Maybe add a symbol there (added a comment at that place) and use that to jump there from here? https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S: https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/smm_stub.S@... PS1, Line 96: 1: I'd just jump here from the later test in mitigate sinkhole. -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 27 Nov 2019 18:16:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S: https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/smm_stub.S@... PS1, Line 96: 1:
I'd just jump here from the later test in mitigate sinkhole.
This is 16bit code. Does that work? -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 27 Nov 2019 18:24:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Georgi <pgeorgi@google.com> Gerrit-MessageType: comment
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S: https://review.coreboot.org/c/coreboot/+/37289/1/src/cpu/x86/smm/smm_stub.S@... PS1, Line 96: 1:
I'd just jump here from the later test in mitigate sinkhole. […] oops, you're right, I missed that.
Doing a quick test with gnu as, it would still almost work out by accident though: the first two moves would be conflated into one (into edx, the upper 16 bit containing the opcode for the second move), so instead we'd get ?rash, with the first value being whatever happened to be in %al at the time. Anybody trying to debug that would probably go crazy though. -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 27 Nov 2019 18:53:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Comment-In-Reply-To: Patrick Georgi <pgeorgi@google.com> Gerrit-MessageType: comment
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/37289 to look at the new patch set (#2). Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub This adds a check for LAPIC base twice. There is a very early check when the CPU is still executing in real mode checks if the LAPIC base is inside the region [smmbase,smmbase + SMM_DEFAULT_SIZE). The CPU cannot use anything but a hardcoded size since even accessing the relocatable parameters is impossible in the state of the CPU. The actual SMI handler is located above smmbase + SMM_DEFAULT_SIZE and before jumping to it the LAPIC base is checked against the whole SMM region. Given that we have a working stack at this point, this is done in C code. UNTESTED. Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/sinkhole.c M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 4 files changed, 84 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37289/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/37289 to look at the new patch set (#3). Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub This adds a check for LAPIC base twice. There is a very early check when the CPU is still executing in real mode checks if the LAPIC base is inside the region [smmbase,smmbase + SMM_DEFAULT_SIZE). The CPU cannot use anything but a hardcoded size since even accessing the relocatable parameters is impossible in the state of the CPU. The actual SMI handler is located above smmbase + SMM_DEFAULT_SIZE and before jumping to it the LAPIC base is checked against the whole SMM region. Given that we have a working stack at this point, this is done in C code. UNTESTED. Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/cpu/x86/smm/Makefile.inc A src/cpu/x86/smm/sinkhole.c M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 4 files changed, 84 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37289/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 3: (1 comment) https://review.coreboot.org/c/coreboot/+/37289/3/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S: https://review.coreboot.org/c/coreboot/+/37289/3/src/cpu/x86/smm/smm_stub.S@... PS3, Line 218: $0xfffffff0, %esp stack is located above DEFAULT_SMM_SIZE, so the check needs to happen before accessing the stack. -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 3 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 29 Nov 2019 12:18:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/37289 to look at the new patch set (#4). Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub This adds a check for LAPIC base twice. There is a very early check when the CPU is still executing in real mode checks if the LAPIC base is inside the region [smmbase,smmbase + SMM_DEFAULT_SIZE). The CPU cannot use anything but a hardcoded size since even accessing the relocatable parameters is impossible in the state of the CPU. After the CPU operates in protected mode the relocatable parameters are accessible and are used the check for the full smm region. Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 2 files changed, 52 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37289/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 4 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Hello Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/37289 to look at the new patch set (#5). Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub This adds a check for LAPIC base twice. There is a very early check when the CPU is still executing in real mode checks if the LAPIC base is inside the region [smmbase,smmbase + SMM_DEFAULT_SIZE). The CPU cannot use anything but a hardcoded size since even accessing the relocatable parameters is impossible in the state of the CPU. After the CPU operates in protected mode the relocatable parameters are accessible and are used the check for the full smm region. Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/cpu/x86/smm/smm_stub.S 1 file changed, 49 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37289/5 -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 5: (2 comments) https://review.coreboot.org/c/coreboot/+/37289/5/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S: https://review.coreboot.org/c/coreboot/+/37289/5/src/cpu/x86/smm/smm_stub.S@... PS5, Line 87: /* emit "Crash" on serial */ only write something on serial in case of CONSOLE_SERIAL? https://review.coreboot.org/c/coreboot/+/37289/5/src/cpu/x86/smm/smm_stub.S@... PS5, Line 100: ud2 do you need to crash? isn't returning from SMM enough? -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 01 Dec 2019 17:35:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 5: (4 comments) https://review.coreboot.org/c/coreboot/+/37289/5/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S: https://review.coreboot.org/c/coreboot/+/37289/5/src/cpu/x86/smm/smm_stub.S@... PS5, Line 87: /* emit "Crash" on serial */
only write something on serial in case of CONSOLE_SERIAL? I guess we could ifdef this out, yes.
https://review.coreboot.org/c/coreboot/+/37289/5/src/cpu/x86/smm/smm_stub.S@... PS5, Line 100: ud2
do you need to crash? isn't returning from SMM enough? That would still enable using the lapic relocation as an SMM blocker, which may be undesirable. Since there's no good reason for the lapic to be configured to such addresses at all, crashing is the most robust way to deal with it.
https://review.coreboot.org/c/coreboot/+/37289/5/src/cpu/x86/smm/smm_stub.S@... PS5, Line 154: protected mode I suppose the GDT is in the already sanitized region? https://review.coreboot.org/c/coreboot/+/37289/5/src/cpu/x86/smm/smm_stub.S@... PS5, Line 154: relocateble relocatable -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 01 Dec 2019 23:09:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 5: Code-Review+1 (1 comment) File src/cpu/x86/smm/sinkhole.c: https://review.coreboot.org/c/coreboot/+/37289/comment/e4f6813e_2011e4de PS1, Line 31: printk(BIOS_EMERG, "Wrong LAPIC base detected! dying...\n");
We don't need to be too friendly about this as there's really no reason for anybody to do this legit […] Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Attention: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Comment-Date: Fri, 05 Feb 2021 19:22:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Patrick Georgi <pgeorgi@google.com> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans. Hello build bot (Jenkins), Martin Roth, Angel Pons, Aaron Durbin, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/37289 to look at the new patch set (#6). Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub The sinkhole exploit exists in placing the lapic base such that it messes with GDT. This can be mitigated by checking the lapic MSR against the current program counter. Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/cpu/x86/smm/smm_stub.S 1 file changed, 37 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37289/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: newpatchset
Attention is currently required from: Arthur Heymans. Hello build bot (Jenkins), Martin Roth, Angel Pons, Aaron Durbin, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/37289 to look at the new patch set (#7). Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub The sinkhole exploit exists in placing the lapic base such that it messes with GDT. This can be mitigated by checking the lapic MSR against the current program counter. Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> --- M src/cpu/x86/smm/smm_stub.S 1 file changed, 39 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/37289/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: newpatchset
Attention is currently required from: Patrick Rudolph. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 5: (5 comments) File src/cpu/x86/smm/smm_stub.S: https://review.coreboot.org/c/coreboot/+/37289/comment/63ca5f15_06e393fd PS3, Line 218: $0xfffffff0, %esp
stack is located above DEFAULT_SMM_SIZE, so the check needs to happen before accessing the stack. Done
File src/cpu/x86/smm/smm_stub.S: https://review.coreboot.org/c/coreboot/+/37289/comment/ed7187fc_b47741ce PS5, Line 87: /* emit "Crash" on serial */
I guess we could ifdef this out, yes. Done
https://review.coreboot.org/c/coreboot/+/37289/comment/6086b753_9a0445f6 PS5, Line 100: ud2
That would still enable using the lapic relocation as an SMM blocker, which may be undesirable. […] Done
https://review.coreboot.org/c/coreboot/+/37289/comment/1c9129e7_fe4e4eb1 PS5, Line 154: relocateble
relocatable Done
https://review.coreboot.org/c/coreboot/+/37289/comment/d288e184_0f4821b4 PS5, Line 154: protected mode
I suppose the GDT is in the already sanitized region? Done. Yes that's actually the only thing that needs to be checked.
-- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Comment-Date: Fri, 08 Apr 2022 14:09:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Georgi <patrick@coreboot.org> Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph@9elements.com> Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment
Attention is currently required from: Patrick Rudolph, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... Patch Set 7: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-Attention: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Comment-Date: Fri, 08 Apr 2022 22:04:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37289 ) Change subject: cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub ...................................................................... cpu/x86/smm: Add sinkhole mitigation to relocatable smmstub The sinkhole exploit exists in placing the lapic base such that it messes with GDT. This can be mitigated by checking the lapic MSR against the current program counter. Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> Reviewed-on: https://review.coreboot.org/c/coreboot/+/37289 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Angel Pons <th3fanbus@gmail.com> --- M src/cpu/x86/smm/smm_stub.S 1 file changed, 39 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index c83839c..02532a4 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -45,10 +45,49 @@ (CR0_CD | CR0_NW | CR0_PG | CR0_AM | CR0_WP | \ CR0_NE | CR0_TS | CR0_EM | CR0_MP) +#define SMM_DEFAULT_SIZE 0x10000 + .text .code16 .global _start _start: +smm_handler_start: +#if CONFIG(SMM_LAPIC_REMAP_MITIGATION) + /* Check if the LAPIC register block overlaps with the stub. + * This block needs to work without data accesses because they + * may be routed into the LAPIC register block. + * Code accesses, on the other hand, are never routed to LAPIC, + * which is what makes this work in the first place. + */ + mov $LAPIC_BASE_MSR, %ecx + rdmsr + and $(~0xfff), %eax + call 1f + /* Get the current program counter */ +1: + pop %ebx + sub %ebx, %eax + cmp $(SMM_DEFAULT_SIZE), %eax + ja untampered_lapic +1: +#if CONFIG(CONSOLE_SERIAL) + /* emit "Crash" on serial */ + mov $(CONFIG_TTYS0_BASE), %dx + mov $'C', %al + out %al, (%dx) + mov $'r', %al + out %al, (%dx) + mov $'a', %al + out %al, (%dx) + mov $'s', %al + out %al, (%dx) + mov $'h', %al + out %al, (%dx) +#endif /* CONFIG_CONSOLE_SERIAL */ + /* now crash for real */ + ud2 +untampered_lapic: +#endif movl $(smm_relocate_gdt), %ebx lgdtl (%ebx) -- To view, visit https://review.coreboot.org/c/coreboot/+/37289 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49927c4f4218552b732bac8aae551d845ad7f079 Gerrit-Change-Number: 37289 Gerrit-PatchSet: 8 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot@felixheld.de> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-MessageType: merged
participants (5)
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Felix Held (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)