Hello Sriram and Drew, It seems to me that the limit of 50 for MAX_NAME_LENGTH is pretty arbitrary. If you expect it to easily overflow, why not make that larger as well?
Kind regards, Maarten Brock > -----Original Message----- > From: Sriram Sriram <[email protected]> > Sent: Wednesday 9 September 2026 21:21 > To: [email protected] > Cc: Michal Simek <[email protected]>; Tom Rini <[email protected]>; Drew > Kluemke <[email protected]>; Sriram > Sriram <[email protected]> > Subject: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode() > > 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
