From 6754c0002872a0a75b7e1197dadac22461dd1017 Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Fri, 19 Dec 2014 16:11:14 -0800 Subject: [PATCH] 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 512K 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: I30bd9c2c77e0e0623df89b9e5bb43ed29506be98 Signed-off-by: Julius Werner Reviewed-on: https://chromium-review.googlesource.com/236978 Reviewed-by: David Hendricks Reviewed-by: Aaron Durbin Reviewed-on: https://chromium-review.googlesource.com/242137 --- 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 +++- .../google/chromeos/vboot2/verstage.c | 2 +- toolchain.inc | 21 ++++++++++++++++--- 7 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/arch/arm/include/arch/memlayout.h b/src/arch/arm/include/arch/memlayout.h index 2c733946a2..f26aa9d077 100644 --- a/src/arch/arm/include/arch/memlayout.h +++ b/src/arch/arm/include/arch/memlayout.h @@ -33,7 +33,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, (1 + IS_ENABLED(CONFIG_ARM_LPAE)) * 1M) \ diff --git a/src/arch/arm64/include/arch/memlayout.h b/src/arch/arm64/include/arch/memlayout.h index 328156bf3a..4cd5ded6b6 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 4cbbe1d02b..36ac6de8f7 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"); /* TODO: Need to add DMA_COHERENT region like on ARM? */ diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index c429f7da42..1bcad9bf29 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -91,6 +91,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) { @@ -107,6 +110,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 0875538bae..1554930705 100644 --- a/src/lib/gpio.c +++ b/src/lib/gpio.c @@ -17,6 +17,7 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ +#include #include #include #include @@ -49,7 +50,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 569e6e569c..735c33a9ff 100644 --- a/src/vendorcode/google/chromeos/vboot2/verstage.c +++ b/src/vendorcode/google/chromeos/vboot2/verstage.c @@ -95,7 +95,7 @@ static int hash_body(struct vb2_context *ctx, struct vboot_region *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); uintptr_t offset; int rv; diff --git a/toolchain.inc b/toolchain.inc index e8b9c5f9bf..d282947e8e 100644 --- a/toolchain.inc +++ b/toolchain.inc @@ -34,9 +34,24 @@ ARCHDIR-arm64 := arm64 ARCHDIR-x86_32 := x86 ARCHDIR-mipsel := mips -CFLAGS_arm := -mno-unaligned-access -ffunction-sections -fdata-sections -CFLAGS_arm64 := -ffunction-sections -fdata-sections -CFLAGS_mipsel := -mips32r2 -G 0 -ffunction-sections -fdata-sections +# About -Wstack-usage: if you arrived here via grep, you were probably trying to +# do something naughty that could've caused boards with 2K stack size to have a +# very bad time somewhere down the road. Since stack overflows are both very +# dangerous and almost impossible to prevent, we're drawing the line at 1.5K +# for a single function frame (with the assumption that you hopefully don't +# chain more than one of that size together). Buffers larger than that should +# be allocated in the BSS (use MAYBE_STATIC if you need to share code with +# __PRE_RAM__ 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_nonx86 := -ffunction-sections -fdata-sections -Wstack-usage=1536 + +CFLAGS_arm := $(CFLAGS_nonx86) -mno-unaligned-access +CFLAGS_arm64 := $(CFLAGS_nonx86) +CFLAGS_mipsel := $(CFLAGS_nonx86) -mips32r2 -G 0 toolchain_to_dir = \ $(foreach arch,$(ARCH_SUPPORTED),\