Attention is currently required from: Angel Pons.
Hello Angel Pons,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/84059?usp=email
to review the following change.
Change subject: drivers/spi: Stop using a variable-length array ......................................................................
drivers/spi: Stop using a variable-length array
Only the call in `spi_flash_cmd_write_page_program` uses non-constant values for the array length. However, the value for `data_len` has an upper bound: `flash->page_size` is set to `1U << vi->page_size_shift` which depends on the flash chip vendor info, and the largest value it can currently have is 8. Thus, the maximum page size is currently 256.
Define the `MAX_FLASH_CMD_DATA_SIZE` macro to place an upper bound on the amount of data that can be written in one command. Then, use this value to allocate a fixed-size buffer in `spi_flash_cmd_write`. Also, add a check to prevent buffer overflow problems. Finally, ensure that the `spi_flash_cmd_write_page_program` function always writes no more than 256 bytes of data when using the `spi_flash_cmd_write` function.
The buffer is placed in .bss so that it does not increase stack usage in some use cases.
Tested on Asrock B85M Pro4 (Winbond W25Q64FV), MRC cache still works.
Repost of https://review.coreboot.org/c/coreboot/+/50480 with 'static' so that the buffer is in bss.
Change-Id: I157ecec69c049ead06467b0328efd7d1a09bd268 Signed-off-by: Angel Pons th3fanbus@gmail.com Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h 2 files changed, 9 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/84059/1
diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index 11597c6..2ad1fe9 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -129,17 +129,16 @@ return ret; }
-/* TODO: This code is quite possibly broken and overflowing stacks. Fix ASAP! */ -#pragma GCC diagnostic push -#if defined(__GNUC__) && !defined(__clang__) -#pragma GCC diagnostic ignored "-Wstack-usage=" -#endif -#pragma GCC diagnostic ignored "-Wvla" int spi_flash_cmd_write(const struct spi_slave *spi, const u8 *cmd, size_t cmd_len, const void *data, size_t data_len) { int ret; - u8 buff[cmd_len + data_len]; + /* Put the buffer in .bss to not put strain the stack (it's limited in SMM) */ + static u8 buff[4 + MAX_FLASH_CMD_DATA_SIZE]; + + if (ARRAY_SIZE(buff) < cmd_len + data_len) + return -1; + memcpy(buff, cmd, cmd_len); memcpy(buff + cmd_len, data, data_len);
@@ -151,7 +150,6 @@
return ret; } -#pragma GCC diagnostic pop
/* Perform the read operation honoring spi controller fifo size, reissuing * the read command until the full request completed. */ @@ -316,6 +314,7 @@ byte_addr = offset % page_size; chunk_len = MIN(len - actual, page_size - byte_addr); chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); + chunk_len = MIN(MAX_FLASH_CMD_DATA_SIZE, chunk_len);
spi_flash_addr(offset, cmd); if (CONFIG(DEBUG_SPI_FLASH)) { diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index 3aea914..9ce2f6a 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -27,6 +27,8 @@ /* Common status */ #define STATUS_WIP 0x01
+#define MAX_FLASH_CMD_DATA_SIZE 256 + /* Send a single-byte command to the device and read the response */ int spi_flash_cmd(const struct spi_slave *spi, u8 cmd, void *response, size_t len);