Hello Aaron Durbin, Frans Hendriks, Philipp Deppenwiese,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32716
to review the following change.
Change subject: vboot: Turn vboot_logic_executed() into a static inline
......................................................................
vboot: Turn vboot_logic_executed() into a static inline
This patch moves vboot_logic_executed() (and its dependencies) into a
header and turns it into a static inline function. The function is used
to guard larger amounts of code in several places, so this should allow
us to safe some more space through compile-time elimination (and also
makes it easier to avoid undefined reference issues in some cases).
Change-Id: I193f608882cbfe07dc91ee90d02fafbd67a3c324
Signed-off-by: Julius Werner <jwerner(a)chromium.org>
---
M src/security/vboot/misc.h
M src/security/vboot/vboot_loader.c
2 files changed, 58 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/32716/1
diff --git a/src/security/vboot/misc.h b/src/security/vboot/misc.h
index b4fae19..2a0523d 100644
--- a/src/security/vboot/misc.h
+++ b/src/security/vboot/misc.h
@@ -16,6 +16,8 @@
#ifndef __VBOOT_MISC_H__
#define __VBOOT_MISC_H__
+#include <assert.h>
+#include <arch/early_variables.h>
#include <security/vboot/vboot_common.h>
struct vb2_context;
@@ -66,13 +68,63 @@
void vboot_fill_handoff(void);
/*
- * Source: security/vboot/vboot_loader.c
- */
-int vboot_logic_executed(void);
-
-/*
* Source: security/vboot/bootmode.c
*/
void vboot_save_recovery_reason_vbnv(void);
+/*
+ * The stage loading code is compiled and entered from multiple stages. The
+ * helper functions below attempt to provide more clarity on when certain
+ * code should be called. They are implemented inline for better compile-time
+ * code elimination.
+ */
+
+static inline int verification_should_run(void)
+{
+ if (CONFIG(VBOOT_SEPARATE_VERSTAGE))
+ return ENV_VERSTAGE;
+ else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
+ return ENV_ROMSTAGE;
+ else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
+ return ENV_BOOTBLOCK;
+ else
+ dead_code();
+}
+
+static inline int verstage_should_load(void)
+{
+ if (CONFIG(VBOOT_SEPARATE_VERSTAGE))
+ return ENV_BOOTBLOCK;
+ else
+ return 0;
+}
+
+/* Only for use by functions in this header, do not access directly! */
+extern int vboot_executed;
+
+static inline int vboot_logic_executed(void)
+{
+ /* If we are in the stage that runs verification, or in the stage that
+ both loads the verstage and is returned to from it afterwards, we
+ need to check a global to see if verfication has run. */
+ if (verification_should_run() ||
+ (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE)))
+ return car_get_var(vboot_executed);
+
+ if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) {
+ /* All other stages are "after the bootblock" */
+ return !ENV_BOOTBLOCK;
+ } else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) {
+ /* Post-RAM stages are "after the romstage" */
+#ifdef __PRE_RAM__
+ return 0;
+#else
+ return 1;
+#endif
+ } else {
+ dead_code();
+ }
+}
+
+
#endif /* __VBOOT_MISC_H__ */
diff --git a/src/security/vboot/vboot_loader.c b/src/security/vboot/vboot_loader.c
index 3bbb3da..1350307 100644
--- a/src/security/vboot/vboot_loader.c
+++ b/src/security/vboot/vboot_loader.c
@@ -36,55 +36,7 @@
CONFIG(VBOOT_SEPARATE_VERSTAGE),
"return from verstage only makes sense for separate verstages");
-/* The stage loading code is compiled and entered from multiple stages. The
- * helper functions below attempt to provide more clarity on when certain
- * code should be called. */
-
-static int verification_should_run(void)
-{
- if (CONFIG(VBOOT_SEPARATE_VERSTAGE))
- return ENV_VERSTAGE;
- else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
- return ENV_ROMSTAGE;
- else if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK))
- return ENV_BOOTBLOCK;
- else
- die("impossible!");
-}
-
-static int verstage_should_load(void)
-{
- if (CONFIG(VBOOT_SEPARATE_VERSTAGE))
- return ENV_BOOTBLOCK;
- else
- return 0;
-}
-
-static int vboot_executed CAR_GLOBAL;
-
-int vboot_logic_executed(void)
-{
- /* If we are in the stage that runs verification, or in the stage that
- both loads the verstage and is returned to from it afterwards, we
- need to check a global to see if verfication has run. */
- if (verification_should_run() ||
- (verstage_should_load() && CONFIG(VBOOT_RETURN_FROM_VERSTAGE)))
- return car_get_var(vboot_executed);
-
- if (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK)) {
- /* All other stages are "after the bootblock" */
- return !ENV_BOOTBLOCK;
- } else if (CONFIG(VBOOT_STARTS_IN_ROMSTAGE)) {
- /* Post-RAM stages are "after the romstage" */
-#ifdef __PRE_RAM__
- return 0;
-#else
- return 1;
-#endif
- } else {
- die("impossible!");
- }
-}
+int vboot_executed CAR_GLOBAL;
static void vboot_prepare(void)
{
--
To view, visit https://review.coreboot.org/c/coreboot/+/32716
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I193f608882cbfe07dc91ee90d02fafbd67a3c324
Gerrit-Change-Number: 32716
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-MessageType: newchange
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34882 )
Change subject: arch/x86: Move stack with CAR_GLOBAL_MIGRATION
......................................................................
arch/x86: Move stack with CAR_GLOBAL_MIGRATION
With the combination of CAR_GLOBAL_MIGRATION=n and
POSTCAR_STAGE=n it is expected that ramstage decompression
will run in romstage. Due the way MAYBE_STATIC was defined
the scratchpad of ulzman() would remain on the stack.
This would conflict with the small allocation made for the
stack when we have C_ENVIRONMENT_BOOTBLOCK=y.
Expand the definition of MAYBE_STATIC to cover the cases that
have zero-initialisation or no initialisation. These can be
located in .bss. As the large stack requirement now only
applies to CAR_GLOBAL_MIGRATION=y case, stack location within
CAR region can be based on that instead.
Change-Id: I58d50c6f8f922c9e8664698d77836cac2c13b126
Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
---
M src/arch/x86/car.ld
M src/cpu/Kconfig
M src/cpu/intel/socket_FCBGA559/Kconfig
M src/cpu/intel/socket_mPGA604/Kconfig
M src/device/device_const.c
M src/include/stddef.h
M src/lib/lzma.c
M src/lib/timestamp.c
M src/northbridge/intel/haswell/Kconfig
M src/security/tpm/tspi/log.c
M src/soc/amd/picasso/Kconfig
M src/soc/amd/stoneyridge/Kconfig
M src/soc/intel/braswell/Kconfig
M src/soc/intel/broadwell/Kconfig
14 files changed, 20 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34882/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld
index 5351fc7..6911505 100644
--- a/src/arch/x86/car.ld
+++ b/src/arch/x86/car.ld
@@ -40,7 +40,7 @@
/* Stack for CAR stages. Since it persists across all stages that
* use CAR it can be reused. The chipset/SoC is expected to provide
* the stack size. */
-#if CONFIG(C_ENVIRONMENT_BOOTBLOCK)
+#if !CONFIG(CAR_GLOBAL_MIGRATION)
_car_stack_start = .;
. += CONFIG_DCACHE_BSP_STACK_SIZE;
_car_stack_end = .;
@@ -91,7 +91,7 @@
_car_global_end = .;
_car_relocatable_data_end = .;
-#if !CONFIG(C_ENVIRONMENT_BOOTBLOCK)
+#if CONFIG(CAR_GLOBAL_MIGRATION)
_car_stack_start = .;
_car_stack_end = _car_region_end;
#endif
diff --git a/src/cpu/Kconfig b/src/cpu/Kconfig
index 3c0bf89..7982569 100644
--- a/src/cpu/Kconfig
+++ b/src/cpu/Kconfig
@@ -21,7 +21,12 @@
hex
config DCACHE_BSP_STACK_SIZE
+ depends on !CAR_GLOBAL_MIGRATION
hex
+ default 0x2000
+ help
+ The amount of anticipated stack usage by bootblock and
+ other pre-ram stages.
config SMP
bool
diff --git a/src/cpu/intel/socket_FCBGA559/Kconfig b/src/cpu/intel/socket_FCBGA559/Kconfig
index d3af4ca..b1b310d 100644
--- a/src/cpu/intel/socket_FCBGA559/Kconfig
+++ b/src/cpu/intel/socket_FCBGA559/Kconfig
@@ -21,11 +21,4 @@
hex
default 0x4000
-config DCACHE_BSP_STACK_SIZE
- hex
- default 0x2000
- help
- The amount of anticipated stack usage in CAR by bootblock and
- other stages.
-
endif
diff --git a/src/cpu/intel/socket_mPGA604/Kconfig b/src/cpu/intel/socket_mPGA604/Kconfig
index 1453f99..65ac777 100644
--- a/src/cpu/intel/socket_mPGA604/Kconfig
+++ b/src/cpu/intel/socket_mPGA604/Kconfig
@@ -28,8 +28,4 @@
hex
default 0x4000
-config DCACHE_BSP_STACK_SIZE
- hex
- default 0x2000
-
endif # CPU_INTEL_SOCKET_MPGA604
diff --git a/src/device/device_const.c b/src/device/device_const.c
index c472aea..ef16d4b 100644
--- a/src/device/device_const.c
+++ b/src/device/device_const.c
@@ -204,7 +204,7 @@
DEVTREE_CONST struct bus *pci_root_bus(void)
{
DEVTREE_CONST struct device *pci_domain;
- MAYBE_STATIC DEVTREE_CONST struct bus *pci_root = NULL;
+ DECLARE_MAYBE_STATIC_ZERO(DEVTREE_CONST struct bus *pci_root);
if (pci_root)
return pci_root;
diff --git a/src/include/stddef.h b/src/include/stddef.h
index 7cae2e6..26b9ef6 100644
--- a/src/include/stddef.h
+++ b/src/include/stddef.h
@@ -42,6 +42,14 @@
#define MAYBE_STATIC static
#endif
+#if defined(__PRE_RAM__) && CONFIG(CAR_GLOBAL_MIGRATION)
+#define MAYBE_STATIC_NOINIT
+#define DECLARE_MAYBE_STATIC_ZERO(x) x = 0
+#else
+#define MAYBE_STATIC_NOINIT static
+#define DECLARE_MAYBE_STATIC_ZERO(x) static x
+#endif
+
#ifndef __ROMCC__
/* Provide a pointer to address 0 that thwarts any "accessing this is
* undefined behaviour and do whatever" trickery in compilers.
diff --git a/src/lib/lzma.c b/src/lib/lzma.c
index eecbb26..7700380 100644
--- a/src/lib/lzma.c
+++ b/src/lib/lzma.c
@@ -26,7 +26,7 @@
int res;
CLzmaDecoderState state;
SizeT mallocneeds;
- MAYBE_STATIC unsigned char scratchpad[15980];
+ MAYBE_STATIC_NOINIT unsigned char scratchpad[15980];
const unsigned char *cp;
memcpy(properties, src, LZMA_PROPERTIES_SIZE);
diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c
index 1319b86..931da3f 100644
--- a/src/lib/timestamp.c
+++ b/src/lib/timestamp.c
@@ -128,7 +128,7 @@
static struct timestamp_table *timestamp_table_get(void)
{
- MAYBE_STATIC struct timestamp_table *ts_table = NULL;
+ DECLARE_MAYBE_STATIC_ZERO(struct timestamp_table *ts_table);
struct timestamp_cache *ts_cache;
if (ts_table != NULL)
diff --git a/src/northbridge/intel/haswell/Kconfig b/src/northbridge/intel/haswell/Kconfig
index aad2674..847cc69 100644
--- a/src/northbridge/intel/haswell/Kconfig
+++ b/src/northbridge/intel/haswell/Kconfig
@@ -70,13 +70,6 @@
help
The amount of cache-as-ram region required by the reference code.
-config DCACHE_BSP_STACK_SIZE
- hex
- default 0x2000
- help
- The amount of anticipated stack usage in CAR by bootblock and
- other stages.
-
config HAVE_MRC
bool "Add a System Agent binary"
help
diff --git a/src/security/tpm/tspi/log.c b/src/security/tpm/tspi/log.c
index 4019962..a513860 100644
--- a/src/security/tpm/tspi/log.c
+++ b/src/security/tpm/tspi/log.c
@@ -26,7 +26,7 @@
static struct tcpa_table *tcpa_cbmem_init(void)
{
- MAYBE_STATIC struct tcpa_table *tclt = NULL;
+ DECLARE_MAYBE_STATIC_ZERO(struct tcpa_table *tclt);
if (tclt)
return tclt;
@@ -47,7 +47,7 @@
static struct tcpa_table *tcpa_log_init(void)
{
- MAYBE_STATIC struct tcpa_table *tclt = NULL;
+ DECLARE_MAYBE_STATIC_ZERO(struct tcpa_table *tclt);
/* We are dealing here with pre CBMEM environment.
* If cbmem isn't available use CAR or SRAM */
diff --git a/src/soc/amd/picasso/Kconfig b/src/soc/amd/picasso/Kconfig
index 26b2ec4..9f2bc06 100644
--- a/src/soc/amd/picasso/Kconfig
+++ b/src/soc/amd/picasso/Kconfig
@@ -87,7 +87,6 @@
default 0x10000
config DCACHE_BSP_STACK_SIZE
- depends on C_ENVIRONMENT_BOOTBLOCK
hex
default 0x4000
help
diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig
index f1d08c0..b494166 100644
--- a/src/soc/amd/stoneyridge/Kconfig
+++ b/src/soc/amd/stoneyridge/Kconfig
@@ -103,7 +103,6 @@
default 0x10000
config DCACHE_BSP_STACK_SIZE
- depends on C_ENVIRONMENT_BOOTBLOCK
hex
default 0x4000
help
diff --git a/src/soc/intel/braswell/Kconfig b/src/soc/intel/braswell/Kconfig
index 5d6438f..7b41f0c 100644
--- a/src/soc/intel/braswell/Kconfig
+++ b/src/soc/intel/braswell/Kconfig
@@ -50,13 +50,6 @@
select SOUTHBRIDGE_INTEL_COMMON_SMBUS
select C_ENVIRONMENT_BOOTBLOCK
-config DCACHE_BSP_STACK_SIZE
- hex
- default 0x2000
- help
- The amount of anticipated stack usage in CAR by bootblock and
- other stages.
-
config C_ENV_BOOTBLOCK_SIZE
hex
default 0x8000
diff --git a/src/soc/intel/broadwell/Kconfig b/src/soc/intel/broadwell/Kconfig
index 5856ef1..dc4dd44 100644
--- a/src/soc/intel/broadwell/Kconfig
+++ b/src/soc/intel/broadwell/Kconfig
@@ -119,13 +119,6 @@
help
The amount of cache-as-ram region required by the reference code.
-config DCACHE_BSP_STACK_SIZE
- hex
- default 0x2000
- help
- The amount of anticipated stack usage in CAR by bootblock and
- other stages.
-
config HAVE_MRC
bool "Add a Memory Reference Code binary"
help
--
To view, visit https://review.coreboot.org/c/coreboot/+/34882
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I58d50c6f8f922c9e8664698d77836cac2c13b126
Gerrit-Change-Number: 34882
Gerrit-PatchSet: 1
Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newchange
John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33232
Change subject: src/cpu/x86: Check pointer processor_name_start before dereference
......................................................................
src/cpu/x86: Check pointer processor_name_start before dereference
Clang Static Analyzer version 8.0.0 detects the left operand of '=='
is a garbage value if pointer processor_name_start is NULL. Add sanity
check for processor_name_start before dereference.
TEST=Built and boot up to kernel.
Change-Id: I1f831a8661a4686d306b8217655942934102ea16
Signed-off-by: John Zhao <john.zhao(a)intel.com>
---
M src/cpu/x86/name/name.c
1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/33232/1
diff --git a/src/cpu/x86/name/name.c b/src/cpu/x86/name/name.c
index fc360cd..44a1cf0 100644
--- a/src/cpu/x86/name/name.c
+++ b/src/cpu/x86/name/name.c
@@ -37,6 +37,9 @@
/* Skip leading spaces. */
processor_name_start = (char *)name_as_ints;
+ if (!processor_name_start)
+ return;
+
while (*processor_name_start == ' ')
processor_name_start++;
--
To view, visit https://review.coreboot.org/c/coreboot/+/33232
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I1f831a8661a4686d306b8217655942934102ea16
Gerrit-Change-Number: 33232
Gerrit-PatchSet: 1
Gerrit-Owner: John Zhao <john.zhao(a)intel.com>
Gerrit-MessageType: newchange
John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/vboot/+/35021 )
Change subject: vboot: Avoid insecure data handling
......................................................................
vboot: Avoid insecure data handling
Coverity detects the overflowed value "rev" used as return value.
Cast the value "rev" to integer after strtol invocation.
BUG=CID 1401793
TEST=Built and boot up to kernel.
Signed-off-by: John Zhao <john.zhao(a)intel.com>
Change-Id: Idcb48d9a8f7c89744c66f50affb5f9acc6aa4c12
---
M futility/updater.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/vboot refs/changes/21/35021/1
diff --git a/futility/updater.c b/futility/updater.c
index ef142b8..14dd469 100644
--- a/futility/updater.c
+++ b/futility/updater.c
@@ -234,7 +234,7 @@
/* Result should be 'revN' */
if (strncmp(result, STR_REV, strlen(STR_REV)) == 0)
- rev = strtol(result + strlen(STR_REV), NULL, 0);
+ rev = (int)strtol(result + strlen(STR_REV), NULL, 0);
VB2_DEBUG("Raw data = [%s], parsed version is %d\n", result, rev);
free(result);
--
To view, visit https://review.coreboot.org/c/vboot/+/35021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: vboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcb48d9a8f7c89744c66f50affb5f9acc6aa4c12
Gerrit-Change-Number: 35021
Gerrit-PatchSet: 1
Gerrit-Owner: John Zhao <john.zhao(a)intel.com>
Gerrit-MessageType: newchange