Re: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Michal Simek <michal.simek@amd.com>]

On 9/9/26 21:20, Sriram Sriram wrote:
> From: Drew Kluemke <ankluemk@microsoft.com>
> 
> 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 <ankluemk@microsoft.com>
> Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
> ---
>   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);

I actually got a path (c&p below) for the same issue some days ago internally 
and also reviewed it already. But unfortunately it wasn't sent out yet.
The difference is that it is checking len at the end of every loop to get out of 
it earlier.

Thanks,
Michal



  Author:     Pranav Tilak <pranav.vinaytilak@amd.com>
  AuthorDate: Wed Sep 9 10:13:09 2026 +0530

      board: xilinx: Fix heap buffer overflow in board_name_decode()

      board_name_decode() builds the board name by repeatedly appending
      to a fixed MAX_NAME_LENGTH (50-byte) buffer with strcat(), and only
      checks for overflow once, after the entire loop over all detected
      nvmemX EEPROM devices has completed. If enough devices are present
      and their name/revision fields are long enough, strcat() can write
      past the end of the buffer before the overflow check is reached,
      resulting in heap memory corruption.

      Fix this by using strlcat() instead, which bounds every write to
      the destination buffer size and prevents overrunning the allocation.
      Use its return value to detect truncation during board name
      construction instead of relying on a single length check after all
      EEPROM devices have been processed.

      Fixes: 52ff1626cf4c ("xilinx: common: Enabling generic function for DT 
reselection")
      Signed-off-by: Pranav Tilak <pranav.vinaytilak@amd.com>

  diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
  index bdccf0732d2a..964817d98e65 100644
  --- a/board/xilinx/common/board.c
  +++ b/board/xilinx/common/board.c
  @@ -571,8 +571,9 @@ int __maybe_unused board_fit_config_name_match(const char 
*name)

   char * __maybe_unused __weak board_name_decode(void)
   {
  -	char *board_local_name;
   	struct xilinx_board_description *desc;
  +	char *board_local_name;
  +	size_t len = 0;
   	int i, id;

   	board_local_name = calloc(1, MAX_NAME_LENGTH);
  @@ -592,39 +593,36 @@ char * __maybe_unused __weak board_name_decode(void)

   		/* The first string should be soc name */
   		if (!id)
  -			strcat(board_local_name, CONFIG_SYS_BOARD);
  +			len = 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, "-");
  +		len = 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);
  +			len = strlcat(board_local_name, desc->name, MAX_NAME_LENGTH);
   		}
   		if (desc->revision[0]) {
  -			strcat(board_local_name, "-rev");
  +			len = 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);
  +			len = 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
  -	 */
  -	if (strlen(board_local_name) >= MAX_NAME_LENGTH)
  -		panic("Board name can't be determined\n");
  +		/* Reject truncated board names. */
  +		if (len >= MAX_NAME_LENGTH)
  +			panic("Board name can't be determined\n");
  +	}

   	if (strlen(board_local_name))
   		return board_local_name;