From: Drew Kluemke <[email protected]> board_name_decode() assembles the board name into a calloc'd buffer of MAX_NAME_LENGTH (50) bytes using strcat(). It appends CONFIG_SYS_BOARD and then, for every detected EEPROM descriptor, "-", desc->name (up to 16 characters), "-rev" and desc->revision (up to 8 characters).
The length check runs only after the loop, so it cannot prevent the overflow it documents. Each descriptor contributes up to 29 characters, so a base board plus a carrier card already exceeds the buffer, and the data being concatenated comes from the EEPROM. Use strlcat() bounded to MAX_NAME_LENGTH so the writes are truncated rather than overflowing the allocation. Truncation leaves a string of MAX_NAME_LENGTH - 1 characters, so relax the check accordingly; as written it could never fire once strlcat() is used. Signed-off-by: Drew Kluemke <[email protected]> Signed-off-by: Sriram Sriram <[email protected]> --- board/xilinx/common/board.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c index f45b879736e..690991bb435 100644 --- a/board/xilinx/common/board.c +++ b/board/xilinx/common/board.c @@ -590,38 +590,42 @@ char * __maybe_unused __weak board_name_decode(void) /* The first string should be soc name */ if (!id) - strcat(board_local_name, CONFIG_SYS_BOARD); + strlcat(board_local_name, CONFIG_SYS_BOARD, + MAX_NAME_LENGTH); /* * For two purpose here: * soc_name- eg: zynqmp- * and between base board and CC eg: ..revA-sck... */ - strcat(board_local_name, "-"); + strlcat(board_local_name, "-", MAX_NAME_LENGTH); if (desc->name[0]) { /* For DT composition name needs to be lowercase */ for (i = 0; i < sizeof(desc->name); i++) desc->name[i] = tolower(desc->name[i]); - strcat(board_local_name, desc->name); + strlcat(board_local_name, desc->name, + MAX_NAME_LENGTH); } if (desc->revision[0]) { - strcat(board_local_name, "-rev"); + strlcat(board_local_name, "-rev", + MAX_NAME_LENGTH); /* And revision needs to be uppercase */ for (i = 0; i < sizeof(desc->revision); i++) desc->revision[i] = toupper(desc->revision[i]); - strcat(board_local_name, desc->revision); + strlcat(board_local_name, desc->revision, + MAX_NAME_LENGTH); } } /* - * Longer strings will end up with buffer overflow and potential - * attacks that's why check it + * Longer strings will be truncated by strlcat, check and + * panic if the source data was too long for the buffer. */ - if (strlen(board_local_name) >= MAX_NAME_LENGTH) + if (strlen(board_local_name) >= MAX_NAME_LENGTH - 1) panic("Board name can't be determined\n"); if (strlen(board_local_name)) -- 2.49.0
