coreboot-gerrit June 2018

coreboot-gerrit@coreboot.org

1 participants
2751 discussions

Change in coreboot[master]: drivers/intel/fsp1_1: Fix veb_len type
by Nico Huber (Code Review) 25 Jun '18

25 Jun '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/27224 ) Change subject: drivers/intel/fsp1_1: Fix veb_len type ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/27224 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22798fa0edb98fcb9acc1b2dd52f34a61bc511e9 Gerrit-Change-Number: 27224 Gerrit-PatchSet: 1 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 25 Jun 2018 18:11:13 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in coreboot[master]: mb/google/*: Add a few VBT files
by build bot (Jenkins) (Code Review) 25 Jun '18

25 Jun '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/27222 ) Change subject: mb/google/*: Add a few VBT files ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-checkpatch/29352/ : SUCCESS https://qa.coreboot.org/job/coreboot-gerrit/75299/ : SUCCESS -- To view, visit https://review.coreboot.org/27222 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f05c913872c5d2d8c8279d89eac52fd4bf4e35e Gerrit-Change-Number: 27222 Gerrit-PatchSet: 2 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 25 Jun 2018 18:10:00 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in coreboot[master]: drivers/intel/fsp1_1: Fix veb_len type
by build bot (Jenkins) (Code Review) 25 Jun '18

25 Jun '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/27224 ) Change subject: drivers/intel/fsp1_1: Fix veb_len type ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-checkpatch/29353/ : SUCCESS https://qa.coreboot.org/job/coreboot-gerrit/75300/ : SUCCESS -- To view, visit https://review.coreboot.org/27224 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22798fa0edb98fcb9acc1b2dd52f34a61bc511e9 Gerrit-Change-Number: 27224 Gerrit-PatchSet: 1 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 25 Jun 2018 18:07:58 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Change in coreboot[master]: smm: Add canary to end of stack and die() if a stack overflow occurs
by Raul Rangel (Code Review) 25 Jun '18

25 Jun '18

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/27229 Change subject: smm: Add canary to end of stack and die() if a stack overflow occurs ...................................................................... smm: Add canary to end of stack and die() if a stack overflow occurs If CPU 0's stack grows to large, it will overflow into CPU 1's stack. If CPU 0 is handling the interrupt then CPU 1 should be in an idle loop. When the stack overflow occurs it will override the return pointer for CPU 1, so when CPU 0 unlocks the SMI lock, CPU 1 will attempt to return to a random address. This method is not full proof. If code allocates some stack variables that overlap with the canary, and if the variables are never set, then the canary will not be overwritten, but it will have been skipped. We could mitigate this by adding a larger canary value if we wanted. We can explore adding other methods of signaling that a stack overflow had occurred in a follow up. I limited die() to debug only because otherwise it would be very hard to track down. TEST=built on grunt with a small and large stack size. Then verified that one causes a stack overflow and the other does not. Stack overflow message: canary 0x0 != 0xcdeafc00 SMM Handler caused a stack overflow Change-Id: I0184de7e3bfb84e0f74e1fa6a307633541f55612 Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> --- M src/cpu/x86/smm/smm_module_handler.c M src/cpu/x86/smm/smm_stub.S M src/include/cpu/x86/smm.h 3 files changed, 28 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/27229/1 diff --git a/src/cpu/x86/smm/smm_module_handler.c b/src/cpu/x86/smm/smm_module_handler.c index c2001ec..6dff941 100644 --- a/src/cpu/x86/smm/smm_module_handler.c +++ b/src/cpu/x86/smm/smm_module_handler.c @@ -122,10 +122,13 @@ const struct smm_module_params *p; const struct smm_runtime *runtime; int cpu; + uintptr_t actual_canary; + uintptr_t expected_canary; p = arg; runtime = p->runtime; cpu = p->cpu; + expected_canary = (uintptr_t)p->canary; /* Make sure to set the global runtime. It's OK to race as the value * will be the same across CPUs as well as multiple SMIs. */ @@ -171,6 +174,17 @@ smi_restore_pci_address(); + actual_canary = *p->canary; + + if (actual_canary != expected_canary) { + printk(BIOS_DEBUG, "canary 0x%lx != 0x%lx\n", actual_canary, + expected_canary); + + // Don't die if we can't indicate an error. + if (IS_ENABLED(CONFIG_DEBUG_SMI)) + die("SMM Handler caused a stack overflow\n"); + } + smi_release_lock(); /* De-assert SMI# signal to allow another SMI */ diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index 060361b..6bc06fc 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -136,6 +136,10 @@ subl %eax, %ebx # %ebx(stack_top) - %eax(offset) = %ebx(stack_top) mov %ebx, %esp + movl stack_size, %eax + subl %eax, %ebx # %ebx(stack_top) - %eax(size) = %ebx(stack_bottom) + movl %ebx, (%ebx) # Write canary to the bottom of the stack. + pushl $0x0 # push a NULL stack base pointer mov %esp, %ebp @@ -164,14 +168,18 @@ fxsave (%edi) 1: - /* Align stack to 16 bytes. Another 16 bytes are pushed below. */ + /* Align stack to 16 bytes. Another 32 bytes are pushed below. */ andl $0xfffffff0, %esp /* Call into the c-based SMM relocation function with the platform * parameters. Equivalent to: - * struct arg = { c_handler_params, cpu_num, smm_runtime }; + * struct arg = { c_handler_params, cpu_num, smm_runtime, canary }; * c_handler(&arg) */ + push $0x0 # Padding + push $0x0 # Padding + push $0x0 # Padding + push %ebx # uintptr_t *canary push $(smm_runtime) push %ecx # cpu push c_handler_arg diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index 7dcf4d7..9942772 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -527,6 +527,10 @@ void *arg; int 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. + */ + const uintptr_t *canary; }; /* smm_handler_t is called with arg of smm_module_params pointer. */ -- To view, visit https://review.coreboot.org/27229 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0184de7e3bfb84e0f74e1fa6a307633541f55612 Gerrit-Change-Number: 27229 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>

1 0

Change in coreboot[master]: smm: Switch from %edx to %ebx
by Raul Rangel (Code Review) 25 Jun '18

25 Jun '18

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/27228 Change subject: smm: Switch from %edx to %ebx ...................................................................... smm: Switch from %edx to %ebx %edx gets clobbered before the c handler is invoked. This is just a cleanup cl to make the next cl look clean. BUG=b:80539294 TEST=verified SMI still works on grunt. Change-Id: I21bf41ed4fdeaaa8737c883f202a39cb57c2b517 Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> --- M src/cpu/x86/smm/smm_stub.S 1 file changed, 3 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/27228/1 diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index 3fea32c..060361b 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -132,9 +132,9 @@ 1: movl stack_size, %eax mul %ecx # %eax(stack_size) * %ecx(cpu) = %eax(offset) - movl stack_top, %edx - subl %eax, %edx # %edx(stack_top) - %eax(offset) = %edx(stack_top) - mov %edx, %esp + movl stack_top, %ebx + subl %eax, %ebx # %ebx(stack_top) - %eax(offset) = %ebx(stack_top) + mov %ebx, %esp pushl $0x0 # push a NULL stack base pointer mov %esp, %ebp -- To view, visit https://review.coreboot.org/27228 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I21bf41ed4fdeaaa8737c883f202a39cb57c2b517 Gerrit-Change-Number: 27228 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>

1 0

Change in coreboot[master]: smm: Push a null stack base pointer
by Raul Rangel (Code Review) 25 Jun '18

25 Jun '18

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/27226 Change subject: smm: Push a null stack base pointer ...................................................................... smm: Push a null stack base pointer When generating a backtrace we need an indicator when we have hit the beginning of the stack. The i386 ABI states that %ebp points to the next stack frame. NULL can be used to indicate the end of the stack. We could add a NULL return pointer at %ebp+4, but I decided to omit it since a NULL stack pointer can be used as an indicator that there is no return pointer. BUG=b:80539294 TEST=built and tested on grunt Change-Id: I8a48114d31a5c716335d264fa4fe4da41dc5bf11 Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> --- M src/cpu/x86/smm/smm_stub.S 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/27226/1 diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index 5162c95..3fea32c 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -135,6 +135,8 @@ movl stack_top, %edx subl %eax, %edx # %edx(stack_top) - %eax(offset) = %edx(stack_top) mov %edx, %esp + + pushl $0x0 # push a NULL stack base pointer mov %esp, %ebp subl $0x4, %esp # Allocate locals (fxsave) -- To view, visit https://review.coreboot.org/27226 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8a48114d31a5c716335d264fa4fe4da41dc5bf11 Gerrit-Change-Number: 27226 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>

1 0

Change in coreboot[master]: smm: Clear SMRAM when debugging
by Raul Rangel (Code Review) 25 Jun '18

25 Jun '18

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/27227 Change subject: smm: Clear SMRAM when debugging ...................................................................... smm: Clear SMRAM when debugging This makes it easier to spot unused RAM. BUG=b:80539294 TEST=built and tested on grunt. Verified unused memory was set to 0xcdcdcdcd Change-Id: I335eaf642bd8526f31819eaac95ce80c2df3c300 Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> --- M src/cpu/x86/smm/smm_module_loader.c 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/27227/1 diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 3d7952f..e92fa05 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -346,6 +346,10 @@ if (rmodule_parse(&_binary_smm_start, &smm_mod)) return -1; + /* Clear SMM region */ + if (IS_ENABLED(CONFIG_DEBUG_SMI)) + memset(smram, 0xcd, size); + total_stack_size = params->per_cpu_stack_size * params->num_concurrent_stacks; -- To view, visit https://review.coreboot.org/27227 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I335eaf642bd8526f31819eaac95ce80c2df3c300 Gerrit-Change-Number: 27227 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>

1 0

Change in coreboot[master]: smm: Make local variables relative to ebp
by Raul Rangel (Code Review) 25 Jun '18

25 Jun '18

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/27225 Change subject: smm: Make local variables relative to ebp ...................................................................... smm: Make local variables relative to ebp This reduces the cognitive overhead of referencing locals via esp since it changes with every push. BUG=b:80539294 TEST=built and booted on grunt. Change-Id: Ib7eb98ce3483d4fc803696c1b2496d8384317536 Signed-off-by: Raul E Rangel <rrangel(a)chromium.org> --- M src/cpu/x86/smm/smm_stub.S 1 file changed, 10 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/27225/1 diff --git a/src/cpu/x86/smm/smm_stub.S b/src/cpu/x86/smm/smm_stub.S index 32435a0..5162c95 100644 --- a/src/cpu/x86/smm/smm_stub.S +++ b/src/cpu/x86/smm/smm_stub.S @@ -131,10 +131,13 @@ jmp 2f 1: movl stack_size, %eax - mul %ecx + mul %ecx # %eax(stack_size) * %ecx(cpu) = %eax(offset) movl stack_top, %edx - subl %eax, %edx + subl %eax, %edx # %edx(stack_top) - %eax(offset) = %edx(stack_top) mov %edx, %esp + mov %esp, %ebp + + subl $0x4, %esp # Allocate locals (fxsave) /* calculate fxsave location */ mov fxsave_area, %edi @@ -146,8 +149,7 @@ 2: /* Save location of fxsave area. */ - push %edi - mov %esp, %ebp + mov %edi, -4(%ebp) test %edi, %edi jz 1f @@ -165,20 +167,18 @@ /* Call into the c-based SMM relocation function with the platform * parameters. Equivalent to: - * struct arg = { c_handler_params, cpu_num, smm_runtime {; + * struct arg = { c_handler_params, cpu_num, smm_runtime }; * c_handler(&arg) */ push $(smm_runtime) - push %ecx + push %ecx # cpu push c_handler_arg - push %esp + push %esp # smm_module_params *arg (allocated on stack). mov c_handler, %eax call *%eax - /* Restore stack from call frame */ - mov %ebp, %esp /* Retrieve fxsave location. */ - pop %edi + mov -4(%ebp), %edi test %edi, %edi jz 1f -- To view, visit https://review.coreboot.org/27225 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib7eb98ce3483d4fc803696c1b2496d8384317536 Gerrit-Change-Number: 27225 Gerrit-PatchSet: 1 Gerrit-Owner: Raul Rangel <rrangel(a)chromium.org>

1 0

Change in coreboot[master]: drivers/intel/fsp1_1: Fix veb_len type
by Matt DeVillier (Code Review) 25 Jun '18

25 Jun '18

Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/27224 Change subject: drivers/intel/fsp1_1: Fix veb_len type ...................................................................... drivers/intel/fsp1_1: Fix veb_len type Type of vbt_len should be size_t, change to fix compiler error Change-Id: I22798fa0edb98fcb9acc1b2dd52f34a61bc511e9 Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M src/drivers/intel/fsp1_1/vbt.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/27224/1 diff --git a/src/drivers/intel/fsp1_1/vbt.c b/src/drivers/intel/fsp1_1/vbt.c index b73d3a2..b6bb308 100644 --- a/src/drivers/intel/fsp1_1/vbt.c +++ b/src/drivers/intel/fsp1_1/vbt.c @@ -26,7 +26,7 @@ void load_vbt(uint8_t s3_resume, SILICON_INIT_UPD *params) { const optionrom_vbt_t *vbt_data = NULL; - uint32_t vbt_len; + size_t vbt_len; /* Check boot mode - for S3 resume path VBT loading is not needed */ if (s3_resume) { -- To view, visit https://review.coreboot.org/27224 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I22798fa0edb98fcb9acc1b2dd52f34a61bc511e9 Gerrit-Change-Number: 27224 Gerrit-PatchSet: 1 Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>

1 0

Change in coreboot[master]: mb/google/octopus: Create bobba variant
by build bot (Jenkins) (Code Review) 25 Jun '18

25 Jun '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/27223 ) Change subject: mb/google/octopus: Create bobba variant ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/coreboot-checkpatch/29351/ : SUCCESS https://qa.coreboot.org/job/coreboot-gerrit/75298/ : SUCCESS -- To view, visit https://review.coreboot.org/27223 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic30c6b0d955ce26f4a9f40cd7fef1c429ab950fc Gerrit-Change-Number: 27223 Gerrit-PatchSet: 2 Gerrit-Owner: Justin TerAvest <teravest(a)chromium.org> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Justin TerAvest <teravest(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 25 Jun 2018 18:01:29 +0000 Gerrit-HasComments: No Gerrit-HasLabels: Yes

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2018