[coreboot-gerrit] Patch set updated for coreboot: aebb6ba arm, arm64, mips: Add rough static stack size checks with -Wstack-usage
Patrick Georgi (pgeorgi@google.com)
gerrit at coreboot.org
Tue Jun 2 12:12:59 CEST 2015
Patrick Georgi (pgeorgi at google.com) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/9729
-gerrit
commit aebb6ba690ca29a953df7d2281aa798467ad0a9f
Author: Julius Werner <jwerner at chromium.org>
Date: Fri Dec 19 16:11:14 2014 -0800
arm, arm64, mips: Add rough static stack size checks with -Wstack-usage
We've seen an increasing need to reduce stack sizes more and more for
space reasons, and it's always guesswork because no one has a good idea
how little is too litte. We now have boards with 3K and 2K stacks, and
old pieces of common code often allocate large temporary buffers that
would lead to very dangerous and hard to detect bugs when someone
eventually tries to use them on one of those.
This patch tries improve this situation at least a bit by declaring 2K
as the minimum stack size all of coreboot code should work with. It
checks all function frames with -Wstack-usage=1536 to make sure we don't
allocate more than 1.5K in a single buffer. This is of course not a
perfect test, but it should catch the most common situation of declaring
a single, large buffer in some close-to-leaf function (with the
assumption that 0.5K is hopefully enough for all the "normal" functions
above that).
Change one example where we were a bit overzealous and put a 1K buffer
into BSS back to stack allocation, since it actually conforms to this
new assumption and frees up another kilobyte of that highly sought-after
verstage space. Not touching x86 with any of this since it's lack of
__PRE_RAM__ BSS often requires it to allocate way more on the stack than
would usually be considered sane.
BRANCH=veyron
BUG=None
TEST=Compiled Cosmos, Daisy, Falco, Blaze, Pit, Storm, Urara and Pinky,
made sure they still build as well as before and don't show any stack
usage warnings.
Change-Id: Idc53d33bd8487bbef49d3ecd751914b0308006ec
Signed-off-by: Patrick Georgi <pgeorgi at chromium.org>
Original-Commit-Id: 8e5931066575e256dfc2295c3dab7f0e1b65417f
Original-Change-Id: I30bd9c2c77e0e0623df89b9e5bb43ed29506be98
Original-Signed-off-by: Julius Werner <jwerner at chromium.org>
Original-Reviewed-on: https://chromium-review.googlesource.com/236978
Original-Reviewed-by: David Hendricks <dhendrix at chromium.org>
Original-Reviewed-by: Aaron Durbin <adurbin at chromium.org>
---
src/arch/arm/include/arch/memlayout.h | 4 +++-
src/arch/arm64/include/arch/memlayout.h | 4 +++-
src/arch/mips/include/arch/memlayout.h | 4 +++-
src/drivers/spi/spi_flash.c | 4 ++++
src/lib/gpio.c | 4 +++-
src/vendorcode/google/chromeos/vboot2/verstage.c | 2 +-
toolchain.inc | 29 +++++++++++++++++-------
7 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/src/arch/arm/include/arch/memlayout.h b/src/arch/arm/include/arch/memlayout.h
index b28e0cf..86f5585 100644
--- a/src/arch/arm/include/arch/memlayout.h
+++ b/src/arch/arm/include/arch/memlayout.h
@@ -35,7 +35,9 @@
"TTB subtable region must be evenly divisible by table size!");
/* ARM stacks need 8-byte alignment and stay in one place through ramstage. */
-#define STACK(addr, size) REGION(stack, addr, size, 8)
+#define STACK(addr, size) \
+ REGION(stack, addr, size, 8) \
+ _ = ASSERT(size >= 2K, "stack should be >= 2K, see toolchain.inc");
#define DMA_COHERENT(addr, size) \
REGION(dma_coherent, addr, size, SUPERPAGE_SIZE) \
diff --git a/src/arch/arm64/include/arch/memlayout.h b/src/arch/arm64/include/arch/memlayout.h
index 522f1ab..30db848 100644
--- a/src/arch/arm64/include/arch/memlayout.h
+++ b/src/arch/arm64/include/arch/memlayout.h
@@ -27,7 +27,9 @@
/* ARM64 stacks need 16-byte alignment. The ramstage will set up its own stacks
* in BSS, so this is only used for the SRAM stages. */
#ifdef __PRE_RAM__
-#define STACK(addr, size) REGION(stack, addr, size, 16)
+#define STACK(addr, size) \
+ REGION(stack, addr, size, 16) \
+ _ = ASSERT(size >= 2K, "stack should be >= 2K, see toolchain.inc");
#else
#define STACK(addr, size) REGION(preram_stack, addr, size, 16)
#endif
diff --git a/src/arch/mips/include/arch/memlayout.h b/src/arch/mips/include/arch/memlayout.h
index 9493173..946fcf3 100644
--- a/src/arch/mips/include/arch/memlayout.h
+++ b/src/arch/mips/include/arch/memlayout.h
@@ -24,7 +24,9 @@
/* MIPS stacks need 8-byte alignment and stay in one place through ramstage. */
/* TODO: Double-check that that's the correct alignment for our ABI. */
-#define STACK(addr, size) REGION(stack, addr, size, 8)
+#define STACK(addr, size) \
+ REGION(stack, addr, size, 8) \
+ _ = ASSERT(size >= 2K, "stack should be >= 2K, see toolchain.inc");
#define DMA_COHERENT(addr, size) REGION(dma_coherent, addr, size, 4K)
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c
index b0558bb..9b6d4ec 100644
--- a/src/drivers/spi/spi_flash.c
+++ b/src/drivers/spi/spi_flash.c
@@ -94,6 +94,9 @@ static int spi_flash_cmd_read(struct spi_slave *spi, const u8 *cmd,
return ret;
}
+/* TODO: This code is quite possibly broken and overflowing stacks. Fix ASAP! */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstack-usage="
int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len,
const void *data, size_t data_len)
{
@@ -110,6 +113,7 @@ int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len,
return ret;
}
+#pragma GCC diagnostic pop
static int spi_flash_cmd_read_array(struct spi_slave *spi, u8 *cmd,
size_t cmd_len, u32 offset,
diff --git a/src/lib/gpio.c b/src/lib/gpio.c
index b185cc2..72bd7ec 100644
--- a/src/lib/gpio.c
+++ b/src/lib/gpio.c
@@ -17,6 +17,7 @@
* Foundation, Inc.
*/
+#include <assert.h>
#include <base3.h>
#include <console/console.h>
#include <delay.h>
@@ -53,7 +54,8 @@ int gpio_base3_value(gpio_t gpio[], int num_gpio)
int temp;
int index;
int result = 0;
- char value[num_gpio];
+ char value[32];
+ assert(num_gpio <= 32);
/* Enable internal pull up */
for (index = 0; index < num_gpio; ++index)
diff --git a/src/vendorcode/google/chromeos/vboot2/verstage.c b/src/vendorcode/google/chromeos/vboot2/verstage.c
index bc7846a..1e653d0 100644
--- a/src/vendorcode/google/chromeos/vboot2/verstage.c
+++ b/src/vendorcode/google/chromeos/vboot2/verstage.c
@@ -119,7 +119,7 @@ static int hash_body(struct vb2_context *ctx, struct region_device *fw_main)
{
uint64_t load_ts;
uint32_t expected_size;
- MAYBE_STATIC uint8_t block[TODO_BLOCK_SIZE];
+ uint8_t block[TODO_BLOCK_SIZE];
size_t block_size = sizeof(block);
size_t offset;
int rv;
diff --git a/toolchain.inc b/toolchain.inc
index e089f2e..572e6c5 100644
--- a/toolchain.inc
+++ b/toolchain.inc
@@ -61,16 +61,29 @@ ARCHDIR-arm64 := arm64
ARCHDIR-riscv := riscv
ARCHDIR-mips := mips
-CFLAGS_arm := -ffunction-sections -fdata-sections
-
-CFLAGS_arm64 := -ffunction-sections -fdata-sections
-
-CFLAGS_mips := -mips32r2 -G 0 -ffunction-sections -fdata-sections
+CFLAGS_common += -ffunction-sections -fdata-sections
+
+# Some boards only provide 2K stacks, so storing lots of data there leads to
+# problems. Since C rules don't allow us to statically determine the maximum
+# stack use, we use 1.5K as heuristic, assuming that we typically have lots
+# of tiny stack frames and the odd large one.
+#
+# Store larger buffers in BSS, use MAYBE_STATIC to share code with __PRE_RAM__
+# on x86.
+# Since GCCs detection of dynamic array bounds unfortunately seems to be
+# very basic, you'll sometimes have to use a static upper bound for the
+# size and an assert() to make sure it's honored (see gpio_base3_value()
+# for an example).
+# (If you absolutely need a larger stack frame and are 100% sure it cannot
+# cause problems, you can whitelist it with #pragma diagnostic.)
+CFLAGS_arm += -Wstack-usage=1536
+CFLAGS_arm64 += -Wstack-usage=1536
+CFLAGS_mips += -Wstack-usage=1536
+CFLAGS_riscv += -Wstack-usage=1536
+
+CFLAGS_mips := -mips32r2 -G 0
CFLAGS_mips += -mno-abicalls -fno-pic
-CFLAGS_x86_32 += -ffunction-sections -fdata-sections
-CFLAGS_riscv := -ffunction-sections -fdata-sections
-
toolchain_to_dir = \
$(foreach arch,$(ARCH_SUPPORTED),\
$(eval CPPFLAGS_$(arch) += \
More information about the coreboot-gerrit
mailing list